This is an automated email from the ASF dual-hosted git repository.
tabish pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/qpid-protonj2.git
The following commit(s) were added to refs/heads/main by this push:
new 6b407996 PROTON-2781 Cleanups in the SASL layer API docs and some code
tidying
6b407996 is described below
commit 6b40799641eba9506f66df61fcbdca26ca2408f5
Author: Timothy Bish <[email protected]>
AuthorDate: Fri Dec 8 16:48:42 2023 -0500
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.
---
.../org/apache/qpid/protonj2/engine/Engine.java | 3 +-
.../qpid/protonj2/engine/EngineSaslDriver.java | 13 +++++---
.../qpid/protonj2/engine/sasl/SaslContext.java | 5 +++
.../protonj2/engine/sasl/SaslSystemException.java | 13 ++++----
.../sasl/client/AbstractScramSHAMechanism.java | 9 +++---
.../sasl/client/SaslCredentialsProvider.java | 4 +++
.../engine/sasl/client/SaslMechanisms.java | 2 +-
.../client/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 88a44279..f4bda43c 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 @@ public interface Engine extends Consumer<ProtonBuffer> {
* 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 57737b83..b0011c94 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 @@ import
org.apache.qpid.protonj2.engine.sasl.SaslServerContext;
*/
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 interface EngineSaslDriver {
/**
* Set the maximum frame size the remote can send before an error is
indicated.
+ * <p>
+ * 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 347d9f2e..ec0a47bc 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 @@ public interface SaslContext {
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 df747a75..29d4aa31 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 @@ import javax.security.sasl.SaslException;
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.
- *
+ * <code>true</code> if the error is permanent and
requires (manual) intervention
*/
public SaslSystemException(boolean permanent) {
this(permanent, null);
@@ -55,10 +54,12 @@ public class SaslSystemException extends SaslException {
}
/**
- * 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.
+ * <p>
+ * 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 <code>true</code> 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 022bd63b..61cc7777 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.nio.charset.StandardCharsets;
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 @@ abstract class AbstractScramSHAMechanism extends
AbstractMechanism {
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 @@ abstract class AbstractScramSHAMechanism extends
AbstractMechanism {
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 ab9ce8aa..c3701c08 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 97dce38e..debc3270 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 enum SaslMechanisms {
}
}
- 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 32b1f833..0af9b2be 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 abstract class AbstractScramSHAMechanismTestBase
extends MechanismTestBas
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 abstract class AbstractScramSHAMechanismTestBase
extends MechanismTestBas
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 abstract class AbstractScramSHAMechanismTestBase
extends MechanismTestBas
"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 abstract class AbstractScramSHAMechanismTestBase
extends MechanismTestBas
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 abstract class AbstractScramSHAMechanismTestBase
extends MechanismTestBas
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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
