Repository: kudu Updated Branches: refs/heads/master 9b6034b4e -> cdc73b900
java: improve error messages when tokens are not used We recently had some trouble debugging Java client connection failures where we expected it to authenticate by token but it did not. Previously it just complained that it couldn't authenticate by SASL and didn't give any clue as to why it didn't use tokens. This patch makes the error more clear by explaining why a token was not used to authenticate. Change-Id: I7f5160a79e973c411c16a784dc105444f5dd2a6f Reviewed-on: http://gerrit.cloudera.org:8080/9070 Reviewed-by: Alexey Serbin <[email protected]> Tested-by: Kudu Jenkins Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/cdc73b90 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/cdc73b90 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/cdc73b90 Branch: refs/heads/master Commit: cdc73b900608000ff2d2f8bc74c05893338454ba Parents: 9b6034b Author: Todd Lipcon <[email protected]> Authored: Thu Jan 18 15:34:48 2018 -0800 Committer: Todd Lipcon <[email protected]> Committed: Mon Mar 5 07:33:19 2018 +0000 ---------------------------------------------------------------------- .../java/org/apache/kudu/client/Negotiator.java | 45 +++++++++++++++++--- .../org/apache/kudu/client/SecurityContext.java | 4 +- .../org/apache/kudu/client/TestSecurity.java | 45 ++++++++++++++++++-- 3 files changed, 83 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/cdc73b90/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java ---------------------------------------------------------------------- diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java b/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java index c58ffe6..fd609d3 100644 --- a/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java +++ b/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java @@ -155,6 +155,19 @@ public class Negotiator extends SimpleChannelUpstreamHandler { */ private final SignedTokenPB authnToken; + private static enum AuthnTokenNotUsedReason { + NONE_AVAILABLE("no token is available"), + NO_TRUSTED_CERTS("no TLS certificates are trusted by the client"), + FORBIDDEN_BY_POLICY("this connection does not allow authentication by tokens"), + NOT_CHOSEN_BY_SERVER("the server chose not to accept token authentication"); + + AuthnTokenNotUsedReason(String msg) { + this.msg = msg; + } + final String msg; + }; + private AuthnTokenNotUsedReason authnTokenNotUsedReason = null; + private State state = State.INITIAL; private SaslClient saslClient; @@ -198,10 +211,20 @@ public class Negotiator extends SimpleChannelUpstreamHandler { boolean ignoreAuthnToken) { this.remoteHostname = remoteHostname; this.securityContext = securityContext; - if (ignoreAuthnToken) { - this.authnToken = null; + SignedTokenPB token = securityContext.getAuthenticationToken(); + if (token != null) { + if (ignoreAuthnToken) { + this.authnToken = null; + this.authnTokenNotUsedReason = AuthnTokenNotUsedReason.FORBIDDEN_BY_POLICY; + } else if (!securityContext.hasTrustedCerts()) { + this.authnToken = null; + this.authnTokenNotUsedReason = AuthnTokenNotUsedReason.NO_TRUSTED_CERTS; + } else { + this.authnToken = token; + } } else { - this.authnToken = securityContext.getAuthenticationToken(); + this.authnToken = null; + this.authnTokenNotUsedReason = AuthnTokenNotUsedReason.NONE_AVAILABLE; } } @@ -230,7 +253,7 @@ public class Negotiator extends SimpleChannelUpstreamHandler { // We may also have a token. But, we can only use the token // if we are able to use authenticated TLS to authenticate the server. - if (authnToken != null && securityContext.hasTrustedCerts()) { + if (authnToken != null) { builder.addAuthnTypesBuilder().setToken( AuthenticationTypePB.Token.getDefaultInstance()); } @@ -396,7 +419,7 @@ public class Negotiator extends SimpleChannelUpstreamHandler { if (clientMech.equals(SaslMechanism.GSSAPI) && (securityContext.getSubject() == null || securityContext.getSubject().getPrivateCredentials(KerberosTicket.class).isEmpty())) { - errorsByMech.put(clientMech.name(), "Client does not have Kerberos credentials (tgt)"); + errorsByMech.put(clientMech.name(), "client does not have Kerberos credentials (tgt)"); continue; } @@ -437,13 +460,18 @@ public class Negotiator extends SimpleChannelUpstreamHandler { if (serverMechs.size() == 1 && serverMechs.contains(SaslMechanism.GSSAPI)) { // Give a better error diagnostic for common case of an unauthenticated client connecting // to a secure server. - message = "server requires authentication, " + - "but client does not have Kerberos credentials available"; + message = "server requires authentication, but " + + errorsByMech.get(SaslMechanism.GSSAPI.name()); } else { message = "client/server supported SASL mechanism mismatch: [" + Joiner.on(", ").withKeyValueSeparator(": ").join(errorsByMech) + "]"; } + if (authnTokenNotUsedReason != null) { + message += ". Authentication tokens were not used because " + + authnTokenNotUsedReason.msg; + } + // If client has valid secondary authn credentials (such as authn token), // but it does not have primary authn credentials (such as Kerberos creds), // throw a recoverable exception. So that the request can be retried as long @@ -469,6 +497,9 @@ public class Negotiator extends SimpleChannelUpstreamHandler { AuthenticationTypePB.TypeCase type = response.getAuthnTypes(0).getTypeCase(); switch (type) { case SASL: + if (authnToken != null) { + authnTokenNotUsedReason = AuthnTokenNotUsedReason.NOT_CHOSEN_BY_SERVER; + } break; case TOKEN: if (authnToken == null) { http://git-wip-us.apache.org/repos/asf/kudu/blob/cdc73b90/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java ---------------------------------------------------------------------- diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java b/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java index 0340bfd..e9bf0c7 100644 --- a/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java +++ b/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java @@ -33,6 +33,7 @@ import javax.net.ssl.TrustManagerFactory; import javax.net.ssl.X509TrustManager; import javax.security.auth.Subject; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; @@ -75,7 +76,8 @@ class SecurityContext { /** * The currently trusted CA certs, in DER format. */ - private List<ByteString> trustedCertDers = Collections.emptyList(); + @VisibleForTesting + List<ByteString> trustedCertDers = Collections.emptyList(); /** * Construct SecurityContext object with the specified JAAS subject. http://git-wip-us.apache.org/repos/asf/kudu/blob/cdc73b90/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java ---------------------------------------------------------------------- diff --git a/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java b/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java index 290526b..793e1f3 100644 --- a/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java +++ b/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java @@ -20,14 +20,16 @@ import java.util.List; import com.stumbleupon.async.Deferred; +import org.apache.kudu.client.Client.AuthenticationCredentialsPB; import org.apache.kudu.master.Master.ConnectToMasterResponsePB; import org.apache.kudu.util.AssertHelpers.BooleanExpression; import org.apache.kudu.util.SecurityUtil; import org.junit.Assert; import org.junit.BeforeClass; -import org.junit.Ignore; import org.junit.Test; +import org.hamcrest.CoreMatchers; + public class TestSecurity extends BaseKuduTest { private static final String TABLE_NAME = "TestSecurity-table"; @@ -61,8 +63,10 @@ public class TestSecurity extends BaseKuduTest { Assert.fail("should not have been able to connect to a secure cluster " + "with no credentials"); } catch (NonRecoverableException e) { - Assert.assertTrue(e.getMessage().contains("server requires authentication, " + - "but client does not have Kerberos credentials available")); + Assert.assertThat(e.getMessage(), CoreMatchers.containsString( + "server requires authentication, but client does not have " + + "Kerberos credentials (tgt). Authentication tokens were not used " + + "because no token is available")); } // If we import the authentication data from the old authenticated client, @@ -80,6 +84,41 @@ public class TestSecurity extends BaseKuduTest { } /** + * Test that if, for some reason, the client has a token but no CA certs, it + * will emit an appropriate error message in the exception. + */ + @Test + public void testErrorMessageWithNoCaCert() throws Exception { + byte[] authnData = client.exportAuthenticationCredentials().join(); + + // Remove the CA certs from the credentials. + authnData = AuthenticationCredentialsPB.parseFrom(authnData).toBuilder() + .clearCaCertDers().build().toByteArray(); + + String oldTicketCache = System.getProperty(SecurityUtil.KUDU_TICKETCACHE_PROPERTY); + System.clearProperty(SecurityUtil.KUDU_TICKETCACHE_PROPERTY); + try { + KuduClient newClient = new KuduClient.KuduClientBuilder(masterAddresses).build(); + newClient.importAuthenticationCredentials(authnData); + + // We shouldn't be able to connect because we have no appropriate CA cert. + try { + newClient.listTabletServers(); + Assert.fail("should not have been able to connect to a secure cluster " + + "with no credentials"); + } catch (NonRecoverableException e) { + Assert.assertThat(e.getMessage(), CoreMatchers.containsString( + "server requires authentication, but client does not have " + + "Kerberos credentials (tgt). Authentication tokens were not used " + + "because no TLS certificates are trusted by the client")); + } + } finally { + // Restore ticket cache for other test cases. + System.setProperty(SecurityUtil.KUDU_TICKETCACHE_PROPERTY, oldTicketCache); + } + } + + /** * Regression test for KUDU-2267 and KUDU-2319. * * A client with valid token but without valid Kerberos credentials
