-
Notifications
You must be signed in to change notification settings - Fork 38
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
BUGFIX: longer tokens violate maximum SMTP command line length #70
Conversation
The unit tests for the original implementation of XOAUTH2 do not exist and I'm afraid it would require a major refactor. I tested it locally on my computer against Dovecot, but I'd feel more comfortable if it could be tested more. How to proceed? |
} | ||
|
||
// server is expected to respond with 334 | ||
if (PEAR::isError($error = $this->parseResponse(334))) { |
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 should return 334 and empty message. I wonder if we should somehow test that the message from server is empty?
@aleshaczech |
Hi @schengawegga, let me add a few more details, hopefully it helps to better review and test my PR. TLDR: Dovecot, when acting as a SMTP client, follows the specification strictly and their implementation can be used to verify my PR. However, when Dovecot acts as a SMTP server, it's probably more tolerant, hence the testing needs to be done with fairly long XOAuth2 tokens in order to observe the buggy behaviour. In Dovecot, this obviously appears only with
…and also for the previous version 2.3.18:
As mentioned in a Dovecot's forum's thread, the affected versions were 2.3.18 and 2.3.19. In my case, the problem was observed with the version 2.3.19. The aforementioned thread also points to a commit that shows how Dovecot handles the From https://github.com/dovecot/core/blob/release-2.3.19/src/submission-login/submission-proxy.c#L267 (and similarly in https://github.com/dovecot/core/blob/release-2.3.19/src/lib-smtp/smtp-client-connection.c#L963): if (str_len(sasl_output_base64) == 0)
str_append(str, " =");
else if ((5 + strlen(mech_name) + 1 + str_len(sasl_output_base64)) >
SMTP_BASE_LINE_LENGTH_LIMIT)
client->proxy_sasl_ir = i_strdup(str_c(sasl_output_base64));
else {
str_append_c(str, ' ');
str_append_str(str, sasl_output_base64);
} From https://github.com/dovecot/core/blob/release-2.3.19/src/lib-smtp/smtp-common.h#L10: #define SMTP_BASE_LINE_LENGTH_LIMIT (512 - 2) It's worth mentioning that I observed Dovecot being able to receive tokens longer than 512 bytes. I believe Dovecot is more tolerant than the specification demands when acting as a server, but it's following the specification strictly when acting as a client. I think the relevant values are somehow related to https://github.com/dovecot/core/blob/release-2.3.19/src/lib-smtp/smtp-command.h#L4, but I didn't test it thoroughly enough to pinpoint the exact threshold when Dovecot's submission server refuses the token. If I am not wrong, that means that testing needs to be done with tokens way longer than 512 bytes to observe the behaviour, I vaguely remember tokens over 1.000 bytes still being okay for Dovecot. I observed the issue when the JWT's payload was long and signed with either 2.048-bit or 4.096-bit RSA keys. Dovecot's test suite actually uses tokens of length 1.792 bytes as can be seen here: https://github.com/dovecot/core/blob/release-2.3.19/src/lib-smtp/test-smtp-server-errors.c#L831. Hope this helps. |
Hi @schengawegga, I continued a bit with the research and found out that Dovecot accepts SMTP AUTH command parameters of up to 1.026 bytes length, not including the CRLF (that is whatever follows after Given that XOAUTH2 authentication mechanism command starts with Anything longer should result in:
I created a testing Dovecot Docker container, feel free to use it. |
@aleshaczech The propose change looks good to me. |
@aleshaczech Sorry for the long waiting time. I will try to test your request with your docker container in the next days. |
@aleshaczech thanks for the dovecot container. now i can reproduce the behavior of dovecot with auth parameters over 1.016 length (on my test runs), and your working bugfix. But i have one question. you wrote the RFC 4954 in the comment for the maximum token length of 512 bytes. but i can´t find any description in this RFC about 512 bytes, or any specific byte length. Can you explain, how you choose the length of 512 bytes to separate the auth command? I only want to understand, if it is possible that any other SMTP server has the limit of 512 bytes, and not at 1.016 or 1.026 as dovecot. At least, thanks for your great research and pull request. I will think about, if it is really necessary to do an if-statement in the code. All other SASL send out the authentification method and the credentials in base64 separately. Only XOAuth2 tries to send it as a combination of auth method and credentials. The combination will safe request time, but the question is, is the limit of 512 bytes (497 bytes) a safe limit for all common SMTP servers? Or, should we first put the combinated auth, and if that resolves in an "limit to long" message, do another try with the separated alternative. This will require more time, but it will be more close on the actual behavior of the code. @aleshaczech what is your opinion? |
Hi @schengawegga , thank you for testing it and confirming. Your findings of the maximum length of 1.016 bytes make sense, I guess I made a mistake measuring, maybe something to do with the Regarding the maximum length of SMTP commands, it's mentioned there in the (RFC 4954, section 4) indirectly as following:
and later in the same section regarding the size of the token:
(It's not very clearly said, but I believe that refers to the token length when sent outside of the initial response, that is when not part of the The referenced "SMTP" is RFC 2821, section 4.5.3.1 and states:
There's another definition of length in RFC 4954, section 3, however, that's unrelated to the issue I opened. It's the following:
That only extends the size of the authentication when part of the
I am mentioning it since it might be the reason why some implementations decided to support longer It's quite difficult to check all the RFCs and how they link to each other, so I'm not pretending to be sure. And, apparently, there are mismatches between RFC and reality, such as the case of Dovecot that seems to be more tolerant than the RFC requires. But exactly for that reason I'd suggest to follow the strict interpretation of RFC when sending data and go with 512 bytes for the Regarding the idea not to use the initial response at all, that's related to this part of the RFC 4954, section 4:
So, indeed it's an optimisation. I wouldn't mind keeping it, it can save a bit, but it makes the logic a bit more complex and error prone, so I don't have a strong opinion here. At least in my use case (JWT tokens signed with 2.048 bytes or 4.096 RSA keys) the token is almost always longer, I can imagine more people having similar use case, so perhaps the saving in practice are not that big. On the other hand, for short tokens, the behaviour would stay exactly as it is now, if I get it right; I mean that keeping this optimisation would keep the current behaviour for short-enough tokens, which could reduce the risk of running into another set of problems (e.g. what if some servers do not support receiving the token on a separate line due to their broken implementation of the RFC?). Regarding trying to send the token in the initial response and then handling the "too long" error, I would suggest to avoid it for a couple of reasons:
Hope this helps. |
Hi @aleshaczech , |
This BUGFIX for issue #69 fixes the case when the XOAuth2 is longer than 497 bytes. Such tokens must not be sent in the initial response (the
AUTH XOAUTH2 <token>
command), but rather send an empty initial response (AUTH XOAUTH2
), waiting for the server to reply with 334 and then sending the token. (https://datatracker.ietf.org/doc/html/rfc4954)