From 005ae95beab5e11a72b876c8d8e41e205091947d Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 21 Dec 2017 02:34:39 -0800 Subject: [PATCH] Fixes #1949 - Client-side problems with digest authentication. (#1991) Introduced ContentProvider.isReproducible() to detect whether the request content can be provided more than once, and modified ContentProvider implementation accordingly. Modified AuthenticationProtocolHandler to not send an authenticated request if the content is not reproducible. Modified AuthenticationProtocolHandler to tolerate request failures. Signed-off-by: Simone Bordet --- .../client/AuthenticationProtocolHandler.java | 18 +- .../jetty/client/api/ContentProvider.java | 17 ++ .../util/ByteBufferContentProvider.java | 12 +- .../client/util/BytesContentProvider.java | 12 +- .../client/util/PathContentProvider.java | 6 + .../client/AbstractHttpClientServerTest.java | 2 + .../client/HttpClientAuthenticationTest.java | 166 +++++++++++++++--- 7 files changed, 193 insertions(+), 40 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java index 3fe68cc340d..5531f289740 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java @@ -27,6 +27,7 @@ import java.util.regex.Pattern; import org.eclipse.jetty.client.api.Authentication; import org.eclipse.jetty.client.api.Connection; +import org.eclipse.jetty.client.api.ContentProvider; import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; @@ -86,7 +87,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler { HttpRequest request = (HttpRequest)result.getRequest(); ContentResponse response = new HttpContentResponse(result.getResponse(), getContent(), getMediaType(), getEncoding()); - if (result.isFailed()) + if (result.getResponseFailure() != null) { if (LOG.isDebugEnabled()) LOG.debug("Authentication challenge failed {}", result.getFailure()); @@ -98,7 +99,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler HttpConversation conversation = request.getConversation(); if (conversation.getAttribute(authenticationAttribute) != null) { - // We have already tried to authenticate, but we failed again + // We have already tried to authenticate, but we failed again. if (LOG.isDebugEnabled()) LOG.debug("Bad credentials for {}", request); forwardSuccessComplete(request, response); @@ -111,7 +112,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler { if (LOG.isDebugEnabled()) LOG.debug("Authentication challenge without {} header", header); - forwardFailureComplete(request, null, response, new HttpResponseException("HTTP protocol violation: Authentication challenge without " + header + " header", response)); + forwardFailureComplete(request, result.getRequestFailure(), response, new HttpResponseException("HTTP protocol violation: Authentication challenge without " + header + " header", response)); return; } @@ -138,9 +139,18 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler return; } + ContentProvider requestContent = request.getContent(); + if (requestContent != null && !requestContent.isReproducible()) + { + if (LOG.isDebugEnabled()) + LOG.debug("Request content not reproducible for {}", request); + forwardSuccessComplete(request, response); + return; + } + try { - final Authentication.Result authnResult = authentication.authenticate(request, response, headerInfo, conversation); + Authentication.Result authnResult = authentication.authenticate(request, response, headerInfo, conversation); if (LOG.isDebugEnabled()) LOG.debug("Authentication result {}", authnResult); if (authnResult == null) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/api/ContentProvider.java b/jetty-client/src/main/java/org/eclipse/jetty/client/api/ContentProvider.java index 03754f72c9b..6419b11e8f4 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/api/ContentProvider.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/api/ContentProvider.java @@ -22,6 +22,7 @@ import java.io.Closeable; import java.nio.ByteBuffer; import java.util.Iterator; +import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.util.ByteBufferContentProvider; import org.eclipse.jetty.client.util.PathContentProvider; @@ -48,6 +49,22 @@ public interface ContentProvider extends Iterable */ long getLength(); + /** + *

Whether this ContentProvider can produce exactly the same content more + * than once.

+ *

Implementations should return {@code true} only if the content can be + * produced more than once, which means that invocations to {@link #iterator()} + * must return a new, independent, iterator instance over the content.

+ *

The {@link HttpClient} implementation may use this method in particular + * cases where it detects that it is safe to retry a request that failed.

+ * + * @return whether the content can be produced more than once + */ + default boolean isReproducible() + { + return false; + } + /** * An extension of {@link ContentProvider} that provides a content type string * to be used as a {@code Content-Type} HTTP header in requests. diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/util/ByteBufferContentProvider.java b/jetty-client/src/main/java/org/eclipse/jetty/client/util/ByteBufferContentProvider.java index 68386758da9..2bc5233f8be 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/util/ByteBufferContentProvider.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/util/ByteBufferContentProvider.java @@ -57,6 +57,12 @@ public class ByteBufferContentProvider extends AbstractTypedContentProvider return length; } + @Override + public boolean isReproducible() + { + return true; + } + @Override public Iterator iterator() { @@ -85,12 +91,6 @@ public class ByteBufferContentProvider extends AbstractTypedContentProvider throw new NoSuchElementException(); } } - - @Override - public void remove() - { - throw new UnsupportedOperationException(); - } }; } } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/util/BytesContentProvider.java b/jetty-client/src/main/java/org/eclipse/jetty/client/util/BytesContentProvider.java index d2c080aa700..39f5fd5ad51 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/util/BytesContentProvider.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/util/BytesContentProvider.java @@ -53,6 +53,12 @@ public class BytesContentProvider extends AbstractTypedContentProvider return length; } + @Override + public boolean isReproducible() + { + return true; + } + @Override public Iterator iterator() { @@ -78,12 +84,6 @@ public class BytesContentProvider extends AbstractTypedContentProvider throw new NoSuchElementException(); } } - - @Override - public void remove() - { - throw new UnsupportedOperationException(); - } }; } } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/util/PathContentProvider.java b/jetty-client/src/main/java/org/eclipse/jetty/client/util/PathContentProvider.java index d1051d94136..00147a556df 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/util/PathContentProvider.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/util/PathContentProvider.java @@ -85,6 +85,12 @@ public class PathContentProvider extends AbstractTypedContentProvider return fileSize; } + @Override + public boolean isReproducible() + { + return true; + } + public ByteBufferPool getByteBufferPool() { return bufferPool; diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/AbstractHttpClientServerTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/AbstractHttpClientServerTest.java index b591cd7481f..4eea728f923 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/AbstractHttpClientServerTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/AbstractHttpClientServerTest.java @@ -27,6 +27,7 @@ import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.toolchain.test.TestTracker; +import org.eclipse.jetty.util.SocketAddressResolver; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.After; @@ -96,6 +97,7 @@ public abstract class AbstractHttpClientServerTest clientThreads.setName("client"); client = new HttpClient(transport, sslContextFactory); client.setExecutor(clientThreads); + client.setSocketAddressResolver(new SocketAddressResolver.Sync()); client.start(); } diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java index 6e1a5ca074d..14d23267228 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java @@ -22,11 +22,14 @@ import java.io.File; import java.io.IOException; import java.net.URI; import java.nio.ByteBuffer; -import java.nio.charset.StandardCharsets; +import java.util.Iterator; +import java.util.NoSuchElementException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.IntFunction; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -34,6 +37,7 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.client.api.Authentication; import org.eclipse.jetty.client.api.AuthenticationStore; +import org.eclipse.jetty.client.api.ContentProvider; import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; @@ -41,6 +45,7 @@ import org.eclipse.jetty.client.api.Result; import org.eclipse.jetty.client.util.BasicAuthentication; import org.eclipse.jetty.client.util.DeferredContentProvider; import org.eclipse.jetty.client.util.DigestAuthentication; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.security.Authenticator; import org.eclipse.jetty.security.ConstraintMapping; import org.eclipse.jetty.security.ConstraintSecurityHandler; @@ -220,7 +225,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest { baseRequest.setHandled(true); if (requests.incrementAndGet() == 1) - response.sendRedirect(URIUtil.newURI(scheme,request.getServerName(),request.getServerPort(),request.getRequestURI(),null)); + response.sendRedirect(URIUtil.newURI(scheme, request.getServerName(), request.getServerPort(), request.getRequestURI(), null)); } }); @@ -259,7 +264,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest { baseRequest.setHandled(true); if (request.getRequestURI().endsWith("/redirect")) - response.sendRedirect(URIUtil.newURI(scheme,request.getServerName(),request.getServerPort(),"/secure",null)); + response.sendRedirect(URIUtil.newURI(scheme, request.getServerName(), request.getServerPort(), "/secure", null)); } }); @@ -424,6 +429,40 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest Assert.assertEquals(1, requests.get()); } + @Test + public void test_NonReproducibleContent() throws Exception + { + startBasic(new EmptyServerHandler()); + + AuthenticationStore authenticationStore = client.getAuthenticationStore(); + URI uri = URI.create(scheme + "://localhost:" + connector.getLocalPort()); + BasicAuthentication authentication = new BasicAuthentication(uri, realm, "basic", "basic"); + authenticationStore.addAuthentication(authentication); + + CountDownLatch resultLatch = new CountDownLatch(1); + byte[] data = new byte[]{'h', 'e', 'l', 'l', 'o'}; + DeferredContentProvider content = new DeferredContentProvider(ByteBuffer.wrap(data)) + { + @Override + public boolean isReproducible() + { + return false; + } + }; + Request request = client.newRequest(uri) + .path("/secure") + .content(content); + request.send(result -> + { + if (result.isSucceeded() && result.getResponse().getStatus() == HttpStatus.UNAUTHORIZED_401) + resultLatch.countDown(); + }); + + content.close(); + + Assert.assertTrue(resultLatch.await(5, TimeUnit.SECONDS)); + } + @Test public void test_RequestFailsAfterResponse() throws Exception { @@ -434,32 +473,111 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest BasicAuthentication authentication = new BasicAuthentication(uri, realm, "basic", "basic"); authenticationStore.addAuthentication(authentication); - CountDownLatch successLatch = new CountDownLatch(1); + AtomicBoolean fail = new AtomicBoolean(true); + GeneratingContentProvider content = new GeneratingContentProvider(index -> + { + switch (index) + { + case 0: + return ByteBuffer.wrap(new byte[]{'h', 'e', 'l', 'l', 'o'}); + case 1: + return ByteBuffer.wrap(new byte[]{'w', 'o', 'r', 'l', 'd'}); + case 2: + if (fail.compareAndSet(true, false)) + { + // Wait for the 401 response to arrive + // to the authentication protocol handler. + sleep(1000); + // Trigger request failure. + throw new RuntimeException(); + } + else + { + return null; + } + default: + throw new IllegalStateException(); + } + }); CountDownLatch resultLatch = new CountDownLatch(1); - DeferredContentProvider content = new DeferredContentProvider(); - Request request = client.newRequest("localhost", connector.getLocalPort()) + client.newRequest("localhost", connector.getLocalPort()) .scheme(scheme) .path("/secure") .content(content) - .onResponseSuccess(response -> successLatch.countDown()); - request.send(result -> - { - if (result.isFailed() && result.getResponseFailure() == null) - resultLatch.countDown(); - }); + .send(result -> + { + if (result.isSucceeded() && result.getResponse().getStatus() == HttpStatus.OK_200) + resultLatch.countDown(); + }); - // Send some content to make sure the request is dispatched on the server. - content.offer(ByteBuffer.wrap("hello".getBytes(StandardCharsets.UTF_8))); - - // Wait for the response to arrive to - // the authentication protocol handler. - Thread.sleep(1000); - - // Trigger request failure. - request.abort(new Exception()); - - // Verify that the response was successful, it's the request that failed. - Assert.assertTrue(successLatch.await(5, TimeUnit.SECONDS)); Assert.assertTrue(resultLatch.await(5, TimeUnit.SECONDS)); } + + private void sleep(long time) + { + try + { + Thread.sleep(time); + } + catch (InterruptedException x) + { + throw new RuntimeException(x); + } + } + + private static class GeneratingContentProvider implements ContentProvider + { + private static final ByteBuffer DONE = ByteBuffer.allocate(0); + + private final IntFunction generator; + + private GeneratingContentProvider(IntFunction generator) + { + this.generator = generator; + } + + @Override + public long getLength() + { + return -1; + } + + @Override + public boolean isReproducible() + { + return true; + } + + @Override + public Iterator iterator() + { + return new Iterator() + { + private int index; + public ByteBuffer current; + + @Override + public boolean hasNext() + { + if (current == null) + { + current = generator.apply(index++); + if (current == null) + current = DONE; + } + return current != DONE; + } + + @Override + public ByteBuffer next() + { + ByteBuffer result = current; + current = null; + if (result == null) + throw new NoSuchElementException(); + return result; + } + }; + } + } }