feat(managesieve): add XOAUTH2 authentication mechanism

[felixauringer]

As the managesieve server only supports plain authentication, here is a first implementation of XOAUTH2 as an additional authentication mechanism for managesieve.

I would be happy about feedback :)

[chibenwa]

This looks great.

Can we try to write a test for OIDC support in managedSieve like this is done for IMAP and SMTP?

Also, out of curiosity do we have a managedSive compatible client>

We are missing:

• ManageSiee exemple in https://github.com/apache/james-project/tree/master/examples/oidc
• Documentation on how to configure this feature

[quantranhong1999]

Read it.

Thank you for your contribution.

This lacks some tests indeed.

[felixauringer]

I added OAUTHBEARER authentication and some tests.

The authentication logic I wrote is very similar to james-project/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/UsersRepositoryAuthHook.java or james-project/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java, I assume that those implement correct SASL XOAUTH2, but still decided to test many cases here.

I was not sure about using MPT and decided to implement a ManageSieveTestSystem like already existing for SMTP. As there is no managesieve client by Apache, I also had to write a small client.

I added tests for the whole authentication logic and found some bugs / non-standard-conforming behavior and tried to fix them.
There is one test I did not get working so far: After sending the logout command, I would like to check that the server really closes the connection. This happens asynchronously but even when waiting multiple seconds, isConnected still returns true. The RFC says the server MUST close the connection and in my understanding of the code, it also does so (channel.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(ChannelFutureListener.CLOSE)).

Also, out of curiosity do we have a managedSive compatible client>

Roundcube supports using managesieve with XOAUTH2/OAUTHBEARER.

I am working on the example and documentation, but if you have feedback to the tests before that, I would appreciate it 🙂

[felixauringer]

I have updated documentation and the oidc example. I added some CLI commands to test oidc authentication and made test.sh succeed. I did not test with thunderbird but my changes should not break the existing documentation on that.

[chibenwa]

Failing tests seems to be related. Can we investigate this?

[felixauringer]

The MPT tests are working again now.

The two logout tests which I added are still failing. As described above, I would like to keep those tests but do not know how to fix them.

There is also one test which I marked as disabled. It checks for malformed authentication data and currently fails but would also have failed with the original implementation. The current code is definitive too lenient when looking at the RFC but I didn't know whether there was a specific reason to allow this case
(You also allow spaces as separators in the authentication data instead of null bytes. This is also not correct, but I left it as is and didn't write a test for it.)
Are there any clients that rely on these non-RFC-compliant behaviors?

[chibenwa]

+1 for fixing this likely non-compliant unpquoted no explanation (thanks)

[chibenwa]

I am not fully sure that this code is compliant with eg Thunderbird extension

From my memories of SASL we either have

C: AUTHENTICATE "PLAIN" "AGFsaWNlAHNlY3JldA=="\r\n
S: OK "Logged in"

Or

C: AUTHENTICATE "PLAIN"\r\n
S: +\r\n
C: AGFsaWNlAHNlY3JldA==\r\n
S: OK "Logged in"\r\n

(with base64 being \0LOGIN\0password)

So seing not base64 encoded, qutoed password seems indeed weird to me...

From the above exemple the initial server response is + \r\n

---

After reading the code below I think the client payload handling is retro-compatible.

So I would welcome tests that demonstrate previous behaviours are still supported.

[felixauringer]

Yes, I am pretty sure that having spaces as separator is not compliant. If that's okay for you, I am fine with removing them.

The RFC actually also does not mention base64 but I also have never seen it without it.

The current behavior implementation allows the following for PLAIN:

• null as delimiter, then base64 encoded, then quoted
• null as delimiter, then quoted
• space as delimiter, then quoted

(strings always need to be quoted except if the length is given beforehand)

Which of those behaviors would you like to be supported? I implemented all of them because the other protocols allow these options.

[chibenwa]

Why do we need changing the initial server response here?

[felixauringer]

I do not understand where this + "" comes from but I didn't find it in any of the RFCs relating to ManageSieve SASL authentication.
There is one mention in the SASL RFC but only for the EXTERNAL mechanism which we do not use here.
The in my opinion relevant RFC here is RFC 5804: A Protocol for Remotely Managing Sieve Scripts. The formal syntax there defines the response to an authenticate command as follows:

response-authenticate = *(string CRLF)
                            ((response-ok [response-capability]) /
                             response-nobye)
                            ;; <response-capability> is REQUIRED if a
                            ;; SASL security layer was negotiated and
                            ;; MUST be omitted otherwise.

If we want continuation, we want to have a response-ok and because we do not negotiate a SASL security layer, we must omit response-capability.
It then simplifies to response-authenticate = *(string CRLF) response-ok. Assuming we do not want to have a response code or description, it further simplifies to response-authenticate = *(string CRLF) "OK" CRLF.
The description says:

Note that an empty challenge/response is sent as an empty string.

With all our supported authentication mechanisms, the challenge sent by the server is empty, so I would argue that the server should send the empty string. Sadly, the examples in the RFC do not show this case. Using the formal syntax from above, this leads to response-authenticate = DQUOTE DQUOTE CRLF "OK" CRLF. (One could probably also use the literal string variant without quotes but by supplying the exact string length. However, I would argue that it does not make much sense with an empty string.)

That's the reason why I changed it. In the RFC, I do not see any possibility how an (unquoted!) + sign could be the response to an authenticate command. I also didn't find why this server response shouldn't end with an OK, but I'm less sure about that (the examples seem to omit it in the challenge-response exchanges but I don't think the formal syntax allows it). What's the reason why you use the + here? Is there another RFC that I do not know?

[felixauringer]

Okay, I found the PLAIN SASL RFC from where your previous server response seems to come from. I really do think that it conflicts with the managesieve RFC (5804) but I am fine with keeping the old syntax then.

[chibenwa]

Hello @felixauringer

Thanks for having a look

I think as an implementer I just sticked to RFC-4616 exemples, yes (but that's10 years back.... I'm a bit rusty on the topic)

Note that the "" is likely none compulsory and not returned eg by the IMAP layer which also implement RFC-4616...

While what comes after is subject to discussion the fact of doing a continuation is mandatory.

[chibenwa]

I we think it is a good thing to be more relaxed than the RFC, I see no issue:

• Keeping the valuable comment
• Testing the existing behaviour with a passing test.

[chibenwa]

Otherall agree with proposed changes but I would need clarifications on AUTHENTICATE "PLAIN" handling.