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
Closed

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

Show file tree
Hide file tree
Changes from all commits
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
5 changes: 4 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 @@ -19,6 +19,9 @@
import java.util.Collections;
import java.util.Map;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.springframework.cloud.dataflow.container.registry.ContainerRegistryProperties;
import org.springframework.core.io.ByteArrayResource;
import org.springframework.core.io.Resource;
Expand All @@ -32,6 +35,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 @@ -62,7 +66,6 @@ public ResponseEntity<Map<String, String>> getBlobRedirect(@RequestHeader("Autho
"&X-Amz-Expires=1200" +
"&X-Amz-SignedHeaders=host" +
"&X-Amz-Signature=test");

return new ResponseEntity<>(redirectHeaders, HttpStatus.TEMPORARY_REDIRECT);
}

Expand Down
30 changes: 20 additions & 10 deletions .../springframework/cloud/dataflow/container/registry/ContainerImageRestTemplateFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.security.cert.X509Certificate;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;
Expand All @@ -31,9 +32,13 @@

import org.apache.hc.client5.http.config.RequestConfig;
import org.apache.hc.client5.http.cookie.StandardCookieSpec;
import org.apache.hc.client5.http.impl.ChainElement;
import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
import org.apache.hc.client5.http.impl.classic.HttpClients;
import org.apache.hc.client5.http.impl.io.BasicHttpClientConnectionManager;
import org.apache.hc.client5.http.impl.routing.DefaultProxyRoutePlanner;
import org.apache.hc.client5.http.impl.routing.DefaultRoutePlanner;
import org.apache.hc.client5.http.socket.ConnectionSocketFactory;
import org.apache.hc.client5.http.socket.PlainConnectionSocketFactory;
import org.apache.hc.client5.http.ssl.NoopHostnameVerifier;
Expand All @@ -44,12 +49,12 @@

import org.springframework.boot.web.client.RestTemplateBuilder;
import org.springframework.cloud.dataflow.container.registry.authorization.DropAuthorizationHeaderRequestRedirectStrategy;
import org.springframework.cloud.dataflow.container.registry.authorization.SpecialRedirectExec;
import org.springframework.http.MediaType;
import org.springframework.http.client.HttpComponentsClientHttpRequestFactory;
import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter;
import org.springframework.web.client.RestTemplate;


/**
* On demand creates a cacheable {@link RestTemplate} instances for the purpose of the Container Registry access.
* Created RestTemplates can be configured to use Http Proxy and/or bypassing the SSL verification.
Expand Down Expand Up @@ -83,6 +88,7 @@
*
* @author Christian Tzolov
* @author Cheng Guan Poh
* @author Corneil du Plessis
*/
public class ContainerImageRestTemplateFactory {

Expand Down Expand Up @@ -197,30 +203,34 @@ private HttpClientBuilder httpClientBuilder(SSLContext sslContext) {
private RestTemplate initRestTemplate(HttpClientBuilder clientBuilder, boolean withHttpProxy, Map<String, String> extra) {

clientBuilder.setDefaultRequestConfig(RequestConfig.custom().setCookieSpec(StandardCookieSpec.RELAXED).build());

DefaultRoutePlanner routePlanner;
// Set the HTTP proxy if configured.
if (withHttpProxy) {
if (!properties.getHttpProxy().isEnabled()) {
throw new ContainerRegistryException("Registry Configuration uses a HttpProxy but non is configured!");
}
HttpHost proxy = new HttpHost(properties.getHttpProxy().getHost(), properties.getHttpProxy().getPort());
clientBuilder.setProxy(proxy);
routePlanner = new DefaultProxyRoutePlanner(proxy, DefaultSchemePortResolver.INSTANCE);
}
else {
routePlanner = new DefaultRoutePlanner(DefaultSchemePortResolver.INSTANCE);
}

HttpComponentsClientHttpRequestFactory customRequestFactory =
new HttpComponentsClientHttpRequestFactory(
clientBuilder
.setRedirectStrategy(new DropAuthorizationHeaderRequestRedirectStrategy(extra))
// Azure redirects may contain double slashes and on default those are normilised
.setDefaultRequestConfig(RequestConfig.custom().build())
.build());
DropAuthorizationHeaderRequestRedirectStrategy redirectStrategy = new DropAuthorizationHeaderRequestRedirectStrategy(extra);
HttpComponentsClientHttpRequestFactory customRequestFactory = new HttpComponentsClientHttpRequestFactory(
clientBuilder.setRedirectStrategy(redirectStrategy)
.replaceExecInterceptor(ChainElement.REDIRECT.name(), new SpecialRedirectExec(routePlanner, redirectStrategy))
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.

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

Copy link
Contributor Author

@corneil corneil Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

// Azure redirects may contain double slashes and on default those are normalised
.setDefaultRequestConfig(RequestConfig.custom().build())
.build());

// DockerHub response's media-type is application/octet-stream although the content is in JSON.
// Similarly the Github CR response's media-type is always text/plain although the content is in JSON.
// Therefore we extend the MappingJackson2HttpMessageConverter media-types to
// include application/octet-stream and text/plain.
MappingJackson2HttpMessageConverter octetSupportJsonConverter = new MappingJackson2HttpMessageConverter();
ArrayList<MediaType> mediaTypeList = new ArrayList(octetSupportJsonConverter.getSupportedMediaTypes());
List<MediaType> mediaTypeList = new ArrayList(octetSupportJsonConverter.getSupportedMediaTypes());
mediaTypeList.add(MediaType.APPLICATION_OCTET_STREAM);
mediaTypeList.add(MediaType.TEXT_PLAIN);
octetSupportJsonConverter.setSupportedMediaTypes(mediaTypeList);
Expand Down
129 changes: 44 additions & 85 deletions ...flow/container/registry/authorization/DropAuthorizationHeaderRequestRedirectStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,10 @@

import java.net.URI;
import java.net.URISyntaxException;
import java.util.Arrays;
import java.util.Map;

import org.apache.hc.client5.http.classic.methods.HttpGet;
import org.apache.hc.client5.http.classic.methods.HttpHead;
import org.apache.hc.client5.http.classic.methods.HttpUriRequestBase;
import org.apache.hc.client5.http.impl.DefaultRedirectStrategy;
import org.apache.hc.core5.http.Header;
import org.apache.hc.core5.http.HttpException;
Expand Down Expand Up @@ -62,6 +60,7 @@
* @author Janne Valkealahti
* @author Christian Tzolov
* @author Cheng Guan Poh
* @author Corneil du Plessis
*/
public class DropAuthorizationHeaderRequestRedirectStrategy extends DefaultRedirectStrategy {

Expand Down Expand Up @@ -92,109 +91,69 @@ public URI getLocationURI(final HttpRequest request, final HttpResponse response
URI httpUriRequest = super.getLocationURI(request, response, context);
String query = httpUriRequest.getQuery();
String method = request.getMethod();

// Handle Amazon requests
if (StringUtils.hasText(query) && query.contains(AMZ_CREDENTIAL)) {
if (isHeadOrGetMethod(method)) {
try {
return new DropAuthorizationHeaderHttpRequestBase(httpUriRequest, method).getUri();
} catch (URISyntaxException e) {
throw new HttpException("Unable to get location URI", e);
}
}
removeAuthorizationHeader(request, false);
return createResponseURI(method, httpUriRequest);
}
}

// Handle Azure requests
try {
if (request.getUri().getRawPath().contains(AZURECR_URI_SUFFIX)) {
if (isHeadOrGetMethod(method)) {
return (new DropAuthorizationHeaderHttpRequestBase(httpUriRequest, method) {
// Drop headers only for the Basic Auth and leave unchanged for OAuth2
@Override
protected boolean isDropHeader(String name, Object value) {
return name.equalsIgnoreCase(AUTHORIZATION_HEADER) && StringUtils.hasText((String) value) && ((String)value).contains(BASIC_AUTH);
}
}).getUri();
}
}


// Handle Custom requests
if (extra.containsKey(CUSTOM_REGISTRY) && request.getUri().getRawPath().contains(extra.get(CUSTOM_REGISTRY))) {
if (isHeadOrGetMethod(method)) {
return new DropAuthorizationHeaderHttpRequestBase(httpUriRequest, method).getUri();
try {
if (request.getUri().getRawPath().contains(AZURECR_URI_SUFFIX)) {
if (isHeadOrGetMethod(method)) {
removeAuthorizationHeader(request, true);
return createResponseURI(method, httpUriRequest);
}
}

// Handle Custom requests
if (extra.containsKey(CUSTOM_REGISTRY)
&& request.getUri().getRawPath().contains(extra.get(CUSTOM_REGISTRY))) {
if (isHeadOrGetMethod(method)) {
removeAuthorizationHeader(request, false);
return createResponseURI(method, httpUriRequest);
}
}
}
} catch (URISyntaxException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice cleanup!

throw new HttpException("Unable to get Locaction URI", e);
catch (URISyntaxException e) {
throw new HttpException("Unable to get Location URI", e);
}
return httpUriRequest;
}

private boolean isHeadOrGetMethod(String method) {
return StringUtils.hasText(method)
&& (method.equalsIgnoreCase(HttpHead.METHOD_NAME) || method.equalsIgnoreCase(HttpGet.METHOD_NAME));
}

/**
* Overrides all header setter methods to filter out the Authorization headers.
*/
static class DropAuthorizationHeaderHttpRequestBase extends HttpUriRequestBase {

private final String method;

DropAuthorizationHeaderHttpRequestBase(URI uri, String method) {
super(method, uri);
this.method = method;
}

@Override
public String getMethod() {
return this.method;
}

@Override
public void addHeader(Header header) {
if (!isDropHeader(header)) {
super.addHeader(header);
private URI createResponseURI(String method, URI httpUriRequest) throws HttpException {
try {
if (isHeadMethod(method)) {
return new HttpHead(httpUriRequest).getUri();
}
}

@Override
public void addHeader(String name, Object value) {
if (!isDropHeader(name, value)) {
super.addHeader(name, value);
else {
return new HttpGet(httpUriRequest).getUri();
}
}

@Override
public void setHeader(Header header) {
if (!isDropHeader(header)) {
super.setHeader(header);
}
catch (URISyntaxException e) {
throw new HttpException("Unable to get location URI", e);
}
}

@Override
public void setHeader(String name, Object value) {
if (!isDropHeader(name, value)) {
super.setHeader(name, value);
private static void removeAuthorizationHeader(HttpRequest request, boolean onlyBasicAuth) {
for (Header header : request.getHeaders()) {
if (header.getName().equalsIgnoreCase(AUTHORIZATION_HEADER)
&& (!onlyBasicAuth || (onlyBasicAuth && header.getValue().contains(BASIC_AUTH)))) {
request.removeHeaders(header.getName());
break;
}
}
}

@Override
public void setHeaders(Header[] headers) {
Header[] filteredHeaders = Arrays.stream(headers)
.filter(header -> !isDropHeader(header))
.toArray(Header[]::new);
super.setHeaders(filteredHeaders);
}

protected boolean isDropHeader(Header header) {
return isDropHeader(header.getName(), header.getValue());
}
private boolean isHeadOrGetMethod(String method) {
return StringUtils.hasText(method)
&& (method.equalsIgnoreCase(HttpHead.METHOD_NAME) || method.equalsIgnoreCase(HttpGet.METHOD_NAME));
}

protected boolean isDropHeader(String name, Object value) {
return name.equalsIgnoreCase(AUTHORIZATION_HEADER);
}
private boolean isHeadMethod(String method) {
return StringUtils.hasText(method) && method.equalsIgnoreCase(HttpHead.METHOD_NAME);
}

}
Loading
Loading