From 858dd10284ccfa549f7cdafccc3bad0a797bf4ca Mon Sep 17 00:00:00 2001 From: gkc Date: Sat, 11 Jul 2026 14:09:30 +0100 Subject: [PATCH 1/2] feat(auth): send from: on connect and reuse its challenge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The executor now issues from:@atSign as its first command once the connection is ready, so the connection's atSign is established up front and the server's challenge is retained. CRAM/PKAM authentication reuses that challenge instead of sending a second from:, saving a round trip. The challenge is single-use — getFromChallenge() consumes it atomically, so a second authentication on the same connection (onboarding does CRAM then PKAM) gets null and falls back to issuing its own from:. It is also cleared on disconnect, since a from: challenge is only valid for the server session that issued it. Adds NettyAtCommandExecutorTest coverage for the challenge lifecycle: retained and consumed once, absent with no atSign, cleared on disconnect, and refreshed on reconnect. --- .../atsign/client/api/AtCommandExecutor.java | 15 ++++ .../client/impl/AtCommandExecutors.java | 2 + .../atsign/client/impl/cli/AbstractCli.java | 1 + .../impl/commands/AuthenticationCommands.java | 20 +++-- .../impl/netty/NettyAtCommandExecutor.java | 61 +++++++++++++++ .../netty/NettyAtCommandExecutorTest.java | 78 +++++++++++++++++++ 6 files changed, 171 insertions(+), 6 deletions(-) diff --git a/at_client/src/main/java/org/atsign/client/api/AtCommandExecutor.java b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutor.java index 5c66fb5e..60e44197 100644 --- a/at_client/src/main/java/org/atsign/client/api/AtCommandExecutor.java +++ b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutor.java @@ -83,4 +83,19 @@ default CompletableFuture send(String command, Consumer consumer) * @return this (to allow chaining / fluent style invocation) */ AtCommandExecutor onReady(Consumer consumer); + + /** + * Returns the challenge from the {@code from:} command that this executor issued as its first + * command once the connection became ready, so that CRAM / PKAM authentication can reuse it + * rather than sending a second {@code from:}. The challenge is consumed at most once — the + * server's {@code from:} challenge is single-use, so a second authentication on the same + * connection (e.g. onboarding, which does CRAM then PKAM) gets {@code null} and falls back to + * sending its own {@code from:}. Also returns {@code null} when this executor was not configured + * with an atSign (and therefore did not send an initial {@code from:}). + * + * @return the retained {@code from:} challenge, or {@code null} if none is available + */ + default String getFromChallenge() { + return null; + } } diff --git a/at_client/src/main/java/org/atsign/client/impl/AtCommandExecutors.java b/at_client/src/main/java/org/atsign/client/impl/AtCommandExecutors.java index 4b7d1f49..915e1fb7 100644 --- a/at_client/src/main/java/org/atsign/client/impl/AtCommandExecutors.java +++ b/at_client/src/main/java/org/atsign/client/impl/AtCommandExecutors.java @@ -76,6 +76,8 @@ public static AtCommandExecutor createCommandExecutor(String url, .awaitReadyMillis(defaultIfNotSet(awaitReadyMillis, DEFAULT_TIMEOUT_MILLIS)) .reconnect(defaultIfNotSet(reconnect, SimpleReconnectStrategy.builder().build())) .queueLimit(queueLimit) + .atSign(atSign) + .clientConfig(createClientConfig(config)) .onReady(defaultIfNotSet(onReady, createOnReady(atSign, keys, createClientConfig(config)))) .build(); } diff --git a/at_client/src/main/java/org/atsign/client/impl/cli/AbstractCli.java b/at_client/src/main/java/org/atsign/client/impl/cli/AbstractCli.java index 41ffe904..f02a5783 100644 --- a/at_client/src/main/java/org/atsign/client/impl/cli/AbstractCli.java +++ b/at_client/src/main/java/org/atsign/client/impl/cli/AbstractCli.java @@ -106,6 +106,7 @@ private static NettyAtCommandExecutorBuilder createCommandExecutorBuilder(String .build(); return NettyAtCommandExecutor.builder() .endpoint(endpoint) + .atSign(atSign) .reconnect(reconnect) .isVerbose(verbose); } diff --git a/at_client/src/main/java/org/atsign/client/impl/commands/AuthenticationCommands.java b/at_client/src/main/java/org/atsign/client/impl/commands/AuthenticationCommands.java index b7d3e702..e56e6b89 100644 --- a/at_client/src/main/java/org/atsign/client/impl/commands/AuthenticationCommands.java +++ b/at_client/src/main/java/org/atsign/client/impl/commands/AuthenticationCommands.java @@ -59,10 +59,14 @@ public static void authenticateWithPkam(AtCommandExecutor executor, throws AtException { try { + // reuse the challenge from the initial from: if the executor already sent one, otherwise // send a from command and expect to receive a challenge - String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).config(config).build(); - String fromResponse = executor.sendSync(fromCommand); - String challenge = matchDataStringNoWhitespace(throwExceptionIfError(fromResponse)); + String challenge = executor.getFromChallenge(); + if (challenge == null) { + String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).config(config).build(); + String fromResponse = executor.sendSync(fromCommand); + challenge = matchDataStringNoWhitespace(throwExceptionIfError(fromResponse)); + } // send a pkam command with the signed challenge String signature = EncryptionUtils.signSHA256RSA(challenge, keys.getApkamPrivateKey()); @@ -95,10 +99,14 @@ public static void authenticateWithCram(AtCommandExecutor executor, AtSign atSig throws AtException { try { + // reuse the challenge from the initial from: if the executor already sent one, otherwise // send a from command and expect to receive a challenge - String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).build(); - String fromResponse = executor.sendSync(fromCommand); - String challenge = matchDataStringNoWhitespace(throwExceptionIfError(fromResponse)); + String challenge = executor.getFromChallenge(); + if (challenge == null) { + String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).build(); + String fromResponse = executor.sendSync(fromCommand); + challenge = matchDataStringNoWhitespace(throwExceptionIfError(fromResponse)); + } // send a cram command String cramDigest = createDigest(cramSecret, challenge); diff --git a/at_client/src/main/java/org/atsign/client/impl/netty/NettyAtCommandExecutor.java b/at_client/src/main/java/org/atsign/client/impl/netty/NettyAtCommandExecutor.java index 7d12747c..8f94de0c 100644 --- a/at_client/src/main/java/org/atsign/client/impl/netty/NettyAtCommandExecutor.java +++ b/at_client/src/main/java/org/atsign/client/impl/netty/NettyAtCommandExecutor.java @@ -2,12 +2,15 @@ import static java.util.concurrent.CompletableFuture.failedFuture; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.atsign.client.impl.commands.DataResponses.matchDataStringNoWhitespace; +import static org.atsign.client.impl.commands.ErrorResponses.throwExceptionIfError; import static org.atsign.client.impl.common.CommandElement.isPrompt; import static org.atsign.client.impl.common.Preconditions.checkNotNull; import java.io.IOException; import java.time.Clock; import java.util.Collection; +import java.util.Map; import java.util.concurrent.*; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -18,7 +21,9 @@ import javax.net.ssl.SSLException; import org.atsign.client.api.AtCommandExecutor; +import org.atsign.client.api.AtSign; import org.atsign.client.impl.AtEndpointSupplier; +import org.atsign.client.impl.commands.CommandBuilders; import org.atsign.client.impl.common.ReconnectStrategy; import org.atsign.client.impl.common.CommandElement; import org.atsign.client.impl.common.CommandQueue; @@ -102,6 +107,12 @@ String toLowerCase() { private volatile long lastReadMillis; + private final AtSign atSign; + + private final Map clientConfig; + + private final AtomicReference fromChallenge = new AtomicReference<>(); + /** * Builder method for instantiating instances of a Netty based implementation of * {@link AtCommandExecutor} @@ -135,9 +146,13 @@ protected NettyAtCommandExecutor(AtEndpointSupplier endpoint, Integer queueLimit, Long awaitReadyMillis, Boolean isVerbose, + AtSign atSign, + Map clientConfig, Clock clock, Logger log) throws AtException { + this.atSign = atSign; + this.clientConfig = clientConfig; this.endpointSupplier = checkNotNull(endpoint, "endpoint is not set"); this.maxFrameLength = defaultIfUnset(maxFrameLength, DEFAULT_MAX_FRAME_LENGTH); this.reconnectStrategy = defaultIfNull(reconnect, ReconnectStrategy.NONE); @@ -466,6 +481,7 @@ private Runnable createOnReadyRunnable() { threadFactory.markCurrentThreadOnReadyThread(); try { readyingLock.lock(); + sendFromIfRequired(); onReadyConsumer.get().accept(this); } catch (AtOnReadyException e) { log.error("onReady exception", e); @@ -484,6 +500,49 @@ private Runnable createOnReadyRunnable() { }; } + /** + * Sends {@code from:@atSign} as the first command once the connection is ready, so the + * connection's atSign is established before any other verb (including a {@code scan} sent by + * an onReady consumer prior to authenticating). The challenge returned by the server is + * retained so that CRAM / PKAM authentication can reuse it instead of issuing a second + * {@code from:}. When no atSign was supplied to the builder this is a no-op and + * {@link #getFromChallenge()} returns {@code null}. + * + *

+ * Runs on the onReady thread, where {@link #sendSync(String)} is permitted. On reconnect + * the ready sequence re-runs, so the challenge is refreshed on each connect. + */ + private void sendFromIfRequired() { + if (atSign == null) { + return; + } + try { + String fromCommand = CommandBuilders.fromCommandBuilder() + .atSign(atSign) + .config(clientConfig) + .build(); + String fromResponse = sendSync(fromCommand); + String challenge = matchDataStringNoWhitespace(throwExceptionIfError(fromResponse)); + fromChallenge.set(challenge); + } catch (AtException | ExecutionException | InterruptedException | RuntimeException e) { + throw new AtOnReadyException("from command failed : " + e.getMessage(), e); + } + } + + /** + * Returns the challenge from the initial {@code from:} and clears it, so it is consumed at most + * once. The server's {@code from:} challenge is single-use — whichever authentication (CRAM or + * PKAM) sends its digest first consumes it — so a second authentication on the same connection + * (e.g. onboarding, which does CRAM then PKAM) must issue its own {@code from:} and gets + * {@code null} here to signal that fallback. The challenge is also cleared on disconnect — + * it is only valid for the server session that issued it — so a caller can never consume a + * challenge from a connection that has since dropped. + */ + @Override + public String getFromChallenge() { + return fromChallenge.getAndSet(null); + } + private void onResponse(String msg) { if (isVerbose) { log.info("RCVD: {}", msg); @@ -522,6 +581,8 @@ public void exceptionCaught(ChannelHandlerContext context, Throwable cause) { @Override public void channelInactive(ChannelHandlerContext context) { channel = null; + // a from: challenge is only valid for the server session that just ended + fromChallenge.set(null); if (status.get().isClosedOrClosing()) { log.debug("connection closed"); } else { diff --git a/at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorTest.java b/at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorTest.java index de8dfd38..2e0f7be6 100644 --- a/at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorTest.java @@ -21,6 +21,7 @@ import java.util.function.Consumer; import org.atsign.client.api.AtCommandExecutor; +import org.atsign.client.api.AtSign; import org.atsign.client.impl.AtEndpointSupplier; import org.atsign.client.impl.common.SimpleReconnectStrategy; import org.atsign.client.impl.netty.NettyAtCommandExecutor.NettyAtCommandExecutorBuilder; @@ -40,6 +41,8 @@ @Slf4j class NettyAtCommandExecutorTest { + private static final String FROM_CHALLENGE = "_a1b2c3d4@alice:12345678"; + private TestServer server; private TestEndPointSupplier endPointSupplier; private NettyAtCommandExecutorBuilder connectionBuilder; @@ -188,6 +191,71 @@ void testOnReadyAtExceptionsShouldBeFatalForTheConnection() throws Exception { assertThat(ex.getMessage(), containsString("deliberate")); } + @Test + void testFromChallengeIsRetainedAfterInitialFromAndConsumedOnce() throws Exception { + stubTestServerWithFromChallenge(server, FROM_CHALLENGE); + connectionBuilder.atSign(AtSign.createAtSign("@alice")); + try (NettyAtCommandExecutor executor = connectionBuilder.build()) { + await().until(executor::isReady); + // the executor issues from: itself as its first command once ready + assertThat(server.poll(), equalTo("from:@alice")); + // so the first authentication reuses that challenge instead of sending a second from: + assertThat(executor.getFromChallenge(), equalTo(FROM_CHALLENGE)); + // but it is single-use: a second authentication on the same connection gets null and + // falls back to issuing its own from: + assertThat(executor.getFromChallenge(), nullValue()); + } + } + + @Test + void testNoFromChallengeWhenNoAtSignConfigured() throws Exception { + try (NettyAtCommandExecutor executor = connectionBuilder.build()) { + await().until(executor::isReady); + // no atSign was supplied to the builder, so no initial from: is sent... + assertThat(server.peek(), nullValue()); + // ...and there is no retained challenge, so authentication sends its own from: + assertThat(executor.getFromChallenge(), nullValue()); + } + } + + @Test + void testFromChallengeIsClearedOnDisconnect() throws Exception { + stubTestServerWithFromChallenge(server, FROM_CHALLENGE); + connectionBuilder.atSign(AtSign.createAtSign("@alice")); + try (NettyAtCommandExecutor executor = connectionBuilder.build()) { + await().until(executor::isReady); + // the initial from: retained a challenge (proven by testFromChallengeIsRetained...) + assertThat(server.poll(), equalTo("from:@alice")); + // drop the connection without consuming the challenge + server.closeClientSocket(); + await().until(() -> !executor.isReady()); + // the challenge was only valid for the session that just ended, so it must not survive + // the disconnect - otherwise out-of-band auth could sign a challenge the server forgot + assertThat(executor.getFromChallenge(), nullValue()); + } + } + + @Test + void testFromChallengeIsRefreshedOnReconnect() throws Exception { + AtomicInteger fromCount = new AtomicInteger(); + server.setRequestHandler(request -> { + if (request != null && request.startsWith("from:")) { + server.writeAndFlush("data:challenge-" + fromCount.incrementAndGet() + "\n@"); + } else { + testServerResponse(server, request); + } + }); + connectionBuilder.atSign(AtSign.createAtSign("@alice")).reconnect(reconnectStrategy); + try (NettyAtCommandExecutor executor = connectionBuilder.build()) { + await().until(executor::isReady); + assertThat(executor.getFromChallenge(), equalTo("challenge-1")); + server.closeClientSocket(); + // the reconnect re-runs the ready sequence, which issues a fresh from: and retains its + // challenge in place of the old one + await().until(() -> "challenge-2".equals(executor.getFromChallenge())); + } + } + @Test void testSendSync() throws Exception { try (AtCommandExecutor executor = connectionBuilder.build()) { @@ -645,6 +713,16 @@ private static void stubTestServerConnectAndResponseBehaviour(TestServer server) server.setRequestHandler(s -> testServerResponse(server, s)); } + private static void stubTestServerWithFromChallenge(TestServer server, String challenge) { + server.setRequestHandler(request -> { + if (request != null && request.startsWith("from:")) { + server.writeAndFlush("data:" + challenge + "\n@"); + } else { + testServerResponse(server, request); + } + }); + } + private static void stubTestServerConnectAndAndAutomaticNotification(TestServer server) { server.setRequestHandler(s -> testServerResponseWithAutomaticNotification(server, s)); } From 9755dfaa1bcf9307566610a2eb502bcaa92d6dde Mon Sep 17 00:00:00 2001 From: gkc Date: Sun, 12 Jul 2026 18:57:27 +0100 Subject: [PATCH 2/2] refactor: attempt 1 to change impl as per @akafredperry review --- .../atsign/client/api/AtCommandExecutor.java | 19 ++-- .../client/api/AtCommandExecutorContext.java | 100 ++++++++++++++++++ .../org/atsign/client/impl/AtClientImpl.java | 2 + .../client/impl/AtCommandExecutors.java | 9 +- .../impl/commands/AuthenticationCommands.java | 43 +++++++- .../impl/netty/NettyAtCommandExecutor.java | 42 ++++---- .../impl/commands/TestExecutorBuilder.java | 4 + .../netty/NettyAtCommandExecutorTest.java | 12 +-- 8 files changed, 187 insertions(+), 44 deletions(-) create mode 100644 at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java diff --git a/at_client/src/main/java/org/atsign/client/api/AtCommandExecutor.java b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutor.java index 60e44197..5f0269eb 100644 --- a/at_client/src/main/java/org/atsign/client/api/AtCommandExecutor.java +++ b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutor.java @@ -85,17 +85,16 @@ default CompletableFuture send(String command, Consumer consumer) AtCommandExecutor onReady(Consumer consumer); /** - * Returns the challenge from the {@code from:} command that this executor issued as its first - * command once the connection became ready, so that CRAM / PKAM authentication can reuse it - * rather than sending a second {@code from:}. The challenge is consumed at most once — the - * server's {@code from:} challenge is single-use, so a second authentication on the same - * connection (e.g. onboarding, which does CRAM then PKAM) gets {@code null} and falls back to - * sending its own {@code from:}. Also returns {@code null} when this executor was not configured - * with an atSign (and therefore did not send an initial {@code from:}). + * Returns this executor's {@link AtCommandExecutorContext authentication context}: the identity it + * authenticates as together with the single-use challenge from the {@code from:} it issued as its + * first command once ready. The authentication commands reuse that challenge rather than sending a + * second {@code from:}, and read the identity from the same context. Never {@code null} — an + * executor that was not configured with an atSign (and so issued no initial {@code from:}) returns + * {@link AtCommandExecutorContext#EMPTY}, whose challenge is always {@code null}. * - * @return the retained {@code from:} challenge, or {@code null} if none is available + * @return this executor's authentication context (never {@code null}) */ - default String getFromChallenge() { - return null; + default AtCommandExecutorContext getContext() { + return AtCommandExecutorContext.EMPTY; } } diff --git a/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java new file mode 100644 index 00000000..94a0b006 --- /dev/null +++ b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java @@ -0,0 +1,100 @@ +package org.atsign.client.api; + +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; + +/** + * The authentication context an {@link AtCommandExecutor} carries for the life of a connection: the + * identity it authenticates as ({@link #getAtSign() atSign}, {@link #getKeys() keys}, + * {@link #getConfig() config}) together with the single-use challenge from the {@code from:} the + * executor issues as its first command once ready. + * + *

+ * Grouping these behind {@link AtCommandExecutor#getContext()} lets the authentication commands + * take + * exactly what they need from one accessor rather than the executor interface growing a separate + * method per field. + * + *

+ * The identity fields are fixed for the life of the context. The challenge is session state: the + * executor {@link #setChallenge(String) retains} it once the initial {@code from:} completes, + * callers {@link #consumeChallenge() consume} it at most once (the server's {@code from:} challenge + * is single-use), and the executor {@link #clearChallenge() clears} it on disconnect — a challenge + * is only valid for the server session that issued it. + */ +public class AtCommandExecutorContext { + + /** + * A context with no identity and no challenge, returned by executors that were not configured with + * an atSign (and so issue no initial {@code from:}). {@link #consumeChallenge()} always yields + * {@code null}, so authentication falls back to sending its own {@code from:}. + */ + public static final AtCommandExecutorContext EMPTY = new AtCommandExecutorContext(null, null, null); + + private final AtSign atSign; + + private final AtKeys keys; + + private final Map config; + + private final AtomicReference challenge = new AtomicReference<>(); + + public AtCommandExecutorContext(AtSign atSign, AtKeys keys, Map config) { + this.atSign = atSign; + this.keys = keys; + this.config = config; + } + + /** + * @return the atSign this executor authenticates as, or {@code null} if none was configured + */ + public AtSign getAtSign() { + return atSign; + } + + /** + * @return the keys this executor authenticates with, or {@code null} if none were configured (e.g. + * an onboarding executor whose keys are generated mid-flow and supplied to the command + * directly) + */ + public AtKeys getKeys() { + return keys; + } + + /** + * @return the config sent in the {@code from:} command, or {@code null} if none was configured + */ + public Map getConfig() { + return config; + } + + /** + * Retains the challenge returned by the initial {@code from:}. Called by the executor once that + * command completes on the ready thread. + * + * @param challenge the challenge from the server's {@code from:} response + */ + public void setChallenge(String challenge) { + this.challenge.set(challenge); + } + + /** + * Returns the retained {@code from:} challenge and clears it, so it is consumed at most once. + * Whichever authentication (CRAM or PKAM) sends its digest first consumes the challenge; a second + * authentication on the same connection (e.g. onboarding, which does CRAM then PKAM) gets + * {@code null} and must issue its own {@code from:}. + * + * @return the retained challenge, or {@code null} if none is available + */ + public String consumeChallenge() { + return challenge.getAndSet(null); + } + + /** + * Clears any retained challenge. Called by the executor on disconnect — a challenge is only valid + * for the server session that issued it, so it must not survive into the next connection. + */ + public void clearChallenge() { + challenge.set(null); + } +} diff --git a/at_client/src/main/java/org/atsign/client/impl/AtClientImpl.java b/at_client/src/main/java/org/atsign/client/impl/AtClientImpl.java index 7968dc88..187164eb 100644 --- a/at_client/src/main/java/org/atsign/client/impl/AtClientImpl.java +++ b/at_client/src/main/java/org/atsign/client/impl/AtClientImpl.java @@ -122,6 +122,8 @@ public void startMonitor() { @Override public void stopMonitor() { isMonitoring.compareAndSet(true, false); + // this client owns its atSign/keys/config, so authenticate with those directly rather than via + // the executor's context (the executor is injected and may not carry this client's identity) executor.onReady(AuthenticationCommands.pkamAuthenticator(atSign, keys, config)); } diff --git a/at_client/src/main/java/org/atsign/client/impl/AtCommandExecutors.java b/at_client/src/main/java/org/atsign/client/impl/AtCommandExecutors.java index 915e1fb7..6cfa16df 100644 --- a/at_client/src/main/java/org/atsign/client/impl/AtCommandExecutors.java +++ b/at_client/src/main/java/org/atsign/client/impl/AtCommandExecutors.java @@ -77,8 +77,9 @@ public static AtCommandExecutor createCommandExecutor(String url, .reconnect(defaultIfNotSet(reconnect, SimpleReconnectStrategy.builder().build())) .queueLimit(queueLimit) .atSign(atSign) + .keys(keys) .clientConfig(createClientConfig(config)) - .onReady(defaultIfNotSet(onReady, createOnReady(atSign, keys, createClientConfig(config)))) + .onReady(defaultIfNotSet(onReady, createOnReady(atSign, keys))) .build(); } @@ -130,10 +131,12 @@ public static Map createClientConfig(Map config) return result; } - private static Consumer createOnReady(AtSign atSign, AtKeys keys, Map config) { + private static Consumer createOnReady(AtSign atSign, AtKeys keys) { Consumer onReady; if (atSign != null && keys != null) { - onReady = AuthenticationCommands.pkamAuthenticator(atSign, keys, config); + // the executor is built with this atSign/keys/config in its context, so the authenticator + // reads the identity (and reuses the initial from: challenge) from there + onReady = AuthenticationCommands.pkamAuthenticator(); } else { onReady = c -> { }; diff --git a/at_client/src/main/java/org/atsign/client/impl/commands/AuthenticationCommands.java b/at_client/src/main/java/org/atsign/client/impl/commands/AuthenticationCommands.java index e56e6b89..5602f452 100644 --- a/at_client/src/main/java/org/atsign/client/impl/commands/AuthenticationCommands.java +++ b/at_client/src/main/java/org/atsign/client/impl/commands/AuthenticationCommands.java @@ -15,6 +15,7 @@ import org.atsign.client.api.AtKeys; import org.atsign.client.api.AtCommandExecutor; +import org.atsign.client.api.AtCommandExecutorContext; import org.atsign.client.impl.util.EncryptionUtils; import org.atsign.client.impl.exceptions.AtException; import org.atsign.client.api.AtSign; @@ -30,6 +31,31 @@ public static Consumer pkamAuthenticator(AtSign atSign, AtKey return throwOnReadyException(executor -> authenticateWithPkam(executor, atSign, keys, config)); } + /** + * Returns an onReady consumer that authenticates using the identity carried by the executor's + * {@link AtCommandExecutorContext context} — the atSign, keys and config it was built with. This + * is the standard client path, where that identity is fixed for the connection. Onboarding, whose + * keys are generated mid-flow, uses {@link #pkamAuthenticator(AtSign, AtKeys, Map)} with explicit + * keys instead. + * + * @return an onReady consumer performing PKAM authentication from the executor's context + */ + public static Consumer pkamAuthenticator() { + return throwOnReadyException(AuthenticationCommands::authenticateWithPkam); + } + + /** + * Implements the protocol workflow / sequence for PKAM authentication, taking the atSign, keys and + * config from the executor's {@link AtCommandExecutorContext context}. + * + * @param executor The executor with which to send the commands; its context supplies the identity. + * @throws AtException If authentication fails. + */ + public static void authenticateWithPkam(AtCommandExecutor executor) throws AtException { + AtCommandExecutorContext context = executor.getContext(); + authenticateWithPkam(executor, context.getAtSign(), context.getKeys(), context.getConfig()); + } + /** * Implements the protocol workflow / sequence for PKAM authentication. * @@ -61,7 +87,7 @@ public static void authenticateWithPkam(AtCommandExecutor executor, // reuse the challenge from the initial from: if the executor already sent one, otherwise // send a from command and expect to receive a challenge - String challenge = executor.getFromChallenge(); + String challenge = consumeFromChallenge(executor); if (challenge == null) { String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).config(config).build(); String fromResponse = executor.sendSync(fromCommand); @@ -101,7 +127,7 @@ public static void authenticateWithCram(AtCommandExecutor executor, AtSign atSig // reuse the challenge from the initial from: if the executor already sent one, otherwise // send a from command and expect to receive a challenge - String challenge = executor.getFromChallenge(); + String challenge = consumeFromChallenge(executor); if (challenge == null) { String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).build(); String fromResponse = executor.sendSync(fromCommand); @@ -132,5 +158,16 @@ private static String createDigest(String cramSecret, String challenge) throws A } } - + /** + * Returns and clears the single-use {@code from:} challenge the executor retained after issuing its + * initial {@code from:}, or {@code null} if none is available. A {@code null} context is treated as + * "no retained challenge" so authentication falls back to sending its own {@code from:} — the same + * behaviour the interface's {@code getContext()} default (an EMPTY context) gives, kept null-safe + * so test doubles that leave {@code getContext()} unstubbed behave as they did before the context + * replaced the former {@code getFromChallenge()} accessor. + */ + private static String consumeFromChallenge(AtCommandExecutor executor) { + AtCommandExecutorContext context = executor.getContext(); + return context == null ? null : context.consumeChallenge(); + } } diff --git a/at_client/src/main/java/org/atsign/client/impl/netty/NettyAtCommandExecutor.java b/at_client/src/main/java/org/atsign/client/impl/netty/NettyAtCommandExecutor.java index 8f94de0c..dd4b18f3 100644 --- a/at_client/src/main/java/org/atsign/client/impl/netty/NettyAtCommandExecutor.java +++ b/at_client/src/main/java/org/atsign/client/impl/netty/NettyAtCommandExecutor.java @@ -21,6 +21,8 @@ import javax.net.ssl.SSLException; import org.atsign.client.api.AtCommandExecutor; +import org.atsign.client.api.AtCommandExecutorContext; +import org.atsign.client.api.AtKeys; import org.atsign.client.api.AtSign; import org.atsign.client.impl.AtEndpointSupplier; import org.atsign.client.impl.commands.CommandBuilders; @@ -107,11 +109,7 @@ String toLowerCase() { private volatile long lastReadMillis; - private final AtSign atSign; - - private final Map clientConfig; - - private final AtomicReference fromChallenge = new AtomicReference<>(); + private final AtCommandExecutorContext context; /** * Builder method for instantiating instances of a Netty based implementation of @@ -147,12 +145,12 @@ protected NettyAtCommandExecutor(AtEndpointSupplier endpoint, Long awaitReadyMillis, Boolean isVerbose, AtSign atSign, + AtKeys keys, Map clientConfig, Clock clock, Logger log) throws AtException { - this.atSign = atSign; - this.clientConfig = clientConfig; + this.context = new AtCommandExecutorContext(atSign, keys, clientConfig); this.endpointSupplier = checkNotNull(endpoint, "endpoint is not set"); this.maxFrameLength = defaultIfUnset(maxFrameLength, DEFAULT_MAX_FRAME_LENGTH); this.reconnectStrategy = defaultIfNull(reconnect, ReconnectStrategy.NONE); @@ -505,42 +503,41 @@ private Runnable createOnReadyRunnable() { * connection's atSign is established before any other verb (including a {@code scan} sent by * an onReady consumer prior to authenticating). The challenge returned by the server is * retained so that CRAM / PKAM authentication can reuse it instead of issuing a second - * {@code from:}. When no atSign was supplied to the builder this is a no-op and - * {@link #getFromChallenge()} returns {@code null}. + * {@code from:}. When no atSign was supplied to the builder this is a no-op and the context's + * {@link AtCommandExecutorContext#consumeChallenge() challenge} stays {@code null}. * *

* Runs on the onReady thread, where {@link #sendSync(String)} is permitted. On reconnect * the ready sequence re-runs, so the challenge is refreshed on each connect. */ private void sendFromIfRequired() { + AtSign atSign = context.getAtSign(); if (atSign == null) { return; } try { String fromCommand = CommandBuilders.fromCommandBuilder() .atSign(atSign) - .config(clientConfig) + .config(context.getConfig()) .build(); String fromResponse = sendSync(fromCommand); String challenge = matchDataStringNoWhitespace(throwExceptionIfError(fromResponse)); - fromChallenge.set(challenge); + context.setChallenge(challenge); } catch (AtException | ExecutionException | InterruptedException | RuntimeException e) { throw new AtOnReadyException("from command failed : " + e.getMessage(), e); } } /** - * Returns the challenge from the initial {@code from:} and clears it, so it is consumed at most - * once. The server's {@code from:} challenge is single-use — whichever authentication (CRAM or - * PKAM) sends its digest first consumes it — so a second authentication on the same connection - * (e.g. onboarding, which does CRAM then PKAM) must issue its own {@code from:} and gets - * {@code null} here to signal that fallback. The challenge is also cleared on disconnect — - * it is only valid for the server session that issued it — so a caller can never consume a - * challenge from a connection that has since dropped. + * Returns this executor's authentication context — the identity it authenticates as plus the + * single-use challenge from the initial {@code from:}. The challenge is retained by + * {@link #sendFromIfRequired()} on the ready thread, consumed at most once by whichever + * authentication sends its digest first, and cleared on disconnect (see {@code channelInactive}) + * so a caller can never consume a challenge from a connection that has since dropped. */ @Override - public String getFromChallenge() { - return fromChallenge.getAndSet(null); + public AtCommandExecutorContext getContext() { + return context; } private void onResponse(String msg) { @@ -581,8 +578,9 @@ public void exceptionCaught(ChannelHandlerContext context, Throwable cause) { @Override public void channelInactive(ChannelHandlerContext context) { channel = null; - // a from: challenge is only valid for the server session that just ended - fromChallenge.set(null); + // a from: challenge is only valid for the server session that just ended (this method's + // `context` parameter is the Netty ChannelHandlerContext, so qualify the executor's field) + NettyAtCommandExecutor.this.context.clearChallenge(); if (status.get().isClosedOrClosing()) { log.debug("connection closed"); } else { diff --git a/at_client/src/test/java/org/atsign/client/impl/commands/TestExecutorBuilder.java b/at_client/src/test/java/org/atsign/client/impl/commands/TestExecutorBuilder.java index 0f224e69..d9bd0cec 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/TestExecutorBuilder.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/TestExecutorBuilder.java @@ -13,6 +13,7 @@ import java.util.regex.Pattern; import org.atsign.client.api.AtCommandExecutor; +import org.atsign.client.api.AtCommandExecutorContext; import org.atsign.client.impl.util.JsonUtils; import org.mockito.Mockito; import org.mockito.stubbing.Answer; @@ -62,6 +63,9 @@ public TestExecutorBuilder stub(Pattern command, Exception ex) { public AtCommandExecutor build() throws ExecutionException, InterruptedException { AtCommandExecutor mock = Mockito.mock(AtCommandExecutor.class); + // an EMPTY context yields no challenge, so authentication sends its own from: (which the tests + // stub) rather than reusing an initial from: challenge this mock never issued + when(mock.getContext()).thenReturn(AtCommandExecutorContext.EMPTY); when(mock.sendSync(anyString())).thenAnswer((Answer) invocation -> { String command = invocation.getArgument(0); for (Map.Entry entry : mapping.entrySet()) { diff --git a/at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorTest.java b/at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorTest.java index 2e0f7be6..4113716d 100644 --- a/at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/netty/NettyAtCommandExecutorTest.java @@ -200,10 +200,10 @@ void testFromChallengeIsRetainedAfterInitialFromAndConsumedOnce() throws Excepti // the executor issues from: itself as its first command once ready assertThat(server.poll(), equalTo("from:@alice")); // so the first authentication reuses that challenge instead of sending a second from: - assertThat(executor.getFromChallenge(), equalTo(FROM_CHALLENGE)); + assertThat(executor.getContext().consumeChallenge(), equalTo(FROM_CHALLENGE)); // but it is single-use: a second authentication on the same connection gets null and // falls back to issuing its own from: - assertThat(executor.getFromChallenge(), nullValue()); + assertThat(executor.getContext().consumeChallenge(), nullValue()); } } @@ -214,7 +214,7 @@ void testNoFromChallengeWhenNoAtSignConfigured() throws Exception { // no atSign was supplied to the builder, so no initial from: is sent... assertThat(server.peek(), nullValue()); // ...and there is no retained challenge, so authentication sends its own from: - assertThat(executor.getFromChallenge(), nullValue()); + assertThat(executor.getContext().consumeChallenge(), nullValue()); } } @@ -231,7 +231,7 @@ void testFromChallengeIsClearedOnDisconnect() throws Exception { await().until(() -> !executor.isReady()); // the challenge was only valid for the session that just ended, so it must not survive // the disconnect - otherwise out-of-band auth could sign a challenge the server forgot - assertThat(executor.getFromChallenge(), nullValue()); + assertThat(executor.getContext().consumeChallenge(), nullValue()); } } @@ -248,11 +248,11 @@ void testFromChallengeIsRefreshedOnReconnect() throws Exception { connectionBuilder.atSign(AtSign.createAtSign("@alice")).reconnect(reconnectStrategy); try (NettyAtCommandExecutor executor = connectionBuilder.build()) { await().until(executor::isReady); - assertThat(executor.getFromChallenge(), equalTo("challenge-1")); + assertThat(executor.getContext().consumeChallenge(), equalTo("challenge-1")); server.closeClientSocket(); // the reconnect re-runs the ready sequence, which issues a fresh from: and retains its // challenge in place of the old one - await().until(() -> "challenge-2".equals(executor.getFromChallenge())); + await().until(() -> "challenge-2".equals(executor.getContext().consumeChallenge())); } }