From 9e939352029e4393703ee6669439d7cb67fca1fc Mon Sep 17 00:00:00 2001 From: Corneil du Plessis Date: Wed, 16 Oct 2024 15:34:14 +0200 Subject: [PATCH] Replace RedirectExec with modified chain handler. Fixes #5989 --- .../S3SignedRedirectRequestController.java | 11 +- .../ContainerImageRestTemplateFactory.java | 30 ++- ...rizationHeaderRequestRedirectStrategy.java | 146 +++++------ .../authorization/SpecialRedirectExec.java | 226 ++++++++++++++++++ 4 files changed, 315 insertions(+), 98 deletions(-) create mode 100644 spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/authorization/SpecialRedirectExec.java diff --git a/spring-cloud-dataflow-configuration-metadata/src/test/java/org/springframework/cloud/dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java b/spring-cloud-dataflow-configuration-metadata/src/test/java/org/springframework/cloud/dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java index d7b3c5ab1e..3762c0ab76 100644 --- a/spring-cloud-dataflow-configuration-metadata/src/test/java/org/springframework/cloud/dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java +++ b/spring-cloud-dataflow-configuration-metadata/src/test/java/org/springframework/cloud/dataflow/container/registry/authorization/support/S3SignedRedirectRequestController.java @@ -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; @@ -36,6 +39,7 @@ */ @RestController public class S3SignedRedirectRequestController { + private final static Logger logger = LoggerFactory.getLogger(S3SignedRedirectRequestController.class); @RequestMapping("/service/token") public ResponseEntity> getToken() { @@ -53,6 +57,7 @@ public ResponseEntity getManifests(@RequestHeader("Authorization") Str @RequestMapping("/v2/test/s3-redirect-image/blobs/signed_redirect_digest") public ResponseEntity> getBlobRedirect(@RequestHeader("Authorization") String token) { if (!"bearer my_token_999".equals(token.trim().toLowerCase())) { + logger.info("getBlobRedirect=BAD_REQUEST"); return new ResponseEntity<>(HttpStatus.BAD_REQUEST); } HttpHeaders redirectHeaders = new HttpHeaders(); @@ -63,13 +68,15 @@ public ResponseEntity> getBlobRedirect(@RequestHeader("Autho "&X-Amz-Expires=1200" + "&X-Amz-SignedHeaders=host" + "&X-Amz-Signature=test"); - + logger.info("getBlobRedirect:{}", redirectHeaders); return new ResponseEntity<>(redirectHeaders, HttpStatus.TEMPORARY_REDIRECT); } @RequestMapping("/test/docker/registry/v2/blobs/test/data") public ResponseEntity getSignedBlob(@RequestHeader Map headers) { - if (!headers.containsKey("authorization")) { + logger.info("getSignedBlob:{}", headers); + if (headers.containsKey("authorization")) { + logger.info("getSignedBlob=BAD_REQUEST"); return new ResponseEntity<>(HttpStatus.BAD_REQUEST); } return buildFromString("{\"config\": {\"Labels\": {\"foo\": \"bar\"} } }"); diff --git a/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/ContainerImageRestTemplateFactory.java b/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/ContainerImageRestTemplateFactory.java index 6e6c78099f..936ce3daff 100644 --- a/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/ContainerImageRestTemplateFactory.java +++ b/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/ContainerImageRestTemplateFactory.java @@ -31,9 +31,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; @@ -44,12 +48,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. @@ -83,6 +87,7 @@ * * @author Christian Tzolov * @author Cheng Guan Poh + * @author Corneil du Plessis */ public class ContainerImageRestTemplateFactory { @@ -197,7 +202,7 @@ private HttpClientBuilder httpClientBuilder(SSLContext sslContext) { private RestTemplate initRestTemplate(HttpClientBuilder clientBuilder, boolean withHttpProxy, Map extra) { clientBuilder.setDefaultRequestConfig(RequestConfig.custom().setCookieSpec(StandardCookieSpec.RELAXED).build()); - + DefaultRoutePlanner routePlanner; // Set the HTTP proxy if configured. if (withHttpProxy) { if (!properties.getHttpProxy().isEnabled()) { @@ -205,15 +210,22 @@ private RestTemplate initRestTemplate(HttpClientBuilder clientBuilder, boolean w } 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)) + // Azure redirects may contain double slashes and on default those are + // normilised + .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. diff --git a/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/authorization/DropAuthorizationHeaderRequestRedirectStrategy.java b/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/authorization/DropAuthorizationHeaderRequestRedirectStrategy.java index e7741b9562..53e98c84dd 100644 --- a/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/authorization/DropAuthorizationHeaderRequestRedirectStrategy.java +++ b/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/authorization/DropAuthorizationHeaderRequestRedirectStrategy.java @@ -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; @@ -62,6 +60,7 @@ * @author Janne Valkealahti * @author Christian Tzolov * @author Cheng Guan Poh + * @author Corneil du Plessis */ public class DropAuthorizationHeaderRequestRedirectStrategy extends DefaultRedirectStrategy { @@ -92,109 +91,82 @@ 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, response, false); + 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); + } + } } // 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, response, true); + if (isHeadMethod(method)) { + return new HttpHead(httpUriRequest).getUri(); + } + else { + return new HttpGet(httpUriRequest).getUri(); + } + } + } + + // Handle Custom requests + if (extra.containsKey(CUSTOM_REGISTRY) + && request.getUri().getRawPath().contains(extra.get(CUSTOM_REGISTRY))) { + if (isHeadOrGetMethod(method)) { + removeAuthorizationHeader(request, response, false); + if (isHeadMethod(method)) { + return new HttpHead(httpUriRequest).getUri(); + } + else { + return new HttpGet(httpUriRequest).getUri(); + } + } } } - } catch (URISyntaxException e) { + catch (URISyntaxException e) { throw new HttpException("Unable to get Locaction 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 static void removeAuthorizationHeader(HttpRequest request, HttpResponse response, boolean onlyBasicAuth) { + for (Header header : response.getHeaders()) { + if (header.getName().equalsIgnoreCase(AUTHORIZATION_HEADER) + && (!onlyBasicAuth || (onlyBasicAuth && header.getValue().contains(BASIC_AUTH)))) { + response.removeHeaders(header.getName()); + break; } } - - @Override - public void addHeader(String name, Object value) { - if (!isDropHeader(name, value)) { - super.addHeader(name, value); + 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 setHeader(Header header) { - if (!isDropHeader(header)) { - super.setHeader(header); - } - } - - @Override - public void setHeader(String name, Object value) { - if (!isDropHeader(name, value)) { - super.setHeader(name, value); - } - } - - @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); } + } diff --git a/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/authorization/SpecialRedirectExec.java b/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/authorization/SpecialRedirectExec.java new file mode 100644 index 0000000000..282f5891bf --- /dev/null +++ b/spring-cloud-dataflow-container-registry/src/main/java/org/springframework/cloud/dataflow/container/registry/authorization/SpecialRedirectExec.java @@ -0,0 +1,226 @@ +package org.springframework.cloud.dataflow.container.registry.authorization; + +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ + +import java.io.IOException; +import java.net.URI; +import java.util.Objects; + +import org.apache.hc.client5.http.CircularRedirectException; +import org.apache.hc.client5.http.HttpRoute; +import org.apache.hc.client5.http.RedirectException; +import org.apache.hc.client5.http.auth.AuthExchange; +import org.apache.hc.client5.http.classic.ExecChain; +import org.apache.hc.client5.http.classic.ExecChainHandler; +import org.apache.hc.client5.http.config.RequestConfig; +import org.apache.hc.client5.http.protocol.HttpClientContext; +import org.apache.hc.client5.http.protocol.RedirectLocations; +import org.apache.hc.client5.http.protocol.RedirectStrategy; +import org.apache.hc.client5.http.routing.HttpRoutePlanner; +import org.apache.hc.client5.http.utils.URIUtils; +import org.apache.hc.core5.annotation.Contract; +import org.apache.hc.core5.annotation.Internal; +import org.apache.hc.core5.annotation.ThreadingBehavior; +import org.apache.hc.core5.http.ClassicHttpRequest; +import org.apache.hc.core5.http.ClassicHttpResponse; +import org.apache.hc.core5.http.HttpEntity; +import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpStatus; +import org.apache.hc.core5.http.Method; +import org.apache.hc.core5.http.ProtocolException; +import org.apache.hc.core5.http.io.entity.EntityUtils; +import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; +import org.apache.hc.core5.util.Args; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Essential copy of RedirectExec to override the behaviour allowing the use of current request when creating a request builder. + * + * @author Corneil du Plessis + */ +@Contract(threading = ThreadingBehavior.STATELESS) +@Internal +public final class SpecialRedirectExec implements ExecChainHandler { + + private static final Logger LOG = LoggerFactory.getLogger(SpecialRedirectExec.class); + + private final RedirectStrategy redirectStrategy; + private final HttpRoutePlanner routePlanner; + + public SpecialRedirectExec( + final HttpRoutePlanner routePlanner, + final RedirectStrategy redirectStrategy) { + super(); + Args.notNull(routePlanner, "HTTP route planner"); + Args.notNull(redirectStrategy, "HTTP redirect strategy"); + this.routePlanner = routePlanner; + this.redirectStrategy = redirectStrategy; + } + + @Override + public ClassicHttpResponse execute( + final ClassicHttpRequest request, + final ExecChain.Scope scope, + final ExecChain chain) throws IOException, HttpException { + Args.notNull(request, "HTTP request"); + Args.notNull(scope, "Scope"); + + final HttpClientContext context = scope.clientContext; + RedirectLocations redirectLocations = context.getRedirectLocations(); + if (redirectLocations == null) { + redirectLocations = new RedirectLocations(); + context.setAttribute(HttpClientContext.REDIRECT_LOCATIONS, redirectLocations); + } + redirectLocations.clear(); + + final RequestConfig config = context.getRequestConfig(); + final int maxRedirects = config.getMaxRedirects() > 0 ? config.getMaxRedirects() : 50; + ClassicHttpRequest originalRequest = scope.originalRequest; + ClassicHttpRequest currentRequest = request; + ExecChain.Scope currentScope = scope; + for (int redirectCount = 0;;) { + final String exchangeId = currentScope.exchangeId; + final ClassicHttpResponse response = chain.proceed(currentRequest, currentScope); + try { + if (config.isRedirectsEnabled() && this.redirectStrategy.isRedirected(request, response, context)) { + final HttpEntity requestEntity = request.getEntity(); + if (requestEntity != null && !requestEntity.isRepeatable()) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} cannot redirect non-repeatable request", exchangeId); + } + return response; + } + if (redirectCount >= maxRedirects) { + throw new RedirectException("Maximum redirects ("+ maxRedirects + ") exceeded"); + } + redirectCount++; + + final URI redirectUri = this.redirectStrategy.getLocationURI(currentRequest, response, context); + if (LOG.isDebugEnabled()) { + LOG.debug("{} redirect requested to location '{}'", exchangeId, redirectUri); + } + + final HttpHost newTarget = URIUtils.extractHost(redirectUri); + if (newTarget == null) { + throw new ProtocolException("Redirect URI does not specify a valid host name: " + + redirectUri); + } + + if (!config.isCircularRedirectsAllowed()) { + if (redirectLocations.contains(redirectUri)) { + throw new CircularRedirectException("Circular redirect to '" + redirectUri + "'"); + } + } + redirectLocations.add(redirectUri); + + final int statusCode = response.getCode(); + final ClassicRequestBuilder redirectBuilder; + switch (statusCode) { + case HttpStatus.SC_MOVED_PERMANENTLY: + case HttpStatus.SC_MOVED_TEMPORARILY: + if (Method.POST.isSame(request.getMethod())) { + redirectBuilder = ClassicRequestBuilder.get(); + } else { + redirectBuilder = ClassicRequestBuilder.copy(currentRequest); + } + break; + case HttpStatus.SC_SEE_OTHER: + if (!Method.GET.isSame(request.getMethod()) && !Method.HEAD.isSame(request.getMethod())) { + redirectBuilder = ClassicRequestBuilder.get(); + } else { + redirectBuilder = ClassicRequestBuilder.copy(currentRequest); + } + break; + default: + redirectBuilder = ClassicRequestBuilder.copy(currentRequest); + } + redirectBuilder.setUri(redirectUri); + + final HttpRoute currentRoute = currentScope.route; + if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) { + final HttpRoute newRoute = this.routePlanner.determineRoute(newTarget, context); + if (!Objects.equals(currentRoute, newRoute)) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} new route required", exchangeId); + } + final AuthExchange targetAuthExchange = context.getAuthExchange(currentRoute.getTargetHost()); + if (LOG.isDebugEnabled()) { + LOG.debug("{} resetting target auth state", exchangeId); + } + targetAuthExchange.reset(); + if (currentRoute.getProxyHost() != null) { + final AuthExchange proxyAuthExchange = context.getAuthExchange(currentRoute.getProxyHost()); + if (proxyAuthExchange.isConnectionBased()) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} resetting proxy auth state", exchangeId); + } + proxyAuthExchange.reset(); + } + } + currentScope = new ExecChain.Scope( + currentScope.exchangeId, + newRoute, + currentScope.originalRequest, + currentScope.execRuntime, + currentScope.clientContext); + } + } + + if (LOG.isDebugEnabled()) { + LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute); + } + originalRequest = redirectBuilder.build(); + currentRequest = redirectBuilder.build(); + + EntityUtils.consume(response.getEntity()); + response.close(); + } else { + return response; + } + } catch (final RuntimeException | IOException ex) { + response.close(); + throw ex; + } catch (final HttpException ex) { + // Protocol exception related to a direct. + // The underlying connection may still be salvaged. + try { + EntityUtils.consume(response.getEntity()); + } catch (final IOException ioex) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} I/O error while releasing connection", exchangeId, ioex); + } + } finally { + response.close(); + } + throw ex; + } + } + } +}