From 6b40799641eba9506f66df61fcbdca26ca2408f5 Mon Sep 17 00:00:00 2001 From: Timothy Bish Date: Fri, 8 Dec 2023 16:48:42 -0500 Subject: [PATCH] PROTON-2781 Cleanups in the SASL layer API docs and some code tidying Add some details in API docs and improve some error messages and code. --- .../apache/qpid/protonj2/engine/Engine.java | 3 +- .../protonj2/engine/EngineSaslDriver.java | 13 +++++-- .../protonj2/engine/sasl/SaslContext.java | 5 +++ .../engine/sasl/SaslSystemException.java | 13 ++++--- .../client/AbstractScramSHAMechanism.java | 9 ++--- .../sasl/client/SaslCredentialsProvider.java | 4 ++ .../engine/sasl/client/SaslMechanisms.java | 2 +- .../AbstractScramSHAMechanismTestBase.java | 37 +++---------------- 8 files changed, 38 insertions(+), 48 deletions(-) diff --git a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/Engine.java b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/Engine.java index 88a442798..f4bda43ca 100644 --- a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/Engine.java +++ b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/Engine.java @@ -232,7 +232,8 @@ default void accept(ProtonBuffer input) throws EngineStateException { * default no-op driver must be returned that indicates this. The SASL driver provides * the engine with client and server side SASL handshaking support. An {@link Engine} * implementation can support pluggable SASL drivers or exert tight control over the - * driver as it sees fit. + * driver as it sees fit. The engine implementation in use will dictate how a SASL + * driver is assigned or discovered. * * @return the SASL driver for the engine. */ diff --git a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/EngineSaslDriver.java b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/EngineSaslDriver.java index 57737b835..b0011c94b 100644 --- a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/EngineSaslDriver.java +++ b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/EngineSaslDriver.java @@ -30,10 +30,10 @@ */ public interface EngineSaslDriver { - /** - * The SASL driver state used to determine at what point the current SASL negotiation process - * is currently in. If the state is 'none' then no SASL negotiations will be performed. - */ + /** + * The SASL driver state used to determine at what point the current SASL negotiation process + * is currently in. If the state is 'none' then no SASL negotiations will be performed. + */ public enum SaslState { /** @@ -116,6 +116,11 @@ public enum SaslState { /** * Set the maximum frame size the remote can send before an error is indicated. + *

+ * The AMQP specification defines a default of 512 bytes for this value however in + * some cases this value is to small for the data needed in some SASL mechanisms. + * This allows the user to configure a larger acceptable value, the lowest possible + * value remains the default 512 bytes. * * @param maxFrameSize * The maximum allowed frame size from the remote sender. diff --git a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslContext.java b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslContext.java index 347d9f2ee..ec0a47bc6 100644 --- a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslContext.java +++ b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslContext.java @@ -76,6 +76,11 @@ enum Role { CLIENT, SERVER } Role getRole(); /** + * Returns if the SASL context has been marked as completed. A completed context + * does not imply the SASL authentication was successful, the caller should check + * the state of the {@link #getSaslOutcome()} value to determine if the SASL + * authentication process was successful or not. + * * @return true if SASL authentication has completed */ boolean isDone(); diff --git a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslSystemException.java b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslSystemException.java index df747a759..29d4aa317 100644 --- a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslSystemException.java +++ b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslSystemException.java @@ -27,15 +27,14 @@ public class SaslSystemException extends SaslException { private static final long serialVersionUID = 1L; + private final boolean permanent; /** * Creates an exception indicating a system error. * * @param permanent - * {@code true} if the error is permanent and requires (manual) - * intervention. - * + * true if the error is permanent and requires (manual) intervention */ public SaslSystemException(boolean permanent) { this(permanent, null); @@ -55,10 +54,12 @@ public SaslSystemException(boolean permanent, String detail) { } /** - * Checks if the condition that caused this exception is of a permanent - * nature. + * Checks if the condition that caused this exception is of a permanent nature. + *

+ * A permanent error should be treated as terminal for transports that implement + * reconnection logic and the implementation should stop attempting connections. * - * @return {@code true} if the error condition is permanent. + * @return true if the error condition is permanent. */ public final boolean isPermanent() { return permanent; diff --git a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanism.java b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanism.java index 022bd63be..61cc7777d 100644 --- a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanism.java +++ b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanism.java @@ -20,7 +20,6 @@ import java.security.InvalidKeyException; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; -import java.util.Arrays; import java.util.Base64; import javax.crypto.Mac; @@ -112,7 +111,7 @@ public void verifyCompletion() throws SaslException { if (state != State.COMPLETE) { throw new SaslException(String.format( - "SASL exchange was not fully completed. Expected state %s but actual state %s", State.COMPLETE, state)); + "SASL exchange was not fully completed. Expected state %s but actual state %s", State.COMPLETE, state)); } } @@ -186,13 +185,13 @@ private void evaluateOutcome(ProtonBuffer challenge) throws SaslException { String[] parts = serverFinalMessage.split(","); if (!parts[0].startsWith("v=")) { - throw new SaslException("Server final message did not contain verifier"); + throw new SaslException("Server final message did not contain expected verifier"); } byte[] serverSignature = Base64.getDecoder().decode(parts[0].substring(2)); - if (!Arrays.equals(this.serverSignature, serverSignature)) { - throw new SaslException("Server signature did not match"); + if (!MessageDigest.isEqual(this.serverSignature, serverSignature)) { + throw new SaslException("Server signature did not match expected signature."); } } diff --git a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslCredentialsProvider.java b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslCredentialsProvider.java index ab9ce8aa7..c3701c08d 100644 --- a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslCredentialsProvider.java +++ b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslCredentialsProvider.java @@ -42,6 +42,10 @@ public interface SaslCredentialsProvider { String password(); /** + * Returns the local principal for use in SASL authentication which is generally + * provided by the IO layer (TLS). This method can return null if there is no + * local {@link Principal} in use. + * * @return the local principal value to use when performing SASL authentication. */ Principal localPrincipal(); diff --git a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslMechanisms.java b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslMechanisms.java index 97dce38e9..debc32701 100644 --- a/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslMechanisms.java +++ b/protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslMechanisms.java @@ -155,7 +155,7 @@ public static SaslMechanisms valueOf(Symbol mechanism) { } } - throw new IllegalArgumentException("No Matching SASL Mechanism with name: " + mechanism.toString()); + throw new IllegalArgumentException("No Matching SASL Mechanism with name: " + mechanism); } /** diff --git a/protonj2/src/test/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanismTestBase.java b/protonj2/src/test/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanismTestBase.java index 32b1f833e..0af9b2be1 100644 --- a/protonj2/src/test/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanismTestBase.java +++ b/protonj2/src/test/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanismTestBase.java @@ -17,7 +17,7 @@ package org.apache.qpid.protonj2.engine.sasl.client; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.assertThrows; import javax.security.sasl.SaslException; @@ -75,12 +75,7 @@ public void testServerFirstMessageMalformed() throws Exception { ProtonBuffer challenge = ProtonBufferAllocator.defaultAllocator().copy("badserverfirst".getBytes()); challenge.setWriteOffset(challenge.capacity()); - try { - mechanism.getChallengeResponse(getTestCredentials(), challenge); - fail("Exception not thrown"); - } catch (SaslException s) { - // PASS - } + assertThrows(SaslException.class, () -> mechanism.getChallengeResponse(getTestCredentials(), challenge)); } /** @@ -101,12 +96,7 @@ public void testServerFirstMessageMandatoryExtensionRejected() throws Exception ProtonBuffer challenge = ProtonBufferAllocator.defaultAllocator().copy("m=notsupported,s=,i=".getBytes()); challenge.setWriteOffset(challenge.capacity()); - try { - mechanism.getChallengeResponse(getTestCredentials(), challenge); - fail("Exception not thrown"); - } catch (SaslException s) { - // PASS - } + assertThrows(SaslException.class, () -> mechanism.getChallengeResponse(getTestCredentials(), challenge)); } /** @@ -127,12 +117,7 @@ public void testServerFirstMessageInvalidNonceRejected() throws Exception { "r=invalidnonce,s=W22ZaJ0SNY7soEsUEjb6gQ==,i=4096".getBytes()); challenge.setWriteOffset(challenge.capacity()); - try { - mechanism.getChallengeResponse(getTestCredentials(), challenge); - fail("Exception not thrown"); - } catch (SaslException s) { - // PASS - } + assertThrows(SaslException.class, () -> mechanism.getChallengeResponse(getTestCredentials(), challenge)); } /** @@ -155,12 +140,7 @@ public void testServerSignatureDiffer() throws Exception { ProtonBuffer challenge = ProtonBufferAllocator.defaultAllocator().copy("v=badserverfinal".getBytes()); challenge.setWriteOffset(challenge.capacity()); - try { - mechanism.getChallengeResponse(getTestCredentials(), challenge); - fail("Exception not thrown"); - } catch (SaslException e) { - // PASS - } + assertThrows(SaslException.class, () -> mechanism.getChallengeResponse(getTestCredentials(), challenge)); } @Test @@ -173,11 +153,6 @@ public void testIncompleteExchange() throws Exception { ProtonBuffer clientFinalMessage = mechanism.getChallengeResponse(getTestCredentials(), serverFirstMessage); assertEquals(expectedClientFinalMessage, clientFinalMessage); - try { - mechanism.verifyCompletion(); - fail("Exception not thrown"); - } catch (SaslException e) { - // PASS - } + assertThrows(SaslException.class, () -> mechanism.verifyCompletion()); } } \ No newline at end of file