From 1ddc23cb8cf9b979eed521a7fa9e0a4a6e7c4556 Mon Sep 17 00:00:00 2001 From: gkc Date: Sat, 11 Jul 2026 14:09:30 +0100 Subject: [PATCH 1/8] 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 ea9d8e32008455c494382a94409025b901cc1cad Mon Sep 17 00:00:00 2001 From: gkc Date: Sun, 12 Jul 2026 18:57:27 +0100 Subject: [PATCH 2/8] 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())); } } From 6c371585c7e00ae1389e56ab0ae1d8a5ce880fd8 Mon Sep 17 00:00:00 2001 From: gkc Date: Tue, 21 Jul 2026 14:32:00 +0100 Subject: [PATCH 3/8] refactor(auth): issue from: via onReady and reuse its challenge across connect paths Reworks the earlier attempts per @akafredperry's review: the command executor stays transport-only (no protocol knowledge, no challenge cache), so NettyAtCommandExecutor and the AtCommandExecutor interface revert to trunk. The "send from: first, remember the challenge" behaviour lives in the onReady layer instead. Every connection that has an atSign now issues from:@atSign as its first command (via an onReady consumer wired by AtCommandExecutors.createOnReady), so proxies / gateways can route it; authentication then reuses that challenge rather than sending a second from:. Connections with no atSign (the atDirectory / root lookup) send no from:. AtCommandExecutorContext bundles the connection identity (atSign, keys, config) and the single-use challenge. By default the builder owns it and closes it over the onReady consumers; a caller may also supply one via context() so an imperative flow that authenticates on the connection can reach the same challenge. CLI consistency (Activate): - onboard threads the connection context into EnrollCommands.onboard, whose CRAM reuses the from: challenge issued on connect (before the intervening scan); PKAM then issues its own from: (challenge is single-use, and it authenticates with the freshly-enrolled keys) -> 2 from: total, matching trunk. - complete uses a connection with no onReady from:, so its own retried PKAM is from-first -> 1 from: per attempt. Reusing a connect-time challenge would be stale by the time the deliberately-delayed, retried PKAM runs. - AbstractCli now builds every connection through AtCommandExecutorBuilder. Cross-tier safety properties: the from: challenge is single-use (consumeChallenge() clears it) and per-connection; on reconnect the onReady re-runs and refreshes it, so a reused challenge always belongs to the currently-live connection. onboard's CRAM reuse assumes the atServer keeps the challenge valid across the intervening unauthenticated scan -- to be confirmed by the functional / e2e suites (mocks cannot exercise the onReady from:). --- .../atsign/client/api/AtCommandExecutor.java | 14 -- .../client/api/AtCommandExecutorContext.java | 100 +++++--------- .../org/atsign/client/impl/AtClientImpl.java | 2 - .../client/impl/AtCommandExecutors.java | 41 ++++-- .../atsign/client/impl/cli/AbstractCli.java | 57 ++++++-- .../org/atsign/client/impl/cli/Activate.java | 17 ++- .../impl/commands/AuthenticationCommands.java | 128 +++++++++++++----- .../client/impl/commands/EnrollCommands.java | 18 ++- .../impl/netty/NettyAtCommandExecutor.java | 59 -------- .../commands/AuthenticationCommandsTest.java | 63 +++++++++ .../impl/commands/EnrollCommandsTest.java | 40 +++++- .../impl/commands/TestExecutorBuilder.java | 4 - .../netty/NettyAtCommandExecutorTest.java | 78 ----------- 13 files changed, 319 insertions(+), 302 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 5f0269eb..5c66fb5e 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,18 +83,4 @@ default CompletableFuture send(String command, Consumer consumer) * @return this (to allow chaining / fluent style invocation) */ AtCommandExecutor onReady(Consumer consumer); - - /** - * 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 this executor's authentication context (never {@code 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 index 94a0b006..89ee07d2 100644 --- a/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java +++ b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java @@ -3,74 +3,51 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicReference; +import lombok.AccessLevel; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import lombok.Value; + /** - * 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. + * The identity a connection authenticates as — its {@link #getAtSign() atSign}, + * {@link #getKeys() keys} and {@link #getConfig() config} — together with the single-use challenge + * from the {@code from:} that is issued as the first command once the connection is 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 context is created by the builder (see + * {@code AtCommandExecutors#createCommandExecutor}) and closed over by the {@code onReady} + * consumers + * it wires, so the {@code from:} sender can retain the challenge and the authentication that + * follows + * on the same connection can reuse it rather than issuing a second {@code from:}. The command + * executor itself is pure transport and knows nothing about this context. * *

- * 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. + * The identity fields are fixed for the life of the context. The challenge is per-connection state: + * {@link #setChallenge(String) retained} when the initial {@code from:} completes and + * {@link #consumeChallenge() consumed} at most once (the server's {@code from:} challenge is + * single-use). On reconnect the ready sequence re-runs, so a fresh challenge overwrites any + * previous + * one before it is consumed. */ +@Value 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; + AtSign atSign; - 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; - } + AtKeys keys; - /** - * @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; - } + Map config; - /** - * @return the config sent in the {@code from:} command, or {@code null} if none was configured - */ - public Map getConfig() { - return config; - } + @Getter(AccessLevel.NONE) + @EqualsAndHashCode.Exclude + @ToString.Exclude + AtomicReference challenge = new AtomicReference<>(); /** - * Retains the challenge returned by the initial {@code from:}. Called by the executor once that - * command completes on the ready thread. + * Retains the challenge returned by the initial {@code from:}, so the authentication that follows + * on the same connection can reuse it. * * @param challenge the challenge from the server's {@code from:} response */ @@ -80,21 +57,12 @@ public void setChallenge(String 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:}. + * Whichever authentication (CRAM or PKAM) sends its digest first consumes it; anything else on the + * same connection 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 187164eb..7968dc88 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,8 +122,6 @@ 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 6cfa16df..8c74c731 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 @@ -13,6 +13,7 @@ import java.util.function.Consumer; 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.commands.AuthenticationCommands; @@ -56,6 +57,7 @@ public class AtCommandExecutors { public static AtCommandExecutor createCommandExecutor(String url, AtSign atSign, AtKeys keys, + AtCommandExecutorContext context, Consumer onReady, Map config, Long timeoutMillis, @@ -65,21 +67,27 @@ public static AtCommandExecutor createCommandExecutor(String url, Boolean isVerbose) throws AtException { + // the context is closed over by the onReady consumers the builder wires (see createOnReady); the + // command executor itself stays pure transport and knows nothing about it. By default the builder + // owns it, but a caller may supply one (via context()) so an imperative flow that authenticates + // on the connection can reach the same from: challenge — see EnrollCommands#onboard. + if (context == null) { + context = new AtCommandExecutorContext(atSign, keys, createClientConfig(config)); + } + AtSign endpointAtSign = context.getAtSign(); + if (AtEndpointSuppliers.isProxyUrl(url)) { - checkNotNull(atSign, "atSign not set"); + checkNotNull(endpointAtSign, "atSign not set"); } return NettyAtCommandExecutor.builder() - .endpoint(AtEndpointSuppliers.builder().url(url).atSign(atSign).build()) + .endpoint(AtEndpointSuppliers.builder().url(url).atSign(endpointAtSign).build()) .isVerbose(isVerbose) .timeoutMillis(defaultIfNotSet(timeoutMillis, DEFAULT_TIMEOUT_MILLIS)) .awaitReadyMillis(defaultIfNotSet(awaitReadyMillis, DEFAULT_TIMEOUT_MILLIS)) .reconnect(defaultIfNotSet(reconnect, SimpleReconnectStrategy.builder().build())) .queueLimit(queueLimit) - .atSign(atSign) - .keys(keys) - .clientConfig(createClientConfig(config)) - .onReady(defaultIfNotSet(onReady, createOnReady(atSign, keys))) + .onReady(defaultIfNotSet(onReady, createOnReady(context))) .build(); } @@ -131,16 +139,21 @@ public static Map createClientConfig(Map config) return result; } - private static Consumer createOnReady(AtSign atSign, AtKeys keys) { - Consumer onReady; - if (atSign != null && keys != null) { - // 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 -> { + /** + * The default protocol for a newly-ready connection. A connection with no atSign (e.g. one talking + * to the atDirectory / root server) sends nothing. Every connection that has an atSign issues + * {@code from:@atSign} first so that proxies / gateways can route it; if keys are also present it + * then authenticates with PKAM, reusing the challenge from that initial {@code from:}. + */ + private static Consumer createOnReady(AtCommandExecutorContext context) { + if (context.getAtSign() == null) { + return c -> { }; } + Consumer onReady = AuthenticationCommands.sendFrom(context); + if (context.getKeys() != null) { + onReady = onReady.andThen(AuthenticationCommands.pkamAuthenticator(context)); + } return onReady; } 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 f02a5783..8f320e48 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 @@ -4,16 +4,14 @@ import java.util.concurrent.TimeUnit; 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.AtEndpointSuppliers; -import org.atsign.client.impl.commands.AuthenticationCommands; +import org.atsign.client.impl.AtCommandExecutors; +import org.atsign.client.impl.AtCommandExecutors.AtCommandExecutorBuilder; import org.atsign.client.impl.common.SimpleReconnectStrategy; import org.atsign.client.impl.exceptions.AtClientConfigException; import org.atsign.client.impl.exceptions.AtException; -import org.atsign.client.impl.netty.NettyAtCommandExecutor; -import org.atsign.client.impl.netty.NettyAtCommandExecutor.NettyAtCommandExecutorBuilder; import org.atsign.client.impl.util.KeysUtils; import picocli.CommandLine.Option; @@ -87,26 +85,55 @@ protected static String ensureNotNull(String value, String defaultValue) { } protected AtCommandExecutor createConnection(String rootUrl, AtSign atSign, int retries) throws AtException { - return createCommandExecutorBuilder(rootUrl, atSign, retries, verbose).build(); + // no keys: the builder wires an onReady that issues from:@atSign only (so proxies can route it) + return connectionBuilder(rootUrl, retries, verbose).atSign(atSign).build(); } protected AtCommandExecutor createAuthenticatedConnection(String rootUrl, AtSign atSign, int retries) throws AtException { - return createCommandExecutorBuilder(rootUrl, atSign, retries, verbose) - .onReady(AuthenticationCommands.pkamAuthenticator(atSign, getKeys(), null)) - .build(); + // keys present: the builder wires from:@atSign followed by PKAM (reusing the from: challenge) + return connectionBuilder(rootUrl, retries, verbose).atSign(atSign).keys(getKeys()).build(); + } + + /** + * A connection that carries the given {@code context}, so an imperative flow driving it (e.g. + * onboarding) can reach the {@code from:} challenge the connection issues on connect and reuse it + * for authentication. The builder wires from:@atSign (plus PKAM if the context has keys) around + * that same context instance. + */ + protected AtCommandExecutor createConnection(AtCommandExecutorContext context, int retries) throws AtException { + return connectionBuilder(rootUrl, retries, verbose).context(context).build(); + } + + /** + * A connection whose initial {@code from:} is issued by the caller's own first command rather than + * on connect — for flows that authenticate imperatively and must control the timing (e.g. a + * pending-retry loop), where a connect-time {@code from:} challenge could go stale before it is + * used. No onReady {@code from:} is wired, so the caller's first command (its authentication) + * establishes the atSign for proxies / gateways. + */ + protected AtCommandExecutor createConnectionForImperativeAuth(String rootUrl, AtSign atSign, int retries) + throws AtException { + return connectionBuilder(rootUrl, retries, verbose).atSign(atSign).onReady(executor -> { + }).build(); + } + + /** + * Creates a keyless {@link AtCommandExecutorContext} for this CLI's atSign, to be passed to + * {@link #createConnection(AtCommandExecutorContext, int)} and threaded into an imperative flow + * that authenticates on the connection and reuses its {@code from:} challenge. + */ + protected AtCommandExecutorContext newConnectionContext() { + return new AtCommandExecutorContext(atSign, null, AtCommandExecutors.createClientConfig(null)); } - private static NettyAtCommandExecutorBuilder createCommandExecutorBuilder(String rootUrl, AtSign atSign, int retries, - boolean verbose) { - AtEndpointSupplier endpoint = AtEndpointSuppliers.builder().url(rootUrl).atSign(atSign).build(); + private static AtCommandExecutorBuilder connectionBuilder(String rootUrl, int retries, boolean verbose) { SimpleReconnectStrategy reconnect = SimpleReconnectStrategy.builder() .maxReconnectRetries(retries) .reconnectPauseMillis(TimeUnit.SECONDS.toMillis(2)) .build(); - return NettyAtCommandExecutor.builder() - .endpoint(endpoint) - .atSign(atSign) + return AtCommandExecutors.builder() + .url(rootUrl) .reconnect(reconnect) .isVerbose(verbose); } diff --git a/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java b/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java index 5bf2cc40..bf75ef11 100644 --- a/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java +++ b/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java @@ -14,6 +14,7 @@ import org.atsign.client.api.AtKeys; import org.atsign.client.api.AtCommandExecutor; +import org.atsign.client.api.AtCommandExecutorContext; import org.atsign.client.impl.commands.EnrollCommands; import org.atsign.client.impl.commands.ScanCommands; import org.atsign.client.impl.common.EnrollmentId; @@ -180,19 +181,20 @@ public Activate addNamespace(String namespace, String accessControl) { } public EnrollmentId onboard() throws Exception { - try (AtCommandExecutor executor = createConnection(rootUrl, atSign, connectionRetries)) { - return onboard(executor); + AtCommandExecutorContext context = newConnectionContext(); + try (AtCommandExecutor executor = createConnection(context, connectionRetries)) { + return onboard(executor, context); } } - public EnrollmentId onboard(AtCommandExecutor executor) throws Exception { + public EnrollmentId onboard(AtCommandExecutor executor, AtCommandExecutorContext context) throws Exception { File file = getAtKeysFile(keysFile, atSign); if (!overwriteKeysFile) { checkNotExists(file); } AtKeys keys = generateAtKeys(true); keys = EnrollCommands.onboard(executor, - atSign, + context, keys, cramSecret, ensureNotNull(appName, DEFAULT_FIRST_APP), @@ -292,7 +294,9 @@ public EnrollmentId enroll(AtCommandExecutor executor) throws Exception { } public void complete() throws Exception { - try (AtCommandExecutor executor = createConnection(rootUrl, atSign, connectionRetries)) { + // complete authenticates imperatively (and retries on "pending"), so it issues its own from: + // rather than reusing a connect-time challenge that could go stale before the retried PKAM + try (AtCommandExecutor executor = createConnectionForImperativeAuth(rootUrl, atSign, connectionRetries)) { complete(executor); } } @@ -304,7 +308,8 @@ public void complete(AtCommandExecutor executor) throws Exception { } public void complete(int retries, long sleepDuration, TimeUnit sleepUnit) throws Exception { - try (AtCommandExecutor executor = createConnection(rootUrl, atSign, connectionRetries)) { + // see complete(): imperative, retried PKAM issues its own from: + try (AtCommandExecutor executor = createConnectionForImperativeAuth(rootUrl, atSign, connectionRetries)) { complete(executor, retries, sleepDuration, sleepUnit); } } 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 5602f452..bcca8ea7 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 @@ -27,33 +27,44 @@ */ public class AuthenticationCommands { - public static Consumer pkamAuthenticator(AtSign atSign, AtKeys keys, Map config) { - 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. + * Returns an {@code onReady} consumer that issues {@code from:@atSign} as the first command on the + * connection and retains the challenge from the response in the given {@code context}. This + * establishes the connection's atSign up front so that proxies / gateways can route it, and lets + * any authentication that follows on the same connection reuse the challenge (see + * {@link #pkamAuthenticator(AtCommandExecutorContext)}) rather than issuing a second {@code from:}. * - * @return an onReady consumer performing PKAM authentication from the executor's context + * @param context the connection context; supplies the atSign / config and receives the challenge + * @return an onReady consumer that sends the initial {@code from:} */ - public static Consumer pkamAuthenticator() { - return throwOnReadyException(AuthenticationCommands::authenticateWithPkam); + public static Consumer sendFrom(AtCommandExecutorContext context) { + return throwOnReadyException(executor -> { + String fromCommand = CommandBuilders.fromCommandBuilder() + .atSign(context.getAtSign()) + .config(context.getConfig()) + .build(); + String fromResponse = executor.sendSync(fromCommand); + context.setChallenge(matchDataStringNoWhitespace(throwExceptionIfError(fromResponse))); + }); } /** - * Implements the protocol workflow / sequence for PKAM authentication, taking the atSign, keys and - * config from the executor's {@link AtCommandExecutorContext context}. + * Returns an {@code onReady} consumer that authenticates with PKAM using the identity in the given + * {@code context}, reusing the challenge from an initial {@code from:} (as issued by + * {@link #sendFrom(AtCommandExecutorContext)}) when one is available and otherwise issuing its own. + * This is the standard client path, where the identity is fixed for the connection. * - * @param executor The executor with which to send the commands; its context supplies the identity. - * @throws AtException If authentication fails. + * @param context the connection context; supplies the atSign / keys / config and the challenge + * @return an onReady consumer that performs PKAM authentication */ - public static void authenticateWithPkam(AtCommandExecutor executor) throws AtException { - AtCommandExecutorContext context = executor.getContext(); - authenticateWithPkam(executor, context.getAtSign(), context.getKeys(), context.getConfig()); + public static Consumer pkamAuthenticator(AtCommandExecutorContext context) { + return throwOnReadyException(executor -> authenticateWithPkam(executor, context.getAtSign(), + context.getKeys(), context.getConfig(), + context.consumeChallenge())); + } + + public static Consumer pkamAuthenticator(AtSign atSign, AtKeys keys, Map config) { + return throwOnReadyException(executor -> authenticateWithPkam(executor, atSign, keys, config)); } /** @@ -83,11 +94,32 @@ public static void authenticateWithPkam(AtCommandExecutor executor, AtKeys keys, Map config) throws AtException { + authenticateWithPkam(executor, atSign, keys, config, null); + } + + /** + * Implements the protocol workflow / sequence for PKAM authentication, reusing an already-issued + * {@code from:} challenge when one is supplied. + * + * @param executor The executor with which to send the commands. + * @param atSign The asign to authenticate. + * @param keys The keys to use to authenticate. + * @param config The map of configuration values to send in the from command. + * @param reusableChallenge The challenge from an initial {@code from:} to reuse, or {@code null} to + * issue a fresh {@code from:}. + * @throws AtException If authentication fails. + */ + private static void authenticateWithPkam(AtCommandExecutor executor, + AtSign atSign, + AtKeys keys, + Map config, + String reusableChallenge) + throws AtException { try { - // reuse the challenge from the initial from: if the executor already sent one, otherwise + // reuse the challenge from the initial from: if one was issued on this connection, otherwise // send a from command and expect to receive a challenge - String challenge = consumeFromChallenge(executor); + String challenge = reusableChallenge; if (challenge == null) { String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).config(config).build(); String fromResponse = executor.sendSync(fromCommand); @@ -113,6 +145,24 @@ public static void authenticateWithPkam(AtCommandExecutor executor, } } + /** + * Implements the protocol workflow / sequence for CRAM authentication, reusing the challenge from + * an initial {@code from:} (as issued by {@link #sendFrom(AtCommandExecutorContext)}) held in the + * given {@code context} when one is available and otherwise issuing its own. Used by onboarding, + * which issues a {@code from:} on connect, runs a connectivity scan, then authenticates. + * + * @param executor The executor with which to send the commands. + * @param context The connection context; supplies the atSign and the retained challenge. + * @param cramSecret The cramSecret that was assigned during At Server provisioning. + * @throws AtException If authentication fails. + */ + public static void authenticateWithCram(AtCommandExecutor executor, + AtCommandExecutorContext context, + String cramSecret) + throws AtException { + authenticateWithCram(executor, context.getAtSign(), cramSecret, context.consumeChallenge()); + } + /** * Implements the protocol workflow / sequence for CRAM authentication. * @@ -123,11 +173,30 @@ public static void authenticateWithPkam(AtCommandExecutor executor, */ public static void authenticateWithCram(AtCommandExecutor executor, AtSign atSign, String cramSecret) throws AtException { + authenticateWithCram(executor, atSign, cramSecret, null); + } + + /** + * Implements the protocol workflow / sequence for CRAM authentication, reusing an already-issued + * {@code from:} challenge when one is supplied. + * + * @param executor The executor with which to send the commands. + * @param atSign The asign to authenticate. + * @param cramSecret The cramSecret that was assigned during At Server provisioning. + * @param reusableChallenge The challenge from an initial {@code from:} to reuse, or {@code null} to + * issue a fresh {@code from:}. + * @throws AtException If authentication fails. + */ + private static void authenticateWithCram(AtCommandExecutor executor, + AtSign atSign, + String cramSecret, + String reusableChallenge) + throws AtException { try { - // reuse the challenge from the initial from: if the executor already sent one, otherwise + // reuse the challenge from the initial from: if one was issued on this connection, otherwise // send a from command and expect to receive a challenge - String challenge = consumeFromChallenge(executor); + String challenge = reusableChallenge; if (challenge == null) { String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).build(); String fromResponse = executor.sendSync(fromCommand); @@ -157,17 +226,4 @@ private static String createDigest(String cramSecret, String challenge) throws A throw new AtEncryptionException("failed to generate cramDigest", e); } } - - /** - * 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/commands/EnrollCommands.java b/at_client/src/main/java/org/atsign/client/impl/commands/EnrollCommands.java index 9b1694cf..0d51be47 100644 --- a/at_client/src/main/java/org/atsign/client/impl/commands/EnrollCommands.java +++ b/at_client/src/main/java/org/atsign/client/impl/commands/EnrollCommands.java @@ -14,6 +14,7 @@ import java.util.stream.Collectors; import org.atsign.client.api.AtCommandExecutor; +import org.atsign.client.api.AtCommandExecutorContext; import org.atsign.client.api.AtKeyNames; import org.atsign.client.api.AtKeys; import org.atsign.client.api.AtSign; @@ -32,8 +33,10 @@ public class EnrollCommands { * app/device keys that can approve subsequent enrollments. * * @param executor The {@link AtCommandExecutor} to use. - * @param atSign The AtSign that corresponds to the executor. - * @param keys The {@link AtKeys} for the {@link AtSign}, these should already be populated. + * @param context The connection context for the executor; supplies the atSign and the {@code from:} + * challenge that CRAM reuses (the connection issues {@code from:} on connect, before the + * connectivity scan below). + * @param keys The {@link AtKeys} for the atSign, these should already be populated. * @param cramSecret The CRAM secret. * @param appName The app name for this first enrollment. * @param deviceName The device name for this first enrollment. @@ -43,13 +46,14 @@ public class EnrollCommands { * @throws AtException If any of the commands fail. */ public static AtKeys onboard(AtCommandExecutor executor, - AtSign atSign, + AtCommandExecutorContext context, AtKeys keys, String cramSecret, String appName, String deviceName, boolean deleteCramKey) throws AtException { + AtSign atSign = context.getAtSign(); try { // verify that the executor is connected to the atsigns atserver @@ -60,8 +64,8 @@ public static AtKeys onboard(AtCommandExecutor executor, throw new IllegalStateException("not connected to the atsign's at server"); } - // authenticate with CRAM - AuthenticationCommands.authenticateWithCram(executor, atSign, cramSecret); + // authenticate with CRAM, reusing the from: challenge the connection issued on connect + AuthenticationCommands.authenticateWithCram(executor, context, cramSecret); // send an enroll request which should automatically be approved after CRAM authentication String requestCommand = CommandBuilders.enrollCommandBuilder() @@ -79,7 +83,9 @@ public static AtKeys onboard(AtCommandExecutor executor, .enrollmentId(createEnrollmentId(response.get("enrollmentId"))) .build(); - // authenticate with PKAM + // authenticate with PKAM — issues its own from: (the connection's challenge was single-use and + // is spent by CRAM above; PKAM also authenticates with the freshly-enrolled keys, not the + // connection identity) AuthenticationCommands.authenticateWithPkam(executor, atSign, keys); // explicitly store the public encryption key in the atserver 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 dd4b18f3..7d12747c 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,15 +2,12 @@ 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; @@ -21,11 +18,7 @@ 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; import org.atsign.client.impl.common.ReconnectStrategy; import org.atsign.client.impl.common.CommandElement; import org.atsign.client.impl.common.CommandQueue; @@ -109,8 +102,6 @@ String toLowerCase() { private volatile long lastReadMillis; - private final AtCommandExecutorContext context; - /** * Builder method for instantiating instances of a Netty based implementation of * {@link AtCommandExecutor} @@ -144,13 +135,9 @@ protected NettyAtCommandExecutor(AtEndpointSupplier endpoint, Integer queueLimit, Long awaitReadyMillis, Boolean isVerbose, - AtSign atSign, - AtKeys keys, - Map clientConfig, Clock clock, Logger log) throws AtException { - 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); @@ -479,7 +466,6 @@ private Runnable createOnReadyRunnable() { threadFactory.markCurrentThreadOnReadyThread(); try { readyingLock.lock(); - sendFromIfRequired(); onReadyConsumer.get().accept(this); } catch (AtOnReadyException e) { log.error("onReady exception", e); @@ -498,48 +484,6 @@ 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 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(context.getConfig()) - .build(); - String fromResponse = sendSync(fromCommand); - String challenge = matchDataStringNoWhitespace(throwExceptionIfError(fromResponse)); - context.setChallenge(challenge); - } catch (AtException | ExecutionException | InterruptedException | RuntimeException e) { - throw new AtOnReadyException("from command failed : " + e.getMessage(), e); - } - } - - /** - * 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 AtCommandExecutorContext getContext() { - return context; - } - private void onResponse(String msg) { if (isVerbose) { log.info("RCVD: {}", msg); @@ -578,9 +522,6 @@ 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 (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/AuthenticationCommandsTest.java b/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java index c52d60dd..fb7e03b9 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java @@ -6,7 +6,11 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.matches; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import org.atsign.client.api.AtCommandExecutorContext; import org.atsign.client.api.AtKeys; import org.atsign.client.api.AtCommandExecutor; import org.atsign.client.impl.exceptions.AtOnReadyException; @@ -103,4 +107,63 @@ public void testPkamAuthenticatorThrowsOnReadyException() throws Exception { assertThat(ex, instanceOf(AtOnReadyException.class)); assertThat(ex.getMessage(), containsString("deliberate")); } + + @Test + public void testSendFromIssuesFromAndRetainsChallenge() throws Exception { + AtCommandExecutor executor = TestExecutorBuilder.builder() + .stub("from:@alice", "data:challenge") + .build(); + AtCommandExecutorContext context = new AtCommandExecutorContext(createAtSign("@alice"), null, null); + + AuthenticationCommands.sendFrom(context).accept(executor); + + verify(executor, times(1)).sendSync(matches("from:.*")); + assertThat(context.consumeChallenge(), is("challenge")); + } + + @Test + public void testPkamAuthenticatorReusesTheInitialFromChallenge() throws Exception { + AtKeys keys = AtKeys.builder().apkamKeyPair(generateRSAKeyPair()).build(); + AtCommandExecutor executor = TestExecutorBuilder.builder() + .stub("from:@alice", "data:challenge") + .stub("pkam:[^{].+", "data:success") + .build(); + AtCommandExecutorContext context = new AtCommandExecutorContext(createAtSign("@alice"), keys, null); + + // the from: sender runs first (as wired by createOnReady), then PKAM reuses its challenge + AuthenticationCommands.sendFrom(context).accept(executor); + AuthenticationCommands.pkamAuthenticator(context).accept(executor); + + // exactly one from: for the whole connection — PKAM did not issue a second one + verify(executor, times(1)).sendSync(matches("from:.*")); + verify(executor, times(1)).sendSync(matches("pkam:.*")); + assertThat(context.consumeChallenge(), is(nullValue())); + } + + @Test + public void testPkamAuthenticatorIssuesItsOwnFromWhenNoChallengeRetained() throws Exception { + AtKeys keys = AtKeys.builder().apkamKeyPair(generateRSAKeyPair()).build(); + AtCommandExecutor executor = TestExecutorBuilder.builder() + .stub("from:@alice", "data:challenge") + .stub("pkam:[^{].+", "data:success") + .build(); + AtCommandExecutorContext context = new AtCommandExecutorContext(createAtSign("@alice"), keys, null); + + // no prior sendFrom: the authenticator must issue its own from: to obtain a challenge + AuthenticationCommands.pkamAuthenticator(context).accept(executor); + + verify(executor, times(1)).sendSync(matches("from:.*")); + verify(executor, times(1)).sendSync(matches("pkam:.*")); + } + + @Test + public void testRetainedChallengeIsConsumedAtMostOnce() { + AtCommandExecutorContext context = new AtCommandExecutorContext(createAtSign("@alice"), null, null); + context.setChallenge("challenge"); + + // single-use: the first consumer gets it, a second (e.g. a further auth on the same connection) + // gets null and falls back to issuing its own from: + assertThat(context.consumeChallenge(), is("challenge")); + assertThat(context.consumeChallenge(), is(nullValue())); + } } diff --git a/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java b/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java index fe07ee02..ff3c5e1a 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java @@ -8,11 +8,15 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.matches; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import java.util.List; import org.atsign.client.api.AtKeys; import org.atsign.client.api.AtCommandExecutor; +import org.atsign.client.api.AtCommandExecutorContext; import org.atsign.client.impl.common.EnrollmentId; import org.atsign.client.impl.exceptions.AtException; import org.atsign.client.api.AtSign; @@ -63,7 +67,8 @@ public void testOnboardThrowsExceptionIfSigningPublicKeyIsMissing() throws Excep .build(); Exception ex = assertThrows(Exception.class, - () -> EnrollCommands.onboard(executor, atSign, keys, "secret", "app", "device", false)); + () -> EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, null, null), + keys, "secret", "app", "device", false)); assertThat(ex.getMessage(), containsString("not connected to the atsign's at server")); } @@ -102,12 +107,43 @@ public void testOnboard() throws Exception { .stub("update:public:publickey@alice .+", "data:1") .build(); - AtKeys newKeys = EnrollCommands.onboard(executor, atSign, keys, "secret", "app", "device", false); + AtKeys newKeys = EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, null, null), keys, "secret", + "app", "device", false); assertThat(newKeys, is(not(sameInstance(keys)))); assertThat(newKeys.getEnrollmentId(), equalTo(createEnrollmentId("904dcbf7"))); } + @Test + public void testOnboardReusesTheConnectionFromChallengeForCram() throws Exception { + AtSign atSign = createAtSign("@alice"); + AtKeys keys = AtKeys.builder() + .apkamKeyPair(generateRSAKeyPair()) + .encryptKeyPair(generateRSAKeyPair()) + .build(); + + // simulate the connection having issued from: on connect (sendFrom) and retained the challenge + AtCommandExecutorContext context = new AtCommandExecutorContext(atSign, null, null); + context.setChallenge("challenge"); + + AtCommandExecutor executor = TestExecutorBuilder.builder() + .stub("scan", "data:[\"signing_publickey@alice\"]") + .stub("cram:7e91508d5.+", "data:success") + .stub("enroll:request.+", "data:{\"enrollmentId\":\"904dcbf7\",\"status\":\"approved\"}") + .stub("from:@alice", "data:challenge2") + .stub("pkam:[^{].+", "data:success") + .stub("update:public:publickey@alice .+", "data:1") + .build(); + + EnrollCommands.onboard(executor, context, keys, "secret", "app", "device", false); + + // CRAM reused the retained challenge, so exactly ONE from: reaches the wire — PKAM's own (the + // challenge is single-use and PKAM authenticates with the freshly-enrolled keys) + verify(executor, times(1)).sendSync(matches("from:.*")); + verify(executor, times(1)).sendSync(matches("cram:.*")); + assertThat(context.consumeChallenge(), is(nullValue())); + } + @Test public void testEnroll() throws Exception { AtSign atSign = createAtSign("@alice"); 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 d9bd0cec..0f224e69 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,7 +13,6 @@ 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; @@ -63,9 +62,6 @@ 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 4113716d..de8dfd38 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,7 +21,6 @@ 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; @@ -41,8 +40,6 @@ @Slf4j class NettyAtCommandExecutorTest { - private static final String FROM_CHALLENGE = "_a1b2c3d4@alice:12345678"; - private TestServer server; private TestEndPointSupplier endPointSupplier; private NettyAtCommandExecutorBuilder connectionBuilder; @@ -191,71 +188,6 @@ 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.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.getContext().consumeChallenge(), 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.getContext().consumeChallenge(), 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.getContext().consumeChallenge(), 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.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.getContext().consumeChallenge())); - } - } - @Test void testSendSync() throws Exception { try (AtCommandExecutor executor = connectionBuilder.build()) { @@ -713,16 +645,6 @@ 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 fcd4f8d7059959fb13a307445c2c44f3da5a40d4 Mon Sep 17 00:00:00 2001 From: gkc Date: Tue, 21 Jul 2026 14:57:12 +0100 Subject: [PATCH 4/8] fix(build): drop Javadoc @link to Lombok-generated getters The AtCommandExecutorContext class doc linked to getAtSign()/getKeys()/ getConfig(), which Lombok's @Value generates at compile time. There is no delombok step feeding the javadoc tool, so it cannot resolve them and the `mvn install` javadoc:jar goal failed with "reference not found" (caught by CI on JDK 25; local `mvn test` does not run javadoc). Replaced the links with plain {@code} references. --- .../org/atsign/client/api/AtCommandExecutorContext.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 index 89ee07d2..6b62419b 100644 --- a/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java +++ b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java @@ -10,9 +10,10 @@ import lombok.Value; /** - * The identity a connection authenticates as — its {@link #getAtSign() atSign}, - * {@link #getKeys() keys} and {@link #getConfig() config} — together with the single-use challenge - * from the {@code from:} that is issued as the first command once the connection is ready. + * The identity a connection authenticates as — its {@code atSign}, {@code keys} and {@code config} + * — + * together with the single-use challenge from the {@code from:} that is issued as the first command + * once the connection is ready. * *

* The context is created by the builder (see From ec3989a3842089b655a96d1252467aa915c8620d Mon Sep 17 00:00:00 2001 From: gkc Date: Tue, 21 Jul 2026 15:53:16 +0100 Subject: [PATCH 5/8] refactor(auth): single-source onboarding context, drop redundant builder input, clarify factory names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-review cleanup of the from:-first / challenge-reuse change (no wire behaviour change; 503 unit tests + javadoc green): - EnrollCommands.onboard takes its identity solely from the AtCommandExecutorContext (atSign AND keys) rather than a separate AtKeys param — the context is the single source of identity for onboarding. - Removed the AtCommandExecutorBuilder.context() input. It was redundant with .atSign() at its only call site (the from: challenge is shared caller-side: sendFrom and the imperative CRAM close over the same context object, not the builder) and silently overrode .atSign()/.keys()/.config(). The builder once again always owns its own context. - Collapsed CRAM to a single context-based authenticateWithCram(executor, context, secret); dropped the identity-explicit overload that no longer had a production caller. - Renamed the imperative-auth connection factories onto the from:-issuance axis: createConnectionSendingFrom(context) (issues from: on connect for the onboarding flow to reuse) and createConnectionDeferringFrom(...) (no from: on connect, for complete's retried, self-issued PKAM). - Documented the from:-first behaviour on the AtCommandExecutors builder doc. --- .../client/impl/AtCommandExecutors.java | 25 +++++------- .../atsign/client/impl/cli/AbstractCli.java | 40 +++++++++++-------- .../org/atsign/client/impl/cli/Activate.java | 22 +++++----- .../impl/commands/AuthenticationCommands.java | 36 +---------------- .../client/impl/commands/EnrollCommands.java | 10 ++--- .../commands/AuthenticationCommandsTest.java | 10 ++++- .../impl/commands/EnrollCommandsTest.java | 10 ++--- 7 files changed, 64 insertions(+), 89 deletions(-) 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 8c74c731..6a0ff091 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 @@ -43,8 +43,11 @@ * * NOTE: If the url is prefixed with proxy (e.g. proxy:host:port) then the builder * will automatically attempt to connect to an At Server at host:port. - * NOTE: If atSign and keys are provided then the builder - * will automatically configure the {@link AtCommandExecutor} to authenticate with PKAM. + * NOTE: If an atSign is provided then the builder issues {@code from:@atSign} as the first + * command once connected (so proxies / gateways can route the connection); if keys are also + * provided + * it then authenticates the {@link AtCommandExecutor} with PKAM, reusing that {@code from:}'s + * challenge. * NOTE: If reconnect is not set then the builder will default to a * {@link SimpleReconnectStrategy} */ @@ -57,7 +60,6 @@ public class AtCommandExecutors { public static AtCommandExecutor createCommandExecutor(String url, AtSign atSign, AtKeys keys, - AtCommandExecutorContext context, Consumer onReady, Map config, Long timeoutMillis, @@ -67,21 +69,16 @@ public static AtCommandExecutor createCommandExecutor(String url, Boolean isVerbose) throws AtException { - // the context is closed over by the onReady consumers the builder wires (see createOnReady); the - // command executor itself stays pure transport and knows nothing about it. By default the builder - // owns it, but a caller may supply one (via context()) so an imperative flow that authenticates - // on the connection can reach the same from: challenge — see EnrollCommands#onboard. - if (context == null) { - context = new AtCommandExecutorContext(atSign, keys, createClientConfig(config)); - } - AtSign endpointAtSign = context.getAtSign(); - if (AtEndpointSuppliers.isProxyUrl(url)) { - checkNotNull(endpointAtSign, "atSign not set"); + checkNotNull(atSign, "atSign not set"); } + // the context is closed over by the onReady consumers the builder wires (see createOnReady); the + // command executor itself stays pure transport and knows nothing about it + AtCommandExecutorContext context = new AtCommandExecutorContext(atSign, keys, createClientConfig(config)); + return NettyAtCommandExecutor.builder() - .endpoint(AtEndpointSuppliers.builder().url(url).atSign(endpointAtSign).build()) + .endpoint(AtEndpointSuppliers.builder().url(url).atSign(atSign).build()) .isVerbose(isVerbose) .timeoutMillis(defaultIfNotSet(timeoutMillis, DEFAULT_TIMEOUT_MILLIS)) .awaitReadyMillis(defaultIfNotSet(awaitReadyMillis, DEFAULT_TIMEOUT_MILLIS)) 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 8f320e48..bae19689 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 @@ -9,6 +9,7 @@ import org.atsign.client.api.AtSign; import org.atsign.client.impl.AtCommandExecutors; import org.atsign.client.impl.AtCommandExecutors.AtCommandExecutorBuilder; +import org.atsign.client.impl.commands.AuthenticationCommands; import org.atsign.client.impl.common.SimpleReconnectStrategy; import org.atsign.client.impl.exceptions.AtClientConfigException; import org.atsign.client.impl.exceptions.AtException; @@ -96,35 +97,40 @@ protected AtCommandExecutor createAuthenticatedConnection(String rootUrl, AtSign } /** - * A connection that carries the given {@code context}, so an imperative flow driving it (e.g. - * onboarding) can reach the {@code from:} challenge the connection issues on connect and reuse it - * for authentication. The builder wires from:@atSign (plus PKAM if the context has keys) around - * that same context instance. + * A connection that issues from:@atSign on connect (so proxies / gateways can route it), wiring the + * {@code from:} over the given {@code context} so the challenge it retains is the one an imperative + * flow driving the connection (e.g. onboarding: scan, then CRAM, then PKAM) later consumes. No + * on-ready authentication is wired — the caller authenticates imperatively. */ - protected AtCommandExecutor createConnection(AtCommandExecutorContext context, int retries) throws AtException { - return connectionBuilder(rootUrl, retries, verbose).context(context).build(); + protected AtCommandExecutor createConnectionSendingFrom(AtCommandExecutorContext context, int retries) + throws AtException { + return connectionBuilder(rootUrl, retries, verbose) + .atSign(context.getAtSign()) + .onReady(AuthenticationCommands.sendFrom(context)) + .build(); } /** - * A connection whose initial {@code from:} is issued by the caller's own first command rather than - * on connect — for flows that authenticate imperatively and must control the timing (e.g. a - * pending-retry loop), where a connect-time {@code from:} challenge could go stale before it is - * used. No onReady {@code from:} is wired, so the caller's first command (its authentication) - * establishes the atSign for proxies / gateways. + * A connection that does NOT issue {@code from:} on connect — its initial {@code from:} is issued + * by the caller's own first command — for flows that authenticate imperatively and must control the + * timing (e.g. a pending-retry loop), where a connect-time {@code from:} challenge could go stale + * before it is used. No onReady {@code from:} is wired, so the caller's first command (its + * authentication) establishes the atSign for proxies / gateways. */ - protected AtCommandExecutor createConnectionForImperativeAuth(String rootUrl, AtSign atSign, int retries) + protected AtCommandExecutor createConnectionDeferringFrom(String rootUrl, AtSign atSign, int retries) throws AtException { return connectionBuilder(rootUrl, retries, verbose).atSign(atSign).onReady(executor -> { }).build(); } /** - * Creates a keyless {@link AtCommandExecutorContext} for this CLI's atSign, to be passed to - * {@link #createConnection(AtCommandExecutorContext, int)} and threaded into an imperative flow - * that authenticates on the connection and reuses its {@code from:} challenge. + * Creates an {@link AtCommandExecutorContext} carrying this CLI's atSign and the given {@code keys} + * (the identity being onboarded), to be passed to + * {@link #createConnectionSendingFrom(AtCommandExecutorContext, int)} and threaded into the + * imperative onboarding flow, which reuses the connection's {@code from:} challenge. */ - protected AtCommandExecutorContext newConnectionContext() { - return new AtCommandExecutorContext(atSign, null, AtCommandExecutors.createClientConfig(null)); + protected AtCommandExecutorContext newConnectionContext(AtKeys keys) { + return new AtCommandExecutorContext(atSign, keys, AtCommandExecutors.createClientConfig(null)); } private static AtCommandExecutorBuilder connectionBuilder(String rootUrl, int retries, boolean verbose) { diff --git a/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java b/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java index bf75ef11..a0fec618 100644 --- a/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java +++ b/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java @@ -181,8 +181,8 @@ public Activate addNamespace(String namespace, String accessControl) { } public EnrollmentId onboard() throws Exception { - AtCommandExecutorContext context = newConnectionContext(); - try (AtCommandExecutor executor = createConnection(context, connectionRetries)) { + AtCommandExecutorContext context = newConnectionContext(generateAtKeys(true)); + try (AtCommandExecutor executor = createConnectionSendingFrom(context, connectionRetries)) { return onboard(executor, context); } } @@ -192,14 +192,12 @@ public EnrollmentId onboard(AtCommandExecutor executor, AtCommandExecutorContext if (!overwriteKeysFile) { checkNotExists(file); } - AtKeys keys = generateAtKeys(true); - keys = EnrollCommands.onboard(executor, - context, - keys, - cramSecret, - ensureNotNull(appName, DEFAULT_FIRST_APP), - ensureNotNull(deviceName, DEFAULT_FIRST_DEVICE), - deleteCramKey); + AtKeys keys = EnrollCommands.onboard(executor, + context, + cramSecret, + ensureNotNull(appName, DEFAULT_FIRST_APP), + ensureNotNull(deviceName, DEFAULT_FIRST_DEVICE), + deleteCramKey); saveKeys(keys, file); return enrollmentId; } @@ -296,7 +294,7 @@ public EnrollmentId enroll(AtCommandExecutor executor) throws Exception { public void complete() throws Exception { // complete authenticates imperatively (and retries on "pending"), so it issues its own from: // rather than reusing a connect-time challenge that could go stale before the retried PKAM - try (AtCommandExecutor executor = createConnectionForImperativeAuth(rootUrl, atSign, connectionRetries)) { + try (AtCommandExecutor executor = createConnectionDeferringFrom(rootUrl, atSign, connectionRetries)) { complete(executor); } } @@ -309,7 +307,7 @@ public void complete(AtCommandExecutor executor) throws Exception { public void complete(int retries, long sleepDuration, TimeUnit sleepUnit) throws Exception { // see complete(): imperative, retried PKAM issues its own from: - try (AtCommandExecutor executor = createConnectionForImperativeAuth(rootUrl, atSign, connectionRetries)) { + try (AtCommandExecutor executor = createConnectionDeferringFrom(rootUrl, atSign, connectionRetries)) { complete(executor, retries, sleepDuration, sleepUnit); } } 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 bcca8ea7..f4ac9b54 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 @@ -160,45 +160,13 @@ public static void authenticateWithCram(AtCommandExecutor executor, AtCommandExecutorContext context, String cramSecret) throws AtException { - authenticateWithCram(executor, context.getAtSign(), cramSecret, context.consumeChallenge()); - } - - /** - * Implements the protocol workflow / sequence for CRAM authentication. - * - * @param executor The executor with which to send the commands. - * @param atSign The asign to authenticate. - * @param cramSecret The cramSecret that was assigned during At Server provisioning. - * @throws AtException If authentication fails. - */ - public static void authenticateWithCram(AtCommandExecutor executor, AtSign atSign, String cramSecret) - throws AtException { - authenticateWithCram(executor, atSign, cramSecret, null); - } - - /** - * Implements the protocol workflow / sequence for CRAM authentication, reusing an already-issued - * {@code from:} challenge when one is supplied. - * - * @param executor The executor with which to send the commands. - * @param atSign The asign to authenticate. - * @param cramSecret The cramSecret that was assigned during At Server provisioning. - * @param reusableChallenge The challenge from an initial {@code from:} to reuse, or {@code null} to - * issue a fresh {@code from:}. - * @throws AtException If authentication fails. - */ - private static void authenticateWithCram(AtCommandExecutor executor, - AtSign atSign, - String cramSecret, - String reusableChallenge) - throws AtException { try { // reuse the challenge from the initial from: if one was issued on this connection, otherwise // send a from command and expect to receive a challenge - String challenge = reusableChallenge; + String challenge = context.consumeChallenge(); if (challenge == null) { - String fromCommand = CommandBuilders.fromCommandBuilder().atSign(atSign).build(); + String fromCommand = CommandBuilders.fromCommandBuilder().atSign(context.getAtSign()).build(); String fromResponse = executor.sendSync(fromCommand); challenge = matchDataStringNoWhitespace(throwExceptionIfError(fromResponse)); } diff --git a/at_client/src/main/java/org/atsign/client/impl/commands/EnrollCommands.java b/at_client/src/main/java/org/atsign/client/impl/commands/EnrollCommands.java index 0d51be47..963eaa88 100644 --- a/at_client/src/main/java/org/atsign/client/impl/commands/EnrollCommands.java +++ b/at_client/src/main/java/org/atsign/client/impl/commands/EnrollCommands.java @@ -33,10 +33,10 @@ public class EnrollCommands { * app/device keys that can approve subsequent enrollments. * * @param executor The {@link AtCommandExecutor} to use. - * @param context The connection context for the executor; supplies the atSign and the {@code from:} - * challenge that CRAM reuses (the connection issues {@code from:} on connect, before the - * connectivity scan below). - * @param keys The {@link AtKeys} for the atSign, these should already be populated. + * @param context The connection context for the executor; the single source of identity for this + * onboarding — its atSign, the {@link AtKeys} being onboarded (already populated), and the + * {@code from:} challenge that CRAM reuses (the connection issues {@code from:} on connect, + * before the connectivity scan below). * @param cramSecret The CRAM secret. * @param appName The app name for this first enrollment. * @param deviceName The device name for this first enrollment. @@ -47,13 +47,13 @@ public class EnrollCommands { */ public static AtKeys onboard(AtCommandExecutor executor, AtCommandExecutorContext context, - AtKeys keys, String cramSecret, String appName, String deviceName, boolean deleteCramKey) throws AtException { AtSign atSign = context.getAtSign(); + AtKeys keys = context.getKeys(); try { // verify that the executor is connected to the atsigns atserver diff --git a/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java b/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java index fb7e03b9..8ee465f7 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java @@ -29,7 +29,9 @@ public void testAuthenticateWithCramDoesNotThrowException() throws Exception { .stub("cram:7e91508d5.+", "data:success") .build(); - AuthenticationCommands.authenticateWithCram(executor, createAtSign("@alice"), "secret"); + AuthenticationCommands.authenticateWithCram(executor, + new AtCommandExecutorContext(createAtSign("@alice"), null, null), + "secret"); } @Test @@ -40,7 +42,11 @@ public void testAuthenticateWithCramFailThrowsExpectedException() throws Excepti .build(); Exception ex = assertThrows(AtUnauthenticatedException.class, - () -> AuthenticationCommands.authenticateWithCram(executor, createAtSign("@alice"), + () -> AuthenticationCommands.authenticateWithCram( + executor, + new AtCommandExecutorContext( + createAtSign("@alice"), null, + null), "secret")); assertThat(ex.getMessage(), containsString("deliberate")); } diff --git a/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java b/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java index ff3c5e1a..667fb499 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java @@ -67,8 +67,8 @@ public void testOnboardThrowsExceptionIfSigningPublicKeyIsMissing() throws Excep .build(); Exception ex = assertThrows(Exception.class, - () -> EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, null, null), - keys, "secret", "app", "device", false)); + () -> EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, keys, null), + "secret", "app", "device", false)); assertThat(ex.getMessage(), containsString("not connected to the atsign's at server")); } @@ -107,7 +107,7 @@ public void testOnboard() throws Exception { .stub("update:public:publickey@alice .+", "data:1") .build(); - AtKeys newKeys = EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, null, null), keys, "secret", + AtKeys newKeys = EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, keys, null), "secret", "app", "device", false); assertThat(newKeys, is(not(sameInstance(keys)))); @@ -123,7 +123,7 @@ public void testOnboardReusesTheConnectionFromChallengeForCram() throws Exceptio .build(); // simulate the connection having issued from: on connect (sendFrom) and retained the challenge - AtCommandExecutorContext context = new AtCommandExecutorContext(atSign, null, null); + AtCommandExecutorContext context = new AtCommandExecutorContext(atSign, keys, null); context.setChallenge("challenge"); AtCommandExecutor executor = TestExecutorBuilder.builder() @@ -135,7 +135,7 @@ public void testOnboardReusesTheConnectionFromChallengeForCram() throws Exceptio .stub("update:public:publickey@alice .+", "data:1") .build(); - EnrollCommands.onboard(executor, context, keys, "secret", "app", "device", false); + EnrollCommands.onboard(executor, context, "secret", "app", "device", false); // CRAM reused the retained challenge, so exactly ONE from: reaches the wire — PKAM's own (the // challenge is single-use and PKAM authenticates with the freshly-enrolled keys) From 1fca71291eebcad2953ac2aa9902613bed5480a1 Mon Sep 17 00:00:00 2001 From: gkc Date: Wed, 22 Jul 2026 09:56:38 +0100 Subject: [PATCH 6/8] chore: weird formatting, my IDE went abit mad --- .../client/api/AtCommandExecutorContext.java | 19 +++---- .../client/impl/AtCommandExecutors.java | 30 ++++++----- .../commands/AuthenticationCommandsTest.java | 46 ++++++++--------- .../impl/commands/EnrollCommandsTest.java | 50 +++++++++---------- 4 files changed, 68 insertions(+), 77 deletions(-) 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 index 6b62419b..3db7cfa9 100644 --- a/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java +++ b/at_client/src/main/java/org/atsign/client/api/AtCommandExecutorContext.java @@ -1,28 +1,21 @@ package org.atsign.client.api; +import lombok.*; + import java.util.Map; import java.util.concurrent.atomic.AtomicReference; -import lombok.AccessLevel; -import lombok.EqualsAndHashCode; -import lombok.Getter; -import lombok.ToString; -import lombok.Value; - /** - * The identity a connection authenticates as — its {@code atSign}, {@code keys} and {@code config} - * — + * The identity a connection authenticates as (its {@code atSign}, {@code keys} and {@code config}) * together with the single-use challenge from the {@code from:} that is issued as the first command * once the connection is ready. * *

* The context is created by the builder (see * {@code AtCommandExecutors#createCommandExecutor}) and closed over by the {@code onReady} - * consumers - * it wires, so the {@code from:} sender can retain the challenge and the authentication that - * follows - * on the same connection can reuse it rather than issuing a second {@code from:}. The command - * executor itself is pure transport and knows nothing about this context. + * consumers it wires, so the {@code from:} sender can retain the challenge and the authentication + * that follows on the same connection can reuse it rather than issuing a second {@code from:}. + * The command executor itself is pure transport and knows nothing about this context. * *

* The identity fields are fixed for the life of the context. The challenge is per-connection state: 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 6a0ff091..70773ada 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 @@ -1,17 +1,8 @@ package org.atsign.client.impl; -import static org.atsign.client.impl.common.Preconditions.checkNotNull; - -import java.io.InputStream; -import java.net.URL; -import java.util.HashMap; -import java.util.Map; -import java.util.Properties; -import java.util.UUID; -import java.util.concurrent.TimeUnit; -import java.util.function.Consumer; - +import lombok.Builder; +import lombok.extern.slf4j.Slf4j; import org.atsign.client.api.AtCommandExecutor; import org.atsign.client.api.AtCommandExecutorContext; import org.atsign.client.api.AtKeys; @@ -22,8 +13,16 @@ import org.atsign.client.impl.exceptions.AtException; import org.atsign.client.impl.netty.NettyAtCommandExecutor; -import lombok.Builder; -import lombok.extern.slf4j.Slf4j; +import java.io.InputStream; +import java.net.URL; +import java.util.HashMap; +import java.util.Map; +import java.util.Properties; +import java.util.UUID; +import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; + +import static org.atsign.client.impl.common.Preconditions.checkNotNull; /** * Utility methods / builders for instantiating {@link AtCommandExecutor} implementations @@ -45,9 +44,8 @@ * will automatically attempt to connect to an At Server at host:port. * NOTE: If an atSign is provided then the builder issues {@code from:@atSign} as the first * command once connected (so proxies / gateways can route the connection); if keys are also - * provided - * it then authenticates the {@link AtCommandExecutor} with PKAM, reusing that {@code from:}'s - * challenge. + * provided it then authenticates the {@link AtCommandExecutor} with PKAM, reusing that + * {@code from:}'s challenge. * NOTE: If reconnect is not set then the builder will default to a * {@link SimpleReconnectStrategy} */ diff --git a/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java b/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java index 8ee465f7..054e1676 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java @@ -1,18 +1,8 @@ package org.atsign.client.impl.commands; -import static org.atsign.client.impl.util.EncryptionUtils.generateRSAKeyPair; -import static org.atsign.client.impl.common.EnrollmentId.createEnrollmentId; -import static org.atsign.client.api.AtSign.createAtSign; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.*; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.mockito.ArgumentMatchers.matches; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; - +import org.atsign.client.api.AtCommandExecutor; import org.atsign.client.api.AtCommandExecutorContext; import org.atsign.client.api.AtKeys; -import org.atsign.client.api.AtCommandExecutor; import org.atsign.client.impl.exceptions.AtOnReadyException; import org.atsign.client.impl.exceptions.AtUnauthenticatedException; import org.junit.jupiter.api.Test; @@ -20,6 +10,16 @@ import java.util.Collections; import java.util.Map; +import static org.atsign.client.api.AtSign.createAtSign; +import static org.atsign.client.impl.common.EnrollmentId.createEnrollmentId; +import static org.atsign.client.impl.util.EncryptionUtils.generateRSAKeyPair; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.matches; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + public class AuthenticationCommandsTest { @Test @@ -30,8 +30,8 @@ public void testAuthenticateWithCramDoesNotThrowException() throws Exception { .build(); AuthenticationCommands.authenticateWithCram(executor, - new AtCommandExecutorContext(createAtSign("@alice"), null, null), - "secret"); + new AtCommandExecutorContext(createAtSign("@alice"), null, null), + "secret"); } @Test @@ -42,12 +42,12 @@ public void testAuthenticateWithCramFailThrowsExpectedException() throws Excepti .build(); Exception ex = assertThrows(AtUnauthenticatedException.class, - () -> AuthenticationCommands.authenticateWithCram( - executor, - new AtCommandExecutorContext( - createAtSign("@alice"), null, - null), - "secret")); + () -> AuthenticationCommands.authenticateWithCram( + executor, + new AtCommandExecutorContext( + createAtSign("@alice"), null, + null), + "secret")); assertThat(ex.getMessage(), containsString("deliberate")); } @@ -94,8 +94,8 @@ public void testAuthenticateWithApkamFailThrowsExpectedException() throws Except .build(); Exception ex = assertThrows(AtUnauthenticatedException.class, - () -> AuthenticationCommands.authenticateWithPkam(executor, createAtSign("@alice"), - keys)); + () -> AuthenticationCommands.authenticateWithPkam(executor, createAtSign("@alice"), + keys)); assertThat(ex.getMessage(), containsString("deliberate")); } @@ -108,8 +108,8 @@ public void testPkamAuthenticatorThrowsOnReadyException() throws Exception { .build(); Exception ex = assertThrows(Exception.class, - () -> AuthenticationCommands.pkamAuthenticator(createAtSign("@alice"), keys, null) - .accept(executor)); + () -> AuthenticationCommands.pkamAuthenticator(createAtSign("@alice"), keys, null) + .accept(executor)); assertThat(ex, instanceOf(AtOnReadyException.class)); assertThat(ex.getMessage(), containsString("deliberate")); } diff --git a/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java b/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java index 667fb499..0e73eb61 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java @@ -1,10 +1,21 @@ package org.atsign.client.impl.commands; +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.common.EnrollmentId; +import org.atsign.client.impl.exceptions.AtException; +import org.atsign.client.impl.exceptions.AtServerRuntimeException; +import org.junit.jupiter.api.Test; + +import java.util.List; + import static java.lang.String.format; import static java.util.Collections.singletonMap; -import static org.atsign.client.impl.util.EncryptionUtils.*; -import static org.atsign.client.impl.common.EnrollmentId.createEnrollmentId; import static org.atsign.client.api.AtSign.createAtSign; +import static org.atsign.client.impl.common.EnrollmentId.createEnrollmentId; +import static org.atsign.client.impl.util.EncryptionUtils.*; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -12,17 +23,6 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import java.util.List; - -import org.atsign.client.api.AtKeys; -import org.atsign.client.api.AtCommandExecutor; -import org.atsign.client.api.AtCommandExecutorContext; -import org.atsign.client.impl.common.EnrollmentId; -import org.atsign.client.impl.exceptions.AtException; -import org.atsign.client.api.AtSign; -import org.atsign.client.impl.exceptions.AtServerRuntimeException; -import org.junit.jupiter.api.Test; - public class EnrollCommandsTest { public static final String RSA_KEY = "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAi0ZpMHagomCnC6MX" + @@ -67,8 +67,8 @@ public void testOnboardThrowsExceptionIfSigningPublicKeyIsMissing() throws Excep .build(); Exception ex = assertThrows(Exception.class, - () -> EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, keys, null), - "secret", "app", "device", false)); + () -> EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, keys, null), + "secret", "app", "device", false)); assertThat(ex.getMessage(), containsString("not connected to the atsign's at server")); } @@ -108,7 +108,7 @@ public void testOnboard() throws Exception { .build(); AtKeys newKeys = EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, keys, null), "secret", - "app", "device", false); + "app", "device", false); assertThat(newKeys, is(not(sameInstance(keys)))); assertThat(newKeys.getEnrollmentId(), equalTo(createEnrollmentId("904dcbf7"))); @@ -175,9 +175,9 @@ public void testComplete() throws Exception { .build(); String selfEncryptKeysGetResponse = format("data:{\"value\":\"%s\",\"iv\":\"%s\"}", - aesEncryptToBase64(AES_KEY, keys.getApkamSymmetricKey(), IV), IV); + aesEncryptToBase64(AES_KEY, keys.getApkamSymmetricKey(), IV), IV); String privateEncryptKeysGetResponse = format("data:{\"value\":\"%s\",\"iv\":\"%s\"}", - aesEncryptToBase64(RSA_KEY, keys.getApkamSymmetricKey(), IV), IV); + aesEncryptToBase64(RSA_KEY, keys.getApkamSymmetricKey(), IV), IV); AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("from:@alice", "data:challenge") @@ -201,13 +201,13 @@ public void testApprove() throws Exception { .build(); String fetchResponse = format("data:{\"appName\":\"app1\",\"deviceName\":\"device1\"," + - "\"namespace\":{\"ns\":\"rw\"},\"encryptedAPKAMSymmetricKey\":\"%s\",\"status\":\"pending\"}", - rsaEncryptToBase64(AES_KEY, keys.getEncryptPublicKey())); + "\"namespace\":{\"ns\":\"rw\"},\"encryptedAPKAMSymmetricKey\":\"%s\",\"status\":\"pending\"}", + rsaEncryptToBase64(AES_KEY, keys.getEncryptPublicKey())); AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:fetch\\{\"enrollmentId\":\"12345\"}", fetchResponse) .stub("enroll:approve\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"approved\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"approved\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.approve(executor, keys, createEnrollmentId("12345")); @@ -217,7 +217,7 @@ public void testApprove() throws Exception { public void testDeny() throws Exception { AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:deny\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"denied\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"denied\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.deny(executor, createEnrollmentId("12345")); @@ -227,7 +227,7 @@ public void testDeny() throws Exception { public void testRevoke() throws Exception { AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:revoke\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"revoked\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"revoked\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.revoke(executor, createEnrollmentId("12345")); @@ -237,7 +237,7 @@ public void testRevoke() throws Exception { public void testUnrevoke() throws Exception { AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:unrevoke\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"approved\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"approved\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.unrevoke(executor, createEnrollmentId("12345")); @@ -247,7 +247,7 @@ public void testUnrevoke() throws Exception { public void testDelete() throws Exception { AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:delete\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"deleted\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"deleted\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.delete(executor, createEnrollmentId("12345")); From dc9a26fbc69a1727de920c04a8ec1cac9b76732f Mon Sep 17 00:00:00 2001 From: gkc Date: Wed, 22 Jul 2026 10:00:14 +0100 Subject: [PATCH 7/8] chore: moar formatign --- .../commands/AuthenticationCommandsTest.java | 24 +++++++++---------- .../impl/commands/EnrollCommandsTest.java | 24 +++++++++---------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java b/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java index 054e1676..65c11cb4 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/AuthenticationCommandsTest.java @@ -30,8 +30,8 @@ public void testAuthenticateWithCramDoesNotThrowException() throws Exception { .build(); AuthenticationCommands.authenticateWithCram(executor, - new AtCommandExecutorContext(createAtSign("@alice"), null, null), - "secret"); + new AtCommandExecutorContext(createAtSign("@alice"), null, null), + "secret"); } @Test @@ -42,12 +42,12 @@ public void testAuthenticateWithCramFailThrowsExpectedException() throws Excepti .build(); Exception ex = assertThrows(AtUnauthenticatedException.class, - () -> AuthenticationCommands.authenticateWithCram( - executor, - new AtCommandExecutorContext( - createAtSign("@alice"), null, - null), - "secret")); + () -> AuthenticationCommands.authenticateWithCram( + executor, + new AtCommandExecutorContext( + createAtSign("@alice"), null, + null), + "secret")); assertThat(ex.getMessage(), containsString("deliberate")); } @@ -94,8 +94,8 @@ public void testAuthenticateWithApkamFailThrowsExpectedException() throws Except .build(); Exception ex = assertThrows(AtUnauthenticatedException.class, - () -> AuthenticationCommands.authenticateWithPkam(executor, createAtSign("@alice"), - keys)); + () -> AuthenticationCommands.authenticateWithPkam(executor, createAtSign("@alice"), + keys)); assertThat(ex.getMessage(), containsString("deliberate")); } @@ -108,8 +108,8 @@ public void testPkamAuthenticatorThrowsOnReadyException() throws Exception { .build(); Exception ex = assertThrows(Exception.class, - () -> AuthenticationCommands.pkamAuthenticator(createAtSign("@alice"), keys, null) - .accept(executor)); + () -> AuthenticationCommands.pkamAuthenticator(createAtSign("@alice"), keys, null) + .accept(executor)); assertThat(ex, instanceOf(AtOnReadyException.class)); assertThat(ex.getMessage(), containsString("deliberate")); } diff --git a/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java b/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java index 0e73eb61..f3f4a8d6 100644 --- a/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java +++ b/at_client/src/test/java/org/atsign/client/impl/commands/EnrollCommandsTest.java @@ -67,8 +67,8 @@ public void testOnboardThrowsExceptionIfSigningPublicKeyIsMissing() throws Excep .build(); Exception ex = assertThrows(Exception.class, - () -> EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, keys, null), - "secret", "app", "device", false)); + () -> EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, keys, null), + "secret", "app", "device", false)); assertThat(ex.getMessage(), containsString("not connected to the atsign's at server")); } @@ -108,7 +108,7 @@ public void testOnboard() throws Exception { .build(); AtKeys newKeys = EnrollCommands.onboard(executor, new AtCommandExecutorContext(atSign, keys, null), "secret", - "app", "device", false); + "app", "device", false); assertThat(newKeys, is(not(sameInstance(keys)))); assertThat(newKeys.getEnrollmentId(), equalTo(createEnrollmentId("904dcbf7"))); @@ -175,9 +175,9 @@ public void testComplete() throws Exception { .build(); String selfEncryptKeysGetResponse = format("data:{\"value\":\"%s\",\"iv\":\"%s\"}", - aesEncryptToBase64(AES_KEY, keys.getApkamSymmetricKey(), IV), IV); + aesEncryptToBase64(AES_KEY, keys.getApkamSymmetricKey(), IV), IV); String privateEncryptKeysGetResponse = format("data:{\"value\":\"%s\",\"iv\":\"%s\"}", - aesEncryptToBase64(RSA_KEY, keys.getApkamSymmetricKey(), IV), IV); + aesEncryptToBase64(RSA_KEY, keys.getApkamSymmetricKey(), IV), IV); AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("from:@alice", "data:challenge") @@ -201,13 +201,13 @@ public void testApprove() throws Exception { .build(); String fetchResponse = format("data:{\"appName\":\"app1\",\"deviceName\":\"device1\"," + - "\"namespace\":{\"ns\":\"rw\"},\"encryptedAPKAMSymmetricKey\":\"%s\",\"status\":\"pending\"}", - rsaEncryptToBase64(AES_KEY, keys.getEncryptPublicKey())); + "\"namespace\":{\"ns\":\"rw\"},\"encryptedAPKAMSymmetricKey\":\"%s\",\"status\":\"pending\"}", + rsaEncryptToBase64(AES_KEY, keys.getEncryptPublicKey())); AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:fetch\\{\"enrollmentId\":\"12345\"}", fetchResponse) .stub("enroll:approve\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"approved\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"approved\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.approve(executor, keys, createEnrollmentId("12345")); @@ -217,7 +217,7 @@ public void testApprove() throws Exception { public void testDeny() throws Exception { AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:deny\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"denied\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"denied\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.deny(executor, createEnrollmentId("12345")); @@ -227,7 +227,7 @@ public void testDeny() throws Exception { public void testRevoke() throws Exception { AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:revoke\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"revoked\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"revoked\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.revoke(executor, createEnrollmentId("12345")); @@ -237,7 +237,7 @@ public void testRevoke() throws Exception { public void testUnrevoke() throws Exception { AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:unrevoke\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"approved\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"approved\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.unrevoke(executor, createEnrollmentId("12345")); @@ -247,7 +247,7 @@ public void testUnrevoke() throws Exception { public void testDelete() throws Exception { AtCommandExecutor executor = TestExecutorBuilder.builder() .stub("enroll:delete\\{\"enrollmentId\":\"12345\".+", - "data:{\"status\":\"deleted\",\"enrollmentId\":\"12345\"}") + "data:{\"status\":\"deleted\",\"enrollmentId\":\"12345\"}") .build(); EnrollCommands.delete(executor, createEnrollmentId("12345")); From fd01dadc0e309e8f49371a8103d383975433945b Mon Sep 17 00:00:00 2001 From: gkc Date: Wed, 22 Jul 2026 10:13:58 +0100 Subject: [PATCH 8/8] fix: fixed challenge staleness bug which I introduced by this latest round of changes. (Was handled directly within the AtCommandExecutor when that was a thing) --- .../atsign/client/impl/cli/AbstractCli.java | 18 ++++++++++++++---- .../org/atsign/client/impl/cli/Activate.java | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-) 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 bae19689..73a85e85 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 @@ -10,6 +10,7 @@ import org.atsign.client.impl.AtCommandExecutors; import org.atsign.client.impl.AtCommandExecutors.AtCommandExecutorBuilder; import org.atsign.client.impl.commands.AuthenticationCommands; +import org.atsign.client.impl.common.ReconnectStrategy; import org.atsign.client.impl.common.SimpleReconnectStrategy; import org.atsign.client.impl.exceptions.AtClientConfigException; import org.atsign.client.impl.exceptions.AtException; @@ -101,12 +102,21 @@ protected AtCommandExecutor createAuthenticatedConnection(String rootUrl, AtSign * {@code from:} over the given {@code context} so the challenge it retains is the one an imperative * flow driving the connection (e.g. onboarding: scan, then CRAM, then PKAM) later consumes. No * on-ready authentication is wired — the caller authenticates imperatively. + * + *

+ * Reconnect is disabled: because the imperative auth reuses the connect-time {@code from:} + * challenge, a mid-flow drop must abort (the one-shot flow is then rerun) rather than reconnect and + * authenticate on the new connection with a challenge the previous connection issued — the new + * connection's server would reject it. (The challenge is not cleared on close, so reconnect-and- + * reuse would be stale.) */ - protected AtCommandExecutor createConnectionSendingFrom(AtCommandExecutorContext context, int retries) - throws AtException { - return connectionBuilder(rootUrl, retries, verbose) + protected AtCommandExecutor createConnectionSendingFrom(AtCommandExecutorContext context) throws AtException { + return AtCommandExecutors.builder() + .url(rootUrl) .atSign(context.getAtSign()) .onReady(AuthenticationCommands.sendFrom(context)) + .reconnect(ReconnectStrategy.NONE) + .isVerbose(verbose) .build(); } @@ -126,7 +136,7 @@ protected AtCommandExecutor createConnectionDeferringFrom(String rootUrl, AtSign /** * Creates an {@link AtCommandExecutorContext} carrying this CLI's atSign and the given {@code keys} * (the identity being onboarded), to be passed to - * {@link #createConnectionSendingFrom(AtCommandExecutorContext, int)} and threaded into the + * {@link #createConnectionSendingFrom(AtCommandExecutorContext)} and threaded into the * imperative onboarding flow, which reuses the connection's {@code from:} challenge. */ protected AtCommandExecutorContext newConnectionContext(AtKeys keys) { diff --git a/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java b/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java index a0fec618..e7685bd7 100644 --- a/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java +++ b/at_client/src/main/java/org/atsign/client/impl/cli/Activate.java @@ -182,7 +182,7 @@ public Activate addNamespace(String namespace, String accessControl) { public EnrollmentId onboard() throws Exception { AtCommandExecutorContext context = newConnectionContext(generateAtKeys(true)); - try (AtCommandExecutor executor = createConnectionSendingFrom(context, connectionRetries)) { + try (AtCommandExecutor executor = createConnectionSendingFrom(context)) { return onboard(executor, context); } }