LOGIN AUTH next handler should unconditionally succeed if more is false

[james-d-elliott]

Description

Looking at the following code portions:

• AUTH LOGIN Next (go-mail)
• AUTH PLAIN Next (go stdlib)
• Client Auth (go stdlib)

It appears to me that when the more is false, then the authentication should be considered successful. Let me give my reasoning step by step:

1, Next is called here if err is nil.

2. The err var is only potentially set here as non-nil as it was prechecked here
3. Here are the possible codes (334 and 235) which are NOT an error and this seems to align with the spec
4. Here we see that more is set true if code is 334, so we can assume a 235 if it's not set and the Next implementation was called.

To Reproduce

Use a SMTP Server which doesn't respond with the suffix Authentication successful and instead uses something like Authentication succeeded. Example ProtonMail Bridge.

Expected behaviour

If a server responds with an irregular response as long as the response follows the spec that it's dealt with accordingly.

I'm happy to fix this, but wanted to see what you thought first and maybe there's a reason this has been done that I'm no aware of, the SMTP landscape is a bit wild west.

Screenshots

No response

Attempted Fixes

func (a *loginAuth) Next(fromServer []byte, more bool) ([]byte, error) {
    if more {
        switch string(fromServer) {
        case ServerRespUsername:
            return []byte(a.username), nil
        case ServerRespPassword:
            return []byte(a.password), nil
        default:
            return nil, fmt.Errorf("unexpected server response: %s", string(fromServer))
        }
    }

    return nil, nil
}

Additional context

Introduced In:

• d0f04353a278126a74d998a7b48f35e4acb62a2c
• 6a446bd4bd12fa0275f184ac84b397ffaa16dad5

I wish Next included the SMTP code for starters, and that SMTP servers used the examples in the spec.. but officially a 235 indicates a successful response. See RFC2554 Section 4

[wneessen]

Thanks for the bug report James. I'll have a closed look and prepare a fix tomorrow (given you already provided code, the fix should be quick ;) ).

[james-d-elliott]

I think the revert to what it was prior should work. I direct messaged you so if you need me to test it I can

[wneessen]

You were completely right James. I've reverted the change accordingly and tested with my usual test server as well as ProtonMail Bridge. Both work as expected now. I believe the change was made by me trying to be overly smart with the server responses, expecting that all servers would respond in a standardized way, which if of course foolish in the the SMTP world (as you call it "wild west" 😄 ). Sorry for causing the trouble with that and thanks again for the thorough report.

I would normaly not release a new version for such a small change, but given that Authelia users are actively affected, I'll create a new release today.

[james-d-elliott]

No drama at all. I went from handling all of this (multipart, auth, proper encoding detection, etc) in Authelia because the go stdlib was lacking to using this lib. So I'm fully aware it's very hard to get 100% right!

Also no rush on a release as we're probably not releasing for about a week as we're in a feature release cycle. I can just use the HEAD commit to test if you're not ready.

Edit: Side note, the fact stdlib doesn't return the status codes and it's completely frozen may turn out to be a reason to fork it later, though I have no specific knowledge that would confirm that other than it being the wild west.

[wneessen]

Edit: Side note, the fact stdlib doesn't return the status codes and it's completely frozen may turn out to be a reason to fork it later, though I have no specific knowledge that would confirm that other than it being the wild west.

Agreed. The frozen part made me thinking about exactly that already as well.