This is an automated email from the ASF dual-hosted git repository. quantranhong1999 pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 7711ad16f8d44b8bbce59155a6cc6ad58ea362c2 Author: Quan Tran <[email protected]> AuthorDate: Tue Jul 28 09:44:30 2026 +0700 JAMES-4210 SMTP: should close SASL exchange upon client disconnection --- .../protocols/smtp/core/esmtp/AuthCmdHandler.java | 46 ++++++++-- .../org/apache/james/smtpserver/SMTPSaslTest.java | 99 ++++++++++++++++++++++ 2 files changed, 136 insertions(+), 9 deletions(-) diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java index eb649ae12f..8246e8d506 100644 --- a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java +++ b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/AuthCmdHandler.java @@ -29,9 +29,11 @@ import java.util.Locale; import java.util.Optional; import org.apache.james.core.Username; +import org.apache.james.protocols.api.ProtocolSession; import org.apache.james.protocols.api.Request; import org.apache.james.protocols.api.Response; import org.apache.james.protocols.api.handler.CommandHandler; +import org.apache.james.protocols.api.handler.DisconnectHandler; import org.apache.james.protocols.api.handler.ExtensibleHandler; import org.apache.james.protocols.api.handler.LineHandler; import org.apache.james.protocols.api.handler.WiringException; @@ -67,12 +69,14 @@ import com.google.common.collect.ImmutableSet; * Authentication is delegated to configured SASL mechanisms. */ public class AuthCmdHandler - implements CommandHandler<SMTPSession>, EhloExtension, ExtensibleHandler, MailParametersHook { + implements CommandHandler<SMTPSession>, DisconnectHandler<SMTPSession>, EhloExtension, ExtensibleHandler, MailParametersHook { private static final Collection<String> COMMANDS = ImmutableSet.of("AUTH"); private static final Logger LOGGER = LoggerFactory.getLogger(AuthCmdHandler.class); private static final Logger AUTHENTICATION_DEDICATED_LOGGER = LoggerFactory.getLogger("org.apache.james.protocols.smtp.AUTHENTICATION"); private static final String[] MAIL_PARAMS = { "AUTH" }; private static final String AUTH_TYPES_DELIMITER = " "; + private static final ProtocolSession.AttachmentKey<SaslExchange> ACTIVE_SASL_EXCHANGE = + ProtocolSession.AttachmentKey.of("SMTP_ACTIVE_SASL_EXCHANGE", SaslExchange.class); private static final Response AUTH_ABORTED = new SMTPResponse(SMTPRetCode.SYNTAX_ERROR_ARGUMENTS, DSNStatus.getStatus(DSNStatus.PERMANENT, DSNStatus.SECURITY_AUTH) + " Authentication aborted").immutable(); private static final Response ALREADY_AUTH = new SMTPResponse(SMTPRetCode.BAD_SEQUENCE, DSNStatus.getStatus(DSNStatus.PERMANENT,DSNStatus.DELIVERY_OTHER) + " User has previously authenticated. " @@ -136,6 +140,7 @@ public class AuthCmdHandler try { SaslExchange exchange = startExchange(maybeMechanism.get(), SaslCodec.initialRequest(authType, initialResponse)); + registerExchange(session, exchange); return handleFirstSaslStep(session, authType, exchange); } catch (IllegalArgumentException e) { LOGGER.info("Could not decode parameters for AUTH {}", authType, e); @@ -153,7 +158,7 @@ public class AuthCmdHandler return handleTerminalSaslStep(session, authType, exchange, step, () -> { }); } catch (RuntimeException e) { - exchange.close(); + closeExchange(session); throw e; } } @@ -182,11 +187,11 @@ public class AuthCmdHandler } catch (IllegalArgumentException e) { LOGGER.info("Could not decode parameters for AUTH {}", authType, e); session.popLineHandler(); - exchange.close(); + closeExchange(session); return new SMTPResponse(SMTPRetCode.SYNTAX_ERROR_ARGUMENTS, "Could not decode parameters for AUTH " + authType); } catch (RuntimeException e) { session.popLineHandler(); - exchange.close(); + closeExchange(session); throw e; } } @@ -195,7 +200,7 @@ public class AuthCmdHandler return (session, line) -> { if (SaslCodec.isAbort(line)) { session.popLineHandler(); - exchange.abort(); + abortExchange(session); return AUTH_ABORTED; } return handleSaslContinuation(session, authType, exchange, new String(line, session.getCharset())); @@ -213,7 +218,7 @@ public class AuthCmdHandler try { return applySaslSuccess(session, authType, exchange, success); } finally { - exchange.close(); + closeExchange(session); } } @@ -229,7 +234,7 @@ public class AuthCmdHandler byte[] bytes = line.getBytes(session.getCharset()); if (SaslCodec.isAbort(bytes)) { aborted = true; - exchange.abort(); + abortExchange(session); return AUTH_ABORTED; } if (!SaslCodec.isEmptyClientResponse(bytes)) { @@ -238,7 +243,7 @@ public class AuthCmdHandler return applySaslSuccess(session, authType, exchange, success); } finally { if (!aborted) { - exchange.close(); + closeExchange(session); } } } @@ -283,7 +288,30 @@ public class AuthCmdHandler case INVALID_CREDENTIALS, AUTHENTICATION_FAILED, USER_DOES_NOT_EXIST, DELEGATION_FORBIDDEN -> AUTH_FAILED; }); } finally { - exchange.close(); + closeExchange(session); + } + } + + private void registerExchange(SMTPSession session, SaslExchange exchange) { + session.setAttachment(ACTIVE_SASL_EXCHANGE, exchange, ProtocolSession.State.Connection) + .ifPresent(SaslExchange::close); + } + + private void closeExchange(SMTPSession session) { + session.removeAttachment(ACTIVE_SASL_EXCHANGE, ProtocolSession.State.Connection) + .ifPresent(SaslExchange::close); + } + + private void abortExchange(SMTPSession session) { + session.removeAttachment(ACTIVE_SASL_EXCHANGE, ProtocolSession.State.Connection) + .ifPresent(SaslExchange::abort); + } + + @Override + public void onDisconnect(SMTPSession session) { + if (session != null) { + session.removeAttachment(ACTIVE_SASL_EXCHANGE, ProtocolSession.State.Connection) + .ifPresent(SaslExchange::close); } } diff --git a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java index 9a71859f95..5d67c858c1 100644 --- a/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java +++ b/server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/SMTPSaslTest.java @@ -26,8 +26,10 @@ import static org.assertj.core.api.Assertions.assertThat; import java.io.IOException; import java.net.InetSocketAddress; import java.nio.charset.StandardCharsets; +import java.time.Duration; import java.util.Base64; import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.commons.configuration2.BaseHierarchicalConfiguration; import org.apache.commons.configuration2.HierarchicalConfiguration; @@ -55,6 +57,7 @@ import org.apache.james.protocols.sasl.plain.PlainSaslMechanism; import org.apache.james.protocols.smtp.core.esmtp.LoginSaslMechanismFactory; import org.apache.james.util.ClassLoaderUtils; import org.assertj.core.api.SoftAssertions; +import org.awaitility.Awaitility; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -620,6 +623,57 @@ class SMTPSaslTest { assertThat(client.getReplyString()).contains("235 Authentication Successful"); } + @Test + void disconnectDuringSaslContinuationShouldCloseExchangeOnce() throws Exception { + AtomicInteger closeCount = new AtomicInteger(); + resetWithMechanisms(ImmutableList.of(new RecordingSaslMechanism(closeCount))); + SMTPClient client = connectedClient(); + + client.sendCommand("AUTH RECORDING"); + assertThat(client.getReplyString()).contains("334 " + base64("challenge")); + client.disconnect(); + + // Disconnect cleanup closes the pending SASL exchange exactly once. + Awaitility.await().atMost(Duration.ofSeconds(2)) + .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1)); + } + + @Test + void abortDuringSaslContinuationShouldAbortExchangeOnce() throws Exception { + AtomicInteger closeCount = new AtomicInteger(); + AtomicInteger abortCount = new AtomicInteger(); + resetWithMechanisms(ImmutableList.of(new RecordingSaslMechanism(closeCount, abortCount))); + SMTPClient client = connectedClient(); + + client.sendCommand("AUTH RECORDING"); + assertThat(client.getReplyString()).contains("334 " + base64("challenge")); + client.sendCommand("*"); + assertThat(client.getReplyString()).contains("501 5.7.1 Authentication aborted"); + client.disconnect(); + + Awaitility.await().during(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2)) + .untilAsserted(() -> { + assertThat(abortCount.get()).isEqualTo(1); + assertThat(closeCount.get()).isZero(); + }); + } + + @Test + void disconnectAfterTerminalSaslStepShouldNotCloseExchangeAgain() throws Exception { + AtomicInteger closeCount = new AtomicInteger(); + resetWithMechanisms(ImmutableList.of(new RecordingSaslMechanism(closeCount))); + SMTPClient client = connectedClient(); + + client.sendCommand("AUTH RECORDING"); + assertThat(client.getReplyString()).contains("334 " + base64("challenge")); + client.sendCommand(base64("response")); + assertThat(client.getReplyString()).contains("535 Authentication Failed"); + client.disconnect(); + + Awaitility.await().during(Duration.ofMillis(100)).atMost(Duration.ofSeconds(2)) + .untilAsserted(() -> assertThat(closeCount.get()).isEqualTo(1)); + } + @Test void mechanismUnavailableOnClearTransportShouldNotBeAdvertisedAndShouldBeRejected() throws Exception { PlainSaslMechanism plain = new PlainSaslMechanism(true, true); @@ -724,4 +778,49 @@ class SMTPSaslTest { }; } } + + private static class RecordingSaslMechanism implements SaslMechanism { + private final AtomicInteger closeCount; + private final AtomicInteger abortCount; + + private RecordingSaslMechanism(AtomicInteger closeCount) { + this(closeCount, new AtomicInteger()); + } + + private RecordingSaslMechanism(AtomicInteger closeCount, AtomicInteger abortCount) { + this.closeCount = closeCount; + this.abortCount = abortCount; + } + + @Override + public String name() { + return "RECORDING"; + } + + @Override + public SaslExchange start(SaslInitialRequest request, SaslAuthenticator authenticator) { + return new SaslExchange() { + @Override + public SaslStep firstStep() { + return new SaslStep.Challenge(Optional.of("challenge".getBytes(UTF_8))); + } + + @Override + public SaslStep onResponse(byte[] clientResponse) { + return new SaslStep.Failure(SaslFailure.authenticationFailed( + Optional.empty(), Optional.empty(), "Test-only mechanism")); + } + + @Override + public void abort() { + abortCount.incrementAndGet(); + } + + @Override + public void close() { + closeCount.incrementAndGet(); + } + }; + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
