Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The authorization header was not dropped by redirect strategy as expected. #5984

Closed
wants to merge 3 commits into from
+296 −99
Closed

The authorization header was not dropped by redirect strategy as expected. #5984

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88ddf2b
The registry stub was rejecting requests that contained an authorizat…
corneil Oct 15, 2024
9e93935
Replace RedirectExec with modified chain handler.
corneil Oct 16, 2024
8b90386
Extract createResponseURI
corneil Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .../registry/authorization/DropAuthorizationHeaderOnSignedS3RequestRedirectStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@
* @author Adam J. Weigold
* @author Corneil du Plessis
*/
//TODO: Boot3x followup
@Disabled("TODO: Boot3x `org.springframework.web.client.HttpClientErrorException$BadRequest: 400 : [no body]` is thrown by REST Template")
public class DropAuthorizationHeaderOnSignedS3RequestRedirectStrategyTest {
@RegisterExtension
public final static S3SignedRedirectRequestServerResource s3SignedRedirectRequestServerResource =
Expand Down
3 changes: 2 additions & 1 deletion .../dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

/**
* @author Adam J. Weigold
* @author Corneil du Plessis
*/
@RestController
public class S3SignedRedirectRequestController {
Copy link
Contributor

@onobc onobc Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[UNRELATED-TO-YOUR-CHANGES] These 2 controllers are terribly named and there is no comment telling us that these are test controllers used to mimic what S3 signed redirects look like.

Because it is a test, we do not need to instrument it w/ log statements to debug. In that case, we can just walk through it in a debugger. Do you mind removing these debugging log statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Expand Down Expand Up @@ -68,7 +69,7 @@ public ResponseEntity<Map<String, String>> getBlobRedirect(@RequestHeader("Autho

@RequestMapping("/test/docker/registry/v2/blobs/test/data")
public ResponseEntity<Resource> getSignedBlob(@RequestHeader Map<String, String> headers) {
if (headers.containsKey("authorization")) {
if (!headers.containsKey("authorization")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why it worked in 2.11.x when the test said it should not contain authorized but now requires authorized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is then how does the authorization header not get dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

org.springframework.cloud.dataflow.container.registry.authorization.DropAuthorizationHeaderRequestRedirectStrategy doesn't reach the code to check if a header should be dropped.

Copy link
Contributor Author

@corneil corneil Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing org.apache.hc.client5.http.protocol.RedirectStrategy
to org.apache.http.client.RedirectStrategy
shows the return type change from HttpUriRequest to URI
So we will have to figure out where we can actually deal with the headers and if we need to actually drop the Authorization header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial change I made was invalid and reversed.
The Authorization header should be dropped for most of the redirects on container queries.

return new ResponseEntity<>(HttpStatus.BAD_REQUEST);
}
return buildFromString("{\"config\": {\"Labels\": {\"foo\": \"bar\"} } }");
Expand Down
95 changes: 53 additions & 42 deletions .../java/org/springframework/cloud/dataflow/container/registry/ContainerRegistryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.springframework.http.HttpMethod;
import org.springframework.http.ResponseEntity;
import org.springframework.util.StringUtils;
import org.springframework.web.client.RestClientException;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.util.UriComponents;
import org.springframework.web.util.UriComponentsBuilder;
Expand All @@ -46,9 +47,9 @@ public class ContainerRegistryService {

private static final Logger logger = LoggerFactory.getLogger(ContainerRegistryService.class);

private static final List<String> SUPPORTED_MANIFEST_MEDIA_TYPES =
Collections.unmodifiableList(Arrays.asList(ContainerRegistryProperties.OCI_IMAGE_MANIFEST_MEDIA_TYPE,
ContainerRegistryProperties.DOCKER_IMAGE_MANIFEST_MEDIA_TYPE));
private static final List<String> SUPPORTED_MANIFEST_MEDIA_TYPES = Collections
.unmodifiableList(Arrays.asList(ContainerRegistryProperties.OCI_IMAGE_MANIFEST_MEDIA_TYPE,
ContainerRegistryProperties.DOCKER_IMAGE_MANIFEST_MEDIA_TYPE));

private static final String HTTPS_SCHEME = "https";

Expand Down Expand Up @@ -88,35 +89,38 @@ public Map<String, ContainerRegistryConfiguration> getContainerRegistryConfigura
}

/**
* Get the tag information for the given container image identified by its repository and registry.
* The registry information is expected to be set via container registry configuration in SCDF.
* Get the tag information for the given container image identified by its repository
* and registry. The registry information is expected to be set via container registry
* configuration in SCDF.
* @param registryName the container registry name
* @param repositoryName the image repository name
* @return the list of tags for the image
*/
public List<String> getTags(String registryName, String repositoryName) {
try {
ContainerRegistryConfiguration containerRegistryConfiguration = this.registryConfigurations.get(registryName);
ContainerRegistryConfiguration containerRegistryConfiguration = this.registryConfigurations
.get(registryName);
Map<String, String> properties = new HashMap<>();
properties.put(DockerOAuth2RegistryAuthorizer.DOCKER_REGISTRY_REPOSITORY_FIELD_KEY, repositoryName);
HttpHeaders httpHeaders = new HttpHeaders(this.registryAuthorizerMap.get(containerRegistryConfiguration.getAuthorizationType()).getAuthorizationHeaders(
containerRegistryConfiguration, properties));
HttpHeaders httpHeaders = new HttpHeaders(
this.registryAuthorizerMap.get(containerRegistryConfiguration.getAuthorizationType())
.getAuthorizationHeaders(containerRegistryConfiguration, properties));
httpHeaders.set(HttpHeaders.ACCEPT, "application/json");

UriComponents manifestUriComponents = UriComponentsBuilder.newInstance()
.scheme(HTTPS_SCHEME)
.host(containerRegistryConfiguration.getRegistryHost())
.path(TAGS_LIST_PATH)
.build().expand(repositoryName);
.scheme(HTTPS_SCHEME)
.host(containerRegistryConfiguration.getRegistryHost())
.path(TAGS_LIST_PATH)
.build()
.expand(repositoryName);

RestTemplate requestRestTemplate = this.containerImageRestTemplateFactory.getContainerRestTemplate(
containerRegistryConfiguration.isDisableSslVerification(),
containerRegistryConfiguration.isUseHttpProxy(),
containerRegistryConfiguration.getExtra());
containerRegistryConfiguration.isUseHttpProxy(), containerRegistryConfiguration.getExtra());

ResponseEntity<Map> manifest = requestRestTemplate.exchange(manifestUriComponents.toUri(),
HttpMethod.GET, new HttpEntity<>(httpHeaders), Map.class);
return (List<String>) manifest.getBody().get(TAGS_FIELD);
ResponseEntity<Map> manifest = requestRestTemplate.exchange(manifestUriComponents.toUri(), HttpMethod.GET,
new HttpEntity<>(httpHeaders), Map.class);
return (List<String>) manifest.getBody().get(TAGS_FIELD);
}
catch (Exception e) {
logger.error("Exception getting tag information for the {} from {}", repositoryName, registryName);
Expand All @@ -132,28 +136,25 @@ public List<String> getTags(String registryName, String repositoryName) {
public Map getRepositories(String registryName) {
try {
ContainerRegistryConfiguration containerRegistryConfiguration = this.registryConfigurations
.get(registryName);
.get(registryName);
Map<String, String> properties = new HashMap<>();
properties.put(DockerOAuth2RegistryAuthorizer.DOCKER_REGISTRY_REPOSITORY_FIELD_KEY, registryName);
HttpHeaders httpHeaders = new HttpHeaders(
this.registryAuthorizerMap.get(containerRegistryConfiguration.getAuthorizationType())
.getAuthorizationHeaders(
containerRegistryConfiguration, properties));
.getAuthorizationHeaders(containerRegistryConfiguration, properties));
httpHeaders.set(HttpHeaders.ACCEPT, "application/json");
UriComponents manifestUriComponents = UriComponentsBuilder.newInstance()
.scheme(HTTPS_SCHEME)
.host(containerRegistryConfiguration.getRegistryHost())
.path(CATALOG_LIST_PATH)
.build();

.scheme(HTTPS_SCHEME)
.host(containerRegistryConfiguration.getRegistryHost())
.path(CATALOG_LIST_PATH)
.build();

RestTemplate requestRestTemplate = this.containerImageRestTemplateFactory.getContainerRestTemplate(
containerRegistryConfiguration.isDisableSslVerification(),
containerRegistryConfiguration.isUseHttpProxy(),
containerRegistryConfiguration.getExtra());
containerRegistryConfiguration.isUseHttpProxy(), containerRegistryConfiguration.getExtra());

ResponseEntity<Map> manifest = requestRestTemplate.exchange(manifestUriComponents.toUri(),
HttpMethod.GET, new HttpEntity<>(httpHeaders), Map.class);
ResponseEntity<Map> manifest = requestRestTemplate.exchange(manifestUriComponents.toUri(), HttpMethod.GET,
new HttpEntity<>(httpHeaders), Map.class);
return manifest.getBody();
}
catch (Exception e) {
Expand Down Expand Up @@ -206,18 +207,20 @@ public <T> T getImageManifest(ContainerRegistryRequest registryRequest, Class<T>
// Docker Registry HTTP V2 API pull manifest
ContainerImage containerImage = registryRequest.getContainerImage();
UriComponents manifestUriComponents = UriComponentsBuilder.newInstance()
.scheme(HTTPS_SCHEME)
.host(containerImage.getHostname())
.port(StringUtils.hasText(containerImage.getPort()) ? containerImage.getPort() : null)
.path(IMAGE_MANIFEST_REFERENCE_PATH)
.build().expand(containerImage.getRepository(), containerImage.getRepositoryReference());
.scheme(HTTPS_SCHEME)
.host(containerImage.getHostname())
.port(StringUtils.hasText(containerImage.getPort()) ? containerImage.getPort() : null)
.path(IMAGE_MANIFEST_REFERENCE_PATH)
.build()
.expand(containerImage.getRepository(), containerImage.getRepositoryReference());

ResponseEntity<T> manifest = registryRequest.getRestTemplate().exchange(manifestUriComponents.toUri(),
HttpMethod.GET, new HttpEntity<>(httpHeaders), responseClassType);
ResponseEntity<T> manifest = registryRequest.getRestTemplate()
.exchange(manifestUriComponents.toUri(), HttpMethod.GET, new HttpEntity<>(httpHeaders), responseClassType);
return manifest.getBody();
}

public <T> T getImageBlob(ContainerRegistryRequest registryRequest, String configDigest, Class<T> responseClassType) {
public <T> T getImageBlob(ContainerRegistryRequest registryRequest, String configDigest,
Class<T> responseClassType) {
ContainerImage containerImage = registryRequest.getContainerImage();
HttpHeaders httpHeaders = new HttpHeaders(registryRequest.getAuthHttpHeaders());

Expand All @@ -227,11 +230,19 @@ public <T> T getImageBlob(ContainerRegistryRequest registryRequest, String confi
.host(containerImage.getHostname())
.port(StringUtils.hasText(containerImage.getPort()) ? containerImage.getPort() : null)
.path(IMAGE_BLOB_DIGEST_PATH)
.build().expand(containerImage.getRepository(), configDigest);

ResponseEntity<T> blob = registryRequest.getRestTemplate().exchange(blobUriComponents.toUri(),
HttpMethod.GET, new HttpEntity<>(httpHeaders), responseClassType);
.build()
.expand(containerImage.getRepository(), configDigest);
try {
logger.info("getImageBlob:request:{},{}", blobUriComponents.toUri(), httpHeaders);
ResponseEntity<T> blob = registryRequest.getRestTemplate()
.exchange(blobUriComponents.toUri(), HttpMethod.GET, new HttpEntity<>(httpHeaders), responseClassType);

return blob.getBody();
return blob.getStatusCode().is2xxSuccessful() ? blob.getBody() : null;
}
catch (RestClientException x) {
logger.error("getImageBlob:exception:" + x, x);
return null;
}
}

}
3 changes: 2 additions & 1 deletion .../dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

/**
* @author Adam J. Weigold
* @author Corneil du Plessis
*/
@RestController
public class S3SignedRedirectRequestController {
Expand Down Expand Up @@ -68,7 +69,7 @@ public ResponseEntity<Map<String, String>> getBlobRedirect(@RequestHeader("Autho

@RequestMapping("/test/docker/registry/v2/blobs/test/data")
public ResponseEntity<Resource> getSignedBlob(@RequestHeader Map<String, String> headers) {
if (headers.containsKey("authorization")) {
if (!headers.containsKey("authorization")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be removed as it is expected that the auth header has been removed. Same as the other S3 test controller thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return new ResponseEntity<>(HttpStatus.BAD_REQUEST);
}
return buildFromString("{\"config\": {\"Labels\": {\"foo\": \"bar\"} } }");
Expand Down
Loading