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

corneil · 2024-10-15T12:29:10Z

Edit:
Discovered that RedirectStrategy was change from Apache HTTP Client 4 -> 5. The change meant header changes were not possible.
I copied to RedirectExec interceptor / chain handler and modified it to use the modified request instead of the original request to create a request builder.

…ion header instead of rejecting requests that didn't.

cppwfs · 2024-10-15T13:06:28Z

...oud/dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java

@@ -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")) {


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

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

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

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.

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

Fixes spring-cloud#5989

rstoyanchev

I can't comment on the solution, but it's worth reaching out through one of the channels on https://hc.apache.org/get-involved.html, mailing list or issue tracker. There may be related issues and discussions there as well.

onobc · 2024-10-16T21:48:58Z

This is on the mailing list here w/ 3 options.

corneil · 2024-10-16T22:11:25Z

This is on the mailing list here w/ 3 options.

I will look into a chain handler that strips the header if I can consistently figure out that we are handling a redirect.

onobc · 2024-10-16T22:18:54Z

...ataflow/container/registry/authorization/DropAuthorizationHeaderRequestRedirectStrategy.java

 			}
 		}
-		} catch (URISyntaxException e) {


Nice cleanup!

onobc · 2024-10-16T22:29:56Z

...oud/dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java

@@ -32,9 +35,11 @@

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


[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?

onobc · 2024-10-16T22:36:09Z

...oud/dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java

@@ -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")) {


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

onobc · 2024-10-16T22:39:51Z

...ataflow/container/registry/authorization/DropAuthorizationHeaderRequestRedirectStrategy.java

+				try {
+					if (isHeadMethod(method)) {
+						return new HttpHead(httpUriRequest).getUri();
+					}
+					else {
+						return new HttpGet(httpUriRequest).getUri();
+					}
+				}
+				catch (URISyntaxException e) {
+					throw new HttpException("Unable to get location URI", e);
+				}
+			}


These lines are repeated in all 3 cases. Let's factor this out for DRY.

onobc · 2024-10-16T22:49:12Z

...org/springframework/cloud/dataflow/container/registry/ContainerImageRestTemplateFactory.java

+		DropAuthorizationHeaderRequestRedirectStrategy redirectStrategy = new DropAuthorizationHeaderRequestRedirectStrategy(extra);
+		HttpComponentsClientHttpRequestFactory customRequestFactory = new HttpComponentsClientHttpRequestFactory(
+				clientBuilder.setRedirectStrategy(redirectStrategy)
+					.replaceExecInterceptor(ChainElement.REDIRECT.name(), new SpecialRedirectExec(routePlanner, redirectStrategy))


Based on your reply to the mailing list comment, it sounds like you are going to pursue option 2) which is to append a custom request interceptor immediately after RedirectExec and strip undesired headers from requests.

I appreciate the scrappiness of figuring out how to get this to work in this case but I would really really like to NOT own this copy of the SpecialRedirectExec as we do not have tests for it, it is immediately out of date as soon as we commit it to this repo, and so on so forth the cost of copy/pasta/patch approach. I did find this test example in httpclient around adding interceptors (may or may not be helpful).

https://github.com/apache/httpcomponents-client/blob/master/httpclient5/src/test/java/org/apache/hc/client5/http/examples/ClientInterceptors.java

After a lot of back and forth with extra exec interceptor and request interceptor the only conclusion I have is that we need a modified version of RedirectExec that makes it possible to remove the header in the RedirectStrategy
There is not other way to get involved in the redirect process that doing exactly what RedirectExec does.

onobc

Thanks for tackling this @corneil . It is definitely a hairy section of the codebase and http client 4->5 has changed quite a bit. I do have some concerns/suggestions.

Remove comments from test controller. Reverted ContainerRegistryService Add SpecialRedirectExec to replace RedirectExec. Ensure DropAuthorizationHeaderRequestRedirectStrategy changes request headers used to create the redirect request. Fixes spring-cloud#5989

corneil · 2024-10-24T15:22:55Z

Using a client that supports request changes on redirect is a better option than a custom redirect handler.

The registry stub was rejecting requests that contained an authorizat…

88ddf2b

…ion header instead of rejecting requests that didn't.

corneil requested a review from cppwfs October 15, 2024 12:29

cppwfs requested changes Oct 15, 2024

View reviewed changes

corneil marked this pull request as draft October 15, 2024 15:43

cppwfs added this to the 3.0.x milestone Oct 16, 2024

corneil force-pushed the corneil/fix-registry-tests branch from 2e5b184 to 3700169 Compare October 16, 2024 13:29

corneil changed the title ~~The registry stub was incorrectly rejecting requests that contained an authorization header.~~ The authorization header was not dropped by redirect strategy as expected. Oct 16, 2024

corneil requested review from cppwfs and rstoyanchev October 16, 2024 13:30

Replace RedirectExec with modified chain handler.

9e93935

Fixes spring-cloud#5989

corneil force-pushed the corneil/fix-registry-tests branch from 3700169 to 9e93935 Compare October 16, 2024 13:34

corneil marked this pull request as ready for review October 16, 2024 13:47

corneil requested review from onobc and removed request for rstoyanchev October 16, 2024 14:36

rstoyanchev reviewed Oct 16, 2024

View reviewed changes

onobc reviewed Oct 16, 2024

View reviewed changes

onobc requested changes Oct 16, 2024

View reviewed changes

corneil marked this pull request as draft October 17, 2024 08:56

corneil requested review from onobc and rstoyanchev October 17, 2024 11:23

Extract createResponseURI

8b90386

Remove comments from test controller. Reverted ContainerRegistryService Add SpecialRedirectExec to replace RedirectExec. Ensure DropAuthorizationHeaderRequestRedirectStrategy changes request headers used to create the redirect request. Fixes spring-cloud#5989

corneil force-pushed the corneil/fix-registry-tests branch from 14ebb44 to 8b90386 Compare October 17, 2024 11:55

corneil marked this pull request as ready for review October 17, 2024 11:55

corneil marked this pull request as draft October 21, 2024 08:59

corneil closed this Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

corneil commented Oct 15, 2024 •

edited

Loading

cppwfs Oct 15, 2024

corneil Oct 15, 2024

corneil Oct 15, 2024

corneil Oct 15, 2024 •

edited

Loading

corneil Oct 17, 2024

rstoyanchev left a comment •

edited

Loading

onobc commented Oct 16, 2024

corneil commented Oct 16, 2024

onobc Oct 16, 2024

onobc Oct 16, 2024 •

edited

Loading

corneil Oct 17, 2024

onobc Oct 16, 2024

corneil Oct 17, 2024

onobc Oct 16, 2024

corneil Oct 17, 2024

onobc Oct 16, 2024 •

edited

Loading

corneil Oct 17, 2024 •

edited

Loading

onobc left a comment

corneil commented Oct 24, 2024

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

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

Conversation

corneil commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corneil Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstoyanchev left a comment • edited Loading

Choose a reason for hiding this comment

onobc commented Oct 16, 2024

corneil commented Oct 16, 2024

Choose a reason for hiding this comment

onobc Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onobc Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

corneil Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

onobc left a comment

Choose a reason for hiding this comment

corneil commented Oct 24, 2024

corneil commented Oct 15, 2024 •

edited

Loading

corneil Oct 15, 2024 •

edited

Loading

rstoyanchev left a comment •

edited

Loading

onobc Oct 16, 2024 •

edited

Loading

onobc Oct 16, 2024 •

edited

Loading

corneil Oct 17, 2024 •

edited

Loading