JAMES-4210 SASL modurlarization adoption for SMTP#3072
Open
quantranhong1999 wants to merge 10 commits into
Open
JAMES-4210 SASL modurlarization adoption for SMTP#3072quantranhong1999 wants to merge 10 commits into
quantranhong1999 wants to merge 10 commits into
Conversation
Introduce an SMTP SASL bridge for decoding initial responses and continuation lines, encoding challenges as SMTP 334 responses, detecting aborts, and supporting final SASL server data through the RFC 4422 additional-challenge flow.
Assert the base64 OAuth error challenge and dummy client response required by RFC 7628 before terminal IMAP authentication failure, aligning IMAP behavior with the existing SMTP OIDC error flow.
Add shared SASL behavior needed by SMTP: transport availability on factories and mechanisms, OAuth invalid-token challenge flow, server-error convenience failure, and legacy PLAIN trailing-NUL compatibility.
Replace embedded SMTP PLAIN/OIDC authentication logic with the shared SaslMechanism exchange flow, including LOGIN framing, final server data acknowledgement, SMTP session authentication state, and SASL-focused regression tests.
Keep legacy SMTP AuthHook registrations usable by wrapping them as a PLAIN SASL mechanism, add post-auth result hook support, deprecate AuthHook, and document migration expectations.
Wire SMTP Guice support for per-server SASL mechanism resolution, default SMTP SASL factories, probe cleanup, and module tests.
Adapt non-Guice and Spring SMTP server creation to provide default SASL mechanisms and authenticator wiring. Add a UsersRepository-backed authenticator for the scaling Pulsar SMTP app.
Remove LMTP test and server implementations of SMTP authentication configuration methods that no longer exist after SMTP AUTH moved to SASL mechanisms.
Stop the custom SMTP command example from explicitly registering UsersRepositoryAuthHook; default SMTP authentication is now handled by the SASL AUTH command path.
chibenwa
reviewed
Jun 24, 2026
chibenwa
left a comment
chibenwa
left a comment
Contributor
There was a problem hiding this comment.
Minor remarks, this looks overall good to me.
| private final String name; | ||
| private final OidcJwtTokenVerifier verifier; | ||
| private final boolean requiresSsl; | ||
| private final byte[] invalidTokenResponse; |
Contributor
There was a problem hiding this comment.
Can't we supply a default / static value ?
I'd try if possible to get a similar invalidToken response in SMTP and IMAP in order to allign IMAP with SMTP
| try { | ||
| SaslInitialRequest request = SASL_BRIDGE.initialRequest(authType, initialResponse); | ||
| exchange = startExchange(session, mechanism, request); | ||
| return handleFirstSaslStep(session, authType, exchange); |
Contributor
There was a problem hiding this comment.
try {
SaslInitialRequest request = ;
188 +
SaslExchange exchange = startExchange(session, maybeMechanism.get(), SASL_BRIDGE.initialRequest(authType, initialResponse));
189 +
return handleFirstSaslStep(session, authType, exchange)
Comment on lines
+220
to
+230
| return switch (step) { | ||
| case SaslStep.Challenge challenge -> SASL_BRIDGE.challenge(challenge); | ||
| case SaslStep.Success success -> { | ||
| session.popLineHandler(); | ||
| yield handleSaslSuccess(session, authType, exchange, success); | ||
| } | ||
| case SaslStep.Failure failure -> { | ||
| session.popLineHandler(); | ||
| yield handleSaslFailure(session, authType, exchange, failure.failure()); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Extract this block, mutualize it use a Runnable to either () -> {} or session::popLineHandler
| if (line != null) { | ||
| String lineWithoutTrailingCrLf = StringUtils.replace(line, "\r\n", ""); | ||
| return new String(Base64.getDecoder().decode(lineWithoutTrailingCrLf), StandardCharsets.UTF_8); | ||
| private Response handleLoginFraming(SMTPSession session, Optional<String> initialResponse) { |
Contributor
There was a problem hiding this comment.
Maybe it would be easier to handle login as a dedicated Sasl mechanism registered only for SMTP
Then we won't have this specific handling here.
Comment on lines
+412
to
+413
| private SaslExchange startExchange(SMTPSession session, SaslMechanism mechanism, SaslInitialRequest request) { | ||
| if (mechanism instanceof AuthHookSaslMechanism authHookSaslMechanism) { |
Contributor
There was a problem hiding this comment.
Why sasl mechanism specific internals leaks in here ?
| import org.apache.james.user.api.UsersRepository; | ||
| import org.apache.james.user.api.UsersRepositoryException; | ||
|
|
||
| class UsersRepositoryBackedAuthenticator implements Authenticator { |
Comment on lines
+60
to
+64
| @PreDestroy | ||
| void destroy() { | ||
| // SMTPServerFactory is provided through a factory method; dispose it explicitly on Guice shutdown. | ||
| smtpServerFactory.destroy(); | ||
| } |
Comment on lines
+58
to
+68
| public interface SmtpSaslMechanismLoader { | ||
| static SmtpSaslMechanismLoader defaultLoader() { | ||
| ImmutableList<SaslMechanismFactory> defaultFactories = ImmutableList.of( | ||
| new PlainSaslMechanismFactory(AuthAnnouncementConfiguration.REQUIRE_SSL_DEFAULT), | ||
| new OauthBearerSaslMechanismFactory(), | ||
| new XOauth2SaslMechanismFactory()); | ||
| return configuration -> loadBuiltInMechanisms(defaultFactories, configuration); | ||
| } | ||
|
|
||
| ImmutableList<SaslMechanism> load(HierarchicalConfiguration<ImmutableNode> configuration) throws ConfigurationException; | ||
| } |
Contributor
There was a problem hiding this comment.
The interface is the same as for IMAP stack, no need to duplicate it IMO, likely idem for default mechanisms
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow previous IMAP work #3059