JAMES-4210 Generic SASL mechanism extension#3059
Conversation
Introduce a protocol-neutral SASL SPI in `protocols/api`. The new API models SASL as a stateful exchange with protocol-neutral initial requests, continuation steps, success/failure results, and authentication identities. It also exposes password and bearer-token authentication service contracts through `SaslSessionContext` so future mechanism implementations do not depend directly on IMAP or SMTP classes. Add contract tests covering one-step mechanisms, multi-step mechanisms, password-like authentication through the context service contract, delegated identities, defensive byte-array copying, and exchange cleanup.
Add a minimal IMAP bridge for the shared SASL SPI. The bridge keeps IMAP-specific wire handling outside the generic SPI: it converts IMAP AUTHENTICATE input to SaslInitialRequest, handles the SASL-IR "=" empty initial response marker, base64-encodes challenge continuations, decodes client continuation lines, and wires abort/close lifecycle handling around SaslExchange. Add unit tests for initial response decoding, continuation formatting, client response decoding, and exchange cleanup.
Add a minimal SMTP bridge for the shared SASL SPI. The bridge keeps SMTP-specific AUTH framing outside the generic SPI: it converts SMTP AUTH initial responses to SaslInitialRequest, handles the "=" empty initial response marker, maps SASL challenges to SMTP 334 responses, decodes client continuation lines, and wires abort/close lifecycle handling around SaslExchange. Add unit tests for initial response decoding, SMTP challenge formatting, client response decoding, and exchange cleanup.
Introduce reusable PLAIN, OAUTHBEARER and XOAUTH2 SASL mechanisms in protocols-api. Add the service-factory SPI so mechanisms can declare protocol-provided services and the registry can initialize them per session.
Refactor IMAP AuthenticateProcessor to use SaslMechanismRegistry while preserving direct non-Guice defaults. Move IMAP password and bearer-token authentication into protocol service factories and keep mailbox session and failure details in the IMAP SASL session context.
Wire SASL mechanism loading into the Guice IMAP server module. Add a default mechanism class-name provider and an extension service-factory provider so custom SASL extensions can load their own auth configuration from imapserver.xml.
Allow SmtpSaslBridge to be reused with a configured SASL protocol and add LMTP to SaslProtocol for future LMTP AUTH support.
Load extra IMAP SASL authentication service factory providers from auth.saslAuthenticationServiceFactoryProviderExtensions. Providers are Guice-instantiated, merged with built-in providers, and can parse their own IMAP auth configuration.
Add an EXAMPLE-TOKEN SASL mechanism to examples/custom-imap to demonstrate a custom mechanism, provider extension, and auth.exampleToken configuration. Add dedicated tests for custom SASL advertisement, successful custom auth, invalid-token rejection, and preserving built-in PLAIN authentication.
14ba04f to
cb204cf
Compare
|
Hello, I did try a lot to make the POC ready so far, for IMAP. I did rebase on the latest master to have the You can just jump to the last commit JAMES-4210 Add custom IMAP SASL extension example to see how we could implement an SASL mechanism extension for IMAP. Note that I did rush to make this POC alive for IMAP, so of course, review + polish and double-check would be needed later :) |
| @Override | ||
| public Set<Class<?>> requiredServices(SaslProtocol protocol) { | ||
| if (supports(protocol)) { | ||
| return Set.of(ExampleTokenSaslAuthenticationService.class); | ||
| } | ||
| return Set.of(); | ||
| } |
There was a problem hiding this comment.
If we need a ExampleTokenSaslAuthenticationService why don't we inject it (or its factory) via Guice ?
| @Override | ||
| public boolean isAvailable(SaslSessionContext context) { | ||
| return context.service(ExampleTokenSaslAuthenticationService.class).isPresent(); | ||
| } |
There was a problem hiding this comment.
Forgive me ut this is the only place the isAvailable method is actually non default - can we remove it?
There was a problem hiding this comment.
I'm glad that we get something more or less working, that's an achievement in itself. Koodo.
Now remain the next step: SIMPLIFY ! ANd rationalize... I'd expect this proposal to be refined so that we drop the extra aaccidental complexity.
Expect the process to take several working day (and to be painful) but we will be happy I believe about the end result!
I'd suggest we rationnalize before extending to other protocols. I'd expect from this protocol an actual code reduction by switching from O(SxP) implementation complexity to O(S+P) complexity where S is the count of SASL mechanism and P the count of protocols - what this implementation do not deliver yet !
Arsnael
left a comment
There was a problem hiding this comment.
Read it, not much to add for now, just minor comments
…inuation behavior
…or conditional continuation behavior
Replace the service/context-driven SASL SPI with mechanism-owned exchanges returning protocol-applied credentials, challenges, success data, or failures. Update built-in PLAIN/OIDC mechanisms and API tests to the simplified model.
Add Guice-backed resolution for configured SASL mechanism class names, with factory lookup for server-specific mechanisms and direct instantiation fallback for stateless mechanisms.
Move IMAP AUTHENTICATE to the simplified exchange model, apply parsed credentials in AuthenticateProcessor, support final server data, and harden exchange cleanup. Buffer the initial continuation and let normal request completion flush once to avoid duplicate continuation responses.
Resolve IMAP SASL mechanisms per IMAP server configuration, preserve defaults when auth.saslMechanisms is absent, and keep capability/enable processors tied to the same IMAP suite instance.
Remove the staged SMTP bridge experiment from this IMAP-focused simplification series. SMTP adoption remains a later step.
Update the custom IMAP extension example to contribute a SASL mechanism factory through Guice, keep auth.saslMechanisms configuration, and cover continuation plus final server-data behavior.
|
Hello, I did simplify the design a lot. I think it should be more or less OK now. Happy to receive any further remarks. |
| @Override | ||
| public void abort() { | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| } |
There was a problem hiding this comment.
What is the distinction between abort and close?
What are we supposed to do?
| <plainAuthDisallowed>false</plainAuthDisallowed> | ||
| <gracefulShutdown>false</gracefulShutdown> | ||
| <auth> | ||
| <saslMechanisms>PlainSaslMechanism,org.apache.james.examples.imap.sasl.ExampleTokenSaslMechanism</saslMechanisms> |
There was a problem hiding this comment.
Wouldn't it be more straightforward to have the SaslMechanismFactory defined here?
The protocol layer would use guice directly to instanciate the factory then instanciate the mechanism: it would be a more straightforward design no?
| private SaslStep authenticate(byte[] clientResponse) { | ||
| return OIDCSASLParser.parseDecoded(new String(clientResponse, StandardCharsets.US_ASCII)) | ||
| .map(response -> (SaslStep) new SaslStep.Credentials(new SaslCredentials.BearerToken( | ||
| response.getToken(), Username.of(response.getAssociatedUser())))) |
There was a problem hiding this comment.
So if I understand this correctly the Sasl layer do not perform the auth but provides the token handed over to the server.
Each calling layer would need to handled each kind of credentials.
That is not acceptable to me imo the athentication layer need to be embedded in the SASL layer so that we mutualize it and leverage a code / complexity reduction which is not the case here.
| /** | ||
| * Parsed credentials to be applied by the protocol handler. | ||
| */ | ||
| record Credentials(SaslCredentials credentials) implements SaslStep { | ||
| } |
There was a problem hiding this comment.
Let's drop this to make authz a SASL concern
| public static class FactoryBackedSaslMechanism extends FixedNameSaslMechanism { | ||
| private final String realm; | ||
|
|
||
| public FactoryBackedSaslMechanism() { | ||
| this("unused"); | ||
| } | ||
|
|
||
| private FactoryBackedSaslMechanism(String realm) { | ||
| super("FACTORY"); | ||
| this.realm = realm; | ||
| } | ||
|
|
||
| private String realm() { | ||
| return realm; | ||
| } | ||
| } |
There was a problem hiding this comment.
By declaring factories we actually avoid this wizardry
chibenwa
left a comment
There was a problem hiding this comment.
Sorry to be picky, but this work shall be simplified further.
Let's invest more time on it.
TODO: