From fb0c0737836a73eb162b564d72d13084008ff724 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sun, 26 Sep 2021 11:49:59 +0200 Subject: [PATCH] RFC 7230: treat presence of a userinfo component in request URI as an HTTP protocol violation --- ...AbstractHttpAsyncClientAuthentication.java | 59 ++----------- .../sync/TestClientAuthentication.java | 62 +------------- .../hc/client5/http/impl/AuthSupport.java | 85 ------------------- .../http/impl/async/AsyncProtocolExec.java | 8 +- .../http/impl/classic/ProtocolExec.java | 8 +- .../hc/client5/http/impl/TestAuthSupport.java | 52 ------------ .../http/impl/classic/TestProtocolExec.java | 18 +--- 7 files changed, 14 insertions(+), 278 deletions(-) delete mode 100644 httpclient5/src/main/java/org/apache/hc/client5/http/impl/AuthSupport.java delete mode 100644 httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestAuthSupport.java diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncClientAuthentication.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncClientAuthentication.java index 5de45111b..7a9819201 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncClientAuthentication.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncClientAuthentication.java @@ -30,6 +30,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Collectors; @@ -55,7 +56,6 @@ import org.apache.hc.client5.testing.BasicTestAuthenticator; import org.apache.hc.client5.testing.auth.Authenticator; import org.apache.hc.core5.function.Decorator; import org.apache.hc.core5.http.ContentType; -import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequestInterceptor; @@ -63,15 +63,14 @@ import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpResponseInterceptor; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.HttpVersion; +import org.apache.hc.core5.http.ProtocolException; import org.apache.hc.core5.http.URIScheme; import org.apache.hc.core5.http.config.Http1Config; import org.apache.hc.core5.http.config.Lookup; import org.apache.hc.core5.http.config.Registry; import org.apache.hc.core5.http.config.RegistryBuilder; import org.apache.hc.core5.http.impl.HttpProcessors; -import org.apache.hc.core5.http.message.BasicHeader; import org.apache.hc.core5.http.nio.AsyncServerExchangeHandler; -import org.apache.hc.core5.http.protocol.HttpCoreContext; import org.apache.hc.core5.http.support.BasicResponseBuilder; import org.apache.hc.core5.http2.config.H2Config; import org.apache.hc.core5.http2.impl.H2Processors; @@ -348,24 +347,6 @@ public abstract class AbstractHttpAsyncClientAuthentication future = httpclient.execute( - SimpleRequestBuilder.get() - .setScheme(target.getSchemeName()) - .setAuthority(new URIAuthority("test:test", target.getHostName(), target.getPort())) - .setPath("/") - .build(), context, null); - final SimpleHttpResponse response = future.get(); - - Assert.assertNotNull(response); - Assert.assertEquals(HttpStatus.SC_OK, response.getCode()); - } - @Test public void testAuthenticationUserinfoInRequestFailure() throws Exception { server.register("*", AsyncEchoHandler::new); @@ -374,41 +355,11 @@ public abstract class AbstractHttpAsyncClientAuthentication future = httpclient.execute(SimpleRequestBuilder.get() .setScheme(target.getSchemeName()) - .setAuthority(new URIAuthority("test:all-worng", target.getHostName(), target.getPort())) + .setAuthority(new URIAuthority("test:test", target.getHostName(), target.getPort())) .setPath("/") .build(), context, null); - final SimpleHttpResponse response = future.get(); - - Assert.assertNotNull(response); - Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getCode()); - } - - @Test - public void testAuthenticationUserinfoInRedirectSuccess() throws Exception { - server.register("*", AsyncEchoHandler::new); - final HttpHost target = start(); - server.register("/thatway", () -> new AbstractSimpleServerExchangeHandler() { - - @Override - protected SimpleHttpResponse handle( - final SimpleHttpRequest request, final HttpCoreContext context) throws HttpException { - final SimpleHttpResponse response = new SimpleHttpResponse(HttpStatus.SC_MOVED_PERMANENTLY); - response.addHeader(new BasicHeader("Location", target.getSchemeName() + "://test:test@" + target.toHostString() + "/")); - return response; - } - }); - - final HttpClientContext context = HttpClientContext.create(); - final Future future = httpclient.execute( - SimpleRequestBuilder.get() - .setScheme(target.getSchemeName()) - .setAuthority(new URIAuthority("test:test", target.getHostName(), target.getPort())) - .setPath("/thatway") - .build(), context, null); - final SimpleHttpResponse response = future.get(); - - Assert.assertNotNull(response); - Assert.assertEquals(HttpStatus.SC_OK, response.getCode()); + final ExecutionException exception = Assert.assertThrows(ExecutionException.class, () -> future.get()); + MatcherAssert.assertThat(exception.getCause(), CoreMatchers.instanceOf(ProtocolException.class)); } @Test diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java index 0af41e11c..80c283471 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java @@ -28,7 +28,6 @@ package org.apache.hc.client5.testing.sync; import java.io.ByteArrayInputStream; import java.io.IOException; -import java.net.InetSocketAddress; import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collections; @@ -37,6 +36,7 @@ import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Collectors; +import org.apache.hc.client5.http.ClientProtocolException; import org.apache.hc.client5.http.auth.AuthCache; import org.apache.hc.client5.http.auth.AuthScheme; import org.apache.hc.client5.http.auth.AuthSchemeFactory; @@ -61,7 +61,6 @@ import org.apache.hc.client5.testing.classic.AuthenticatingDecorator; import org.apache.hc.client5.testing.classic.EchoHandler; import org.apache.hc.core5.http.ClassicHttpRequest; import org.apache.hc.core5.http.ClassicHttpResponse; -import org.apache.hc.core5.http.EndpointDetails; import org.apache.hc.core5.http.HeaderElements; import org.apache.hc.core5.http.HttpEntity; import org.apache.hc.core5.http.HttpException; @@ -76,9 +75,7 @@ import org.apache.hc.core5.http.io.HttpRequestHandler; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.http.io.entity.InputStreamEntity; import org.apache.hc.core5.http.io.entity.StringEntity; -import org.apache.hc.core5.http.message.BasicHeader; import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.hc.core5.http.protocol.HttpCoreContext; import org.apache.hc.core5.http.support.BasicResponseBuilder; import org.apache.hc.core5.net.URIAuthority; import org.hamcrest.CoreMatchers; @@ -434,66 +431,13 @@ public class TestClientAuthentication extends LocalServerTestBase { } @Test - public void testAuthenticationUserinfoInRequestSuccess() throws Exception { + public void testAuthenticationUserinfoInRequest() throws Exception { this.server.registerHandler("*", new EchoHandler()); final HttpHost target = start(); final HttpGet httpget = new HttpGet("http://test:test@" + target.toHostString() + "/"); final HttpClientContext context = HttpClientContext.create(); - try (final ClassicHttpResponse response = this.httpclient.execute(target, httpget, context)) { - final HttpEntity entity = response.getEntity(); - Assert.assertEquals(HttpStatus.SC_OK, response.getCode()); - Assert.assertNotNull(entity); - EntityUtils.consume(entity); - } - } - - @Test - public void testAuthenticationUserinfoInRequestFailure() throws Exception { - this.server.registerHandler("*", new EchoHandler()); - final HttpHost target = start(); - final HttpGet httpget = new HttpGet("http://test:all-wrong@" + target.toHostString() + "/"); - - final HttpClientContext context = HttpClientContext.create(); - try (final ClassicHttpResponse response = this.httpclient.execute(target, httpget, context)) { - final HttpEntity entity = response.getEntity(); - Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, response.getCode()); - Assert.assertNotNull(entity); - EntityUtils.consume(entity); - } - } - - @Test - public void testAuthenticationUserinfoInRedirectSuccess() throws Exception { - this.server.registerHandler("/*", new EchoHandler()); - this.server.registerHandler("/thatway", (request, response, context) -> { - final EndpointDetails endpoint = (EndpointDetails) context.getAttribute(HttpCoreContext.CONNECTION_ENDPOINT); - final InetSocketAddress socketAddress = (InetSocketAddress) endpoint.getLocalAddress(); - final int port = socketAddress.getPort(); - response.setCode(HttpStatus.SC_MOVED_PERMANENTLY); - response.addHeader(new BasicHeader("Location", "http://test:test@localhost:" + port + "/secure")); - }); - - final HttpHost target = start(new BasicTestAuthenticator("test:test", "test realm") { - - @Override - public boolean authenticate(final URIAuthority authority, final String requestUri, final String credentials) { - if (requestUri.equals("/secure") || requestUri.startsWith("/secure/")) { - return super.authenticate(authority, requestUri, credentials); - } - return true; - } - }); - - final HttpGet httpget = new HttpGet("/thatway"); - final HttpClientContext context = HttpClientContext.create(); - - try (final ClassicHttpResponse response = this.httpclient.execute(target, httpget, context)) { - final HttpEntity entity = response.getEntity(); - Assert.assertEquals(HttpStatus.SC_OK, response.getCode()); - Assert.assertNotNull(entity); - EntityUtils.consume(entity); - } + Assert.assertThrows(ClientProtocolException.class, () -> this.httpclient.execute(target, httpget, context)); } @Test diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/AuthSupport.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/AuthSupport.java deleted file mode 100644 index 8de956ff9..000000000 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/AuthSupport.java +++ /dev/null @@ -1,85 +0,0 @@ -/* - * ==================================================================== - * 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 - * . - * - */ - -package org.apache.hc.client5.http.impl; - -import org.apache.hc.client5.http.HttpRoute; -import org.apache.hc.client5.http.auth.StandardAuthScheme; -import org.apache.hc.client5.http.auth.AuthScope; -import org.apache.hc.client5.http.auth.CredentialsStore; -import org.apache.hc.client5.http.auth.UsernamePasswordCredentials; -import org.apache.hc.core5.annotation.Internal; -import org.apache.hc.core5.http.HttpHost; -import org.apache.hc.core5.http.HttpRequest; -import org.apache.hc.core5.net.URIAuthority; -import org.apache.hc.core5.util.Args; - -/** - * Authentication support methods. - * - * @since 5.0 - */ -@Internal -public class AuthSupport { - - public static void extractFromAuthority( - final String scheme, - final URIAuthority authority, - final CredentialsStore credentialsStore) { - Args.notNull(credentialsStore, "Credentials store"); - if (authority == null) { - return; - } - final String userInfo = authority.getUserInfo(); - if (userInfo == null) { - return; - } - final int atColon = userInfo.indexOf(':'); - final String userName = atColon >= 0 ? userInfo.substring(0, atColon) : userInfo; - final char[] password = atColon >= 0 ? userInfo.substring(atColon + 1).toCharArray() : null; - - credentialsStore.setCredentials( - new AuthScope(scheme, authority.getHostName(), authority.getPort(), null, StandardAuthScheme.BASIC), - new UsernamePasswordCredentials(userName, password)); - } - - public static HttpHost resolveAuthTarget(final HttpRequest request, final HttpRoute route) { - Args.notNull(request, "Request"); - Args.notNull(route, "Route"); - final URIAuthority authority = request.getAuthority(); - final String scheme = request.getScheme(); - final HttpHost target = authority != null ? new HttpHost(scheme, authority) : route.getTargetHost(); - if (target.getPort() < 0) { - return new HttpHost( - target.getSchemeName(), - target.getHostName(), - route.getTargetHost().getPort()); - } - return target; - } - -} diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java index 7e285347c..c4d11b47f 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java @@ -39,10 +39,7 @@ import org.apache.hc.client5.http.async.AsyncExecChainHandler; import org.apache.hc.client5.http.async.AsyncExecRuntime; import org.apache.hc.client5.http.auth.AuthExchange; import org.apache.hc.client5.http.auth.ChallengeType; -import org.apache.hc.client5.http.auth.CredentialsProvider; -import org.apache.hc.client5.http.auth.CredentialsStore; import org.apache.hc.client5.http.config.RequestConfig; -import org.apache.hc.client5.http.impl.AuthSupport; import org.apache.hc.client5.http.impl.RequestSupport; import org.apache.hc.client5.http.impl.auth.AuthCacheKeeper; import org.apache.hc.client5.http.impl.auth.HttpAuthenticator; @@ -143,9 +140,8 @@ public final class AsyncProtocolExec implements AsyncExecChainHandler { } final URIAuthority authority = request.getAuthority(); - final CredentialsProvider credsProvider = clientContext.getCredentialsProvider(); - if (credsProvider instanceof CredentialsStore) { - AuthSupport.extractFromAuthority(request.getScheme(), authority, (CredentialsStore) credsProvider); + if (authority.getUserInfo() != null) { + throw new ProtocolException("Request URI authority contains deprecated userinfo component"); } final HttpHost target = new HttpHost(request.getScheme(), request.getAuthority()); diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java index 95f954567..37e7cc563 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java @@ -35,13 +35,10 @@ import org.apache.hc.client5.http.HttpRoute; import org.apache.hc.client5.http.SchemePortResolver; import org.apache.hc.client5.http.auth.AuthExchange; import org.apache.hc.client5.http.auth.ChallengeType; -import org.apache.hc.client5.http.auth.CredentialsProvider; -import org.apache.hc.client5.http.auth.CredentialsStore; import org.apache.hc.client5.http.classic.ExecChain; import org.apache.hc.client5.http.classic.ExecChainHandler; import org.apache.hc.client5.http.classic.ExecRuntime; import org.apache.hc.client5.http.config.RequestConfig; -import org.apache.hc.client5.http.impl.AuthSupport; import org.apache.hc.client5.http.impl.RequestSupport; import org.apache.hc.client5.http.impl.auth.AuthCacheKeeper; import org.apache.hc.client5.http.impl.auth.HttpAuthenticator; @@ -146,9 +143,8 @@ public final class ProtocolExec implements ExecChainHandler { } final URIAuthority authority = request.getAuthority(); - final CredentialsProvider credsProvider = context.getCredentialsProvider(); - if (credsProvider instanceof CredentialsStore) { - AuthSupport.extractFromAuthority(request.getScheme(), authority, (CredentialsStore) credsProvider); + if (authority.getUserInfo() != null) { + throw new ProtocolException("Request URI authority contains deprecated userinfo component"); } final HttpHost target = new HttpHost(request.getScheme(), request.getAuthority()); diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestAuthSupport.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestAuthSupport.java deleted file mode 100644 index 1101050a9..000000000 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestAuthSupport.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * ==================================================================== - * 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 - * . - * - */ -package org.apache.hc.client5.http.impl; - -import org.apache.hc.client5.http.auth.AuthScope; -import org.apache.hc.client5.http.auth.Credentials; -import org.apache.hc.client5.http.impl.auth.BasicCredentialsProvider; -import org.apache.hc.core5.net.URIAuthority; -import org.junit.Assert; -import org.junit.Test; - -/** - * Simple tests for {@link AuthSupport}. - */ -public class TestAuthSupport { - - @Test - public void testExtractFromAuthority() { - final URIAuthority uriAuthority = new URIAuthority("testUser", "localhost", 8080); - final BasicCredentialsProvider basicCredentialsProvider = new BasicCredentialsProvider(); - - AuthSupport.extractFromAuthority("http", uriAuthority, basicCredentialsProvider); - - final Credentials credentials = basicCredentialsProvider.getCredentials(new AuthScope("localhost", 8080), null); - Assert.assertEquals("testUser", credentials.getUserPrincipal().getName()); - Assert.assertNull(credentials.getPassword()); - } -} diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java index e0d78dc8a..b67cad06e 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java @@ -30,7 +30,6 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; -import java.net.URI; import java.util.Collections; import org.apache.hc.client5.http.AuthenticationStrategy; @@ -38,15 +37,12 @@ import org.apache.hc.client5.http.HttpRoute; import org.apache.hc.client5.http.auth.AuthExchange; import org.apache.hc.client5.http.auth.AuthScope; import org.apache.hc.client5.http.auth.ChallengeType; -import org.apache.hc.client5.http.auth.Credentials; -import org.apache.hc.client5.http.auth.CredentialsProvider; import org.apache.hc.client5.http.auth.StandardAuthScheme; import org.apache.hc.client5.http.classic.ExecChain; import org.apache.hc.client5.http.classic.ExecRuntime; import org.apache.hc.client5.http.classic.methods.HttpGet; import org.apache.hc.client5.http.classic.methods.HttpPost; import org.apache.hc.client5.http.entity.EntityBuilder; -import org.apache.hc.client5.http.impl.auth.BasicCredentialsProvider; import org.apache.hc.client5.http.impl.auth.BasicScheme; import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder; import org.apache.hc.client5.http.impl.auth.NTLMScheme; @@ -57,6 +53,7 @@ import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpResponse; +import org.apache.hc.core5.http.ProtocolException; import org.apache.hc.core5.http.message.BasicClassicHttpResponse; import org.apache.hc.core5.http.protocol.HttpProcessor; import org.junit.Assert; @@ -123,20 +120,9 @@ public class TestProtocolExec { final HttpRoute route = new HttpRoute(new HttpHost("somehost", 8080)); final ClassicHttpRequest request = new HttpGet("http://somefella:secret@bar/test"); final HttpClientContext context = HttpClientContext.create(); - context.setCredentialsProvider(new BasicCredentialsProvider()); - - final ClassicHttpResponse response = Mockito.mock(ClassicHttpResponse.class); - Mockito.when(chain.proceed( - Mockito.any(), - Mockito.any())).thenReturn(response); final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); - protocolExec.execute(request, scope, chain); - Assert.assertEquals(new URI("http://bar/test"), request.getUri()); - final CredentialsProvider credentialsProvider = context.getCredentialsProvider(); - final Credentials creds = credentialsProvider.getCredentials(new AuthScope(null, "bar", -1, null, null), null); - Assert.assertNotNull(creds); - Assert.assertEquals("somefella", creds.getUserPrincipal().getName()); + Assert.assertThrows(ProtocolException.class, () -> protocolExec.execute(request, scope, chain)); } @Test