-
Notifications
You must be signed in to change notification settings - Fork 484
feat(managesieve): add XOAUTH2 authentication mechanism #2773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(managesieve): add XOAUTH2 authentication mechanism #2773
Conversation
chibenwa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read it.
Thank you for your contribution.
This lacks some tests indeed.
e17b138 to
6b61443
Compare
|
I added OAUTHBEARER authentication and some tests. The authentication logic I wrote is very similar to 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.
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 🙂 |
6b61443 to
49efc9a
Compare
|
I have updated documentation and the oidc example. I added some CLI commands to test oidc authentication and made |
7fc01a0 to
91ac2a3
Compare
da9b214 to
c31419e
Compare
|
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 |
2387ebe to
05bb331
Compare
...l/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/authenticate.test
Show resolved
Hide resolved
| C: AUTHENTICATE "PLAIN" | ||
| S: \+ "" | ||
| C: user password | ||
| S: "" | ||
| S: OK | ||
| C: "user password" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it was supported before, I added an additional test ensuring that a space still works as delimiter here.
| @Override | ||
| public String initialServerResponse(Session session) { | ||
| return "+ \"\""; | ||
| return "\"\"\r\nOK"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need changing the initial server response here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed my code and all tests to behave like before and send + "".
| // The SASL PLAIN standard (https://datatracker.ietf.org/doc/html/rfc4616) defines the following message: | ||
| // message = [authzid] UTF8NUL authcid UTF8NUL passwd | ||
| // The current code is more lenient and accepts the message without the first null byte. | ||
| @Disabled | ||
| @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I enabled the test and kept the comment.
|
Otherall agree with proposed changes but I would need clarifications on |
8fe1061 to
1a05a2e
Compare
Now, the behavior is the same except that the code is more strict when it comes to quoting. SASL mechanisms and the values sent by the client must be quoted according to the RFC. (In the RFC, there is the possibility of sending unquoted data which must be preceded by the length in the form |
91d2815 to
e5815f9
Compare
|
I do not understand the failing test, I have changed nothing related to the Postgres repository, I think. |
chibenwa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extensive test suite it's pleasan to read.
| C: AUTHENTICATE "PLAIN" | ||
| S: \+ "" | ||
| C: user password | ||
| C: "user password" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When authentication happens as part of continuation I am rather sure that the crediential must nt be quoted
| C: "user password" | |
| C: user password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a commit that ensures continuation must be sent without quotes and adapts the tests accordingly.
However, I am not sure that your reference supports this behavior. All examples using continuation use the syntax where the unquoted message is preceded by the exact length of the client message. With that syntax, it is of course not necessary to quote the client message.
|
The test failure is unrelated. I rescheduled a new build. |
1087d44 to
a78424c
Compare
|
https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2773/23 This seems related. Could you please have a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This work looks nice to me.
I can give a hand with the picky test suite if of any use.
Just ask.
Best regards,
Benoit
a78424c to
4fca7f8
Compare
| C: AUTHENTICATE "PLAIN" | ||
| S: \+ "" | ||
| C: user password | ||
| C: user password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CF exemples in https://datatracker.ietf.org/doc/html/rfc4616
Example mentions <NUL>tim<NUL>tanstaaftanstaaf
We did test those inputs by using (space) and <NUL> in an inter-changeable way hence I explexpect we are still able to handle C: user password`
Is it still the case?
Can we have a tet making sure this is still the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that the line before the one you referenced is also important here: C: {21}
The clients announces the exact length of its message. This syntax is defined in the managesieve standard but currently not implemented in James. I do not know whether there are any clients using it.
The exact line C: user password appears multiple times in the managesieve MPL tests that were there before my contribution.
I think in the MPL tests only use plain authentication with continuation, use spaces instead of NUL and do not quote the client message during continuation.
The Java Tests I wrote usually use NUL (including the leading NUL) and encode it with base64. However, there is also one test for accepting it without the leading NUL and one that accepts the credentials with one space between username and password (not base64 encoded). There are no Java tests that do test those variants also with continuation.
I am not exactly sure what additional tests you would like to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are currently no tests for a leading space when the credentials are not base64 encoded, do you mean that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an additional test.
Yes, should be fixed now.
Thanks for the offer! But this time it was not the testsuite but just a plain programming error by me. I thought I could make this small change without running the tests locally but that didn't work apparently 😅 |
Good job it's 🍏 :-) |
XOAUTH2 is described here: https://developers.google.com/workspace/gmail/imap/xoauth2-protocol OAUTHBERER is described here: https://datatracker.ietf.org/doc/html/rfc5801#section-4 There is a small difference in how the user argument is called and OAUTHBEARER also includes a GS2 header.
- Authentication is now aborted when session is already authenticated - Authentication is now aborted when client sends "*" - Arguments by the client are now consistently unquoted - If unquotation fails, authentication is aborted to prevent later NullPointerExceptions - Fixed NullPointerException on AuthenticationException - Server now sends correctly formatted response (including code) when waiting for additional authentication arguments - When client sends authentication credentials in second message, the first argument (command) is now interpreted as argument - When choosing mechanism failed, a proper error is returned to the client - When argument is not quoted, authentication is aborted I think with these changes, the server now behaves more like described in the RFC: https://datatracker.ietf.org/doc/html/rfc5804#section-4
- More modern syntax in compose file. - Remove non-working links from readme. - Use consistent container names (always ending in .example.com). - Reduce output of test script.
…ation error While trying to get the MPT tests for managesieve running, I realized that a client is trapped in the state "AuthenticationInProgress" when using continuation. This made longer test scripts impossible. To consistently reset the session, I introduced more errors that are handled in one place instead of returning a "NO" answer directly during authentication. Those changes also required some small changes to the MPT and the normal tests. Additionally, there were a parsing bug when sending space-separated username and password in the authentication continuation which is fixed now.
…uring continuation
…ers in SASL PLAIN
bfc44d2 to
07ac6a3
Compare
|
Recent changes regarding SASL improvement impact this PR A rebase is needed. If needed I can do it. |

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 :)