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

SASL reauthentication causes problems with implementations where accounts are linked to server access #192

Closed
SaberUK opened this Issue Dec 18, 2015 · 11 comments

Comments

Projects
None yet
6 participants
@SaberUK
Contributor

SaberUK commented Dec 18, 2015

The sasl-3.2 specification states:

Servers MUST allow a client, authenticated or otherwise, to reauthenticate by sending a new AUTHENTICATE message at any time.

Servers MAY disconnect ANY client at any time as a result of failed authentication, including both unregistered and registered clients, but MUST provide the reason for the authentication failure prior to disconnection.

This works for most uses but causes problems for implementations where your account defines your access to the server and it is undesirable to allow people to log out (e.g. in a corporate environment).

My proposed solution is to change the verb in the first section above from "MUST" to "SHOULD" and to restore the requirement to send ERR_SASLALREADY if reauthentication is not available.

cc: @rburchell

@rburchell

This comment has been minimized.

Show comment
Hide comment
@rburchell

rburchell Dec 18, 2015

Thanks, I was going to submit this but hadn't had time.

Thanks, I was going to submit this but hadn't had time.

@grawity

This comment has been minimized.

Show comment
Hide comment
@grawity

grawity Dec 18, 2015

Contributor

Suggesting "If the server allows clients to connect without authentication, or to log out from an account while connected" – at least in the former case reauth is really desired and I'd rather have it remain a MUST tbh.

Could be stated another way – must send ERR_SASLALREADY if already logged in? Do existing services allow re-identify without a logout? Afaik P10 doesn't support logout...

Contributor

grawity commented Dec 18, 2015

Suggesting "If the server allows clients to connect without authentication, or to log out from an account while connected" – at least in the former case reauth is really desired and I'd rather have it remain a MUST tbh.

Could be stated another way – must send ERR_SASLALREADY if already logged in? Do existing services allow re-identify without a logout? Afaik P10 doesn't support logout...

@DanielOaks

This comment has been minimized.

Show comment
Hide comment
@DanielOaks

DanielOaks Dec 18, 2015

Member

mammon supports logging into a different account at any time with another SASL authentication, while logged in. ERR_SASLALREADY if already logged in and server doesn't support re-authentication while already logged in?

Member

DanielOaks commented Dec 18, 2015

mammon supports logging into a different account at any time with another SASL authentication, while logged in. ERR_SASLALREADY if already logged in and server doesn't support re-authentication while already logged in?

@attilamolnar

This comment has been minimized.

Show comment
Hide comment
@attilamolnar

attilamolnar Dec 18, 2015

Member

This works for most uses but causes problems for implementations where your account defines your access to the server and it is undesirable to allow people to log out (e.g. in a corporate environment).

I don't see the practical problems this causes, sasl-3.2 requires cap-notify. Therefore a server can remove the sasl cap after authentication which must be supported by clients supporting sasl-3.2.

Member

attilamolnar commented Dec 18, 2015

This works for most uses but causes problems for implementations where your account defines your access to the server and it is undesirable to allow people to log out (e.g. in a corporate environment).

I don't see the practical problems this causes, sasl-3.2 requires cap-notify. Therefore a server can remove the sasl cap after authentication which must be supported by clients supporting sasl-3.2.

@rburchell

This comment has been minimized.

Show comment
Hide comment
@rburchell

rburchell Dec 23, 2015

I don't see the practical problems this causes, sasl-3.2 requires cap-notify. Therefore a server can remove the sasl cap after authentication which must be supported by clients supporting sasl-3.2.

Not if one takes this requirement at face value. If reauthentication is a MUST, then you can't just disallow it because some other part of the specification allows you to do that. At least, that would be my interpretation of the required behavior - which just goes to show that having this sort of ambiguity in a specification is a bad thing.

As such, I would suggest that this text is simply removed. If you would instead prefer to specify the handling of this error case too, then there's two options judging by this thread:

  • ERR_SASLALREADY
  • Remove the cap

Either of these is ultimately fine by me too, although this may warrant a little more discussion.

It may be worthwhile to take a look at RFC 2119 point 6:

Imperatives of the type defined in this memo must be used with care
and sparingly. In particular, they MUST only be used where it is
actually required for interoperation or to limit behavior which has
potential for causing harm (e.g., limiting retransmisssions) For
example, they must not be used to try to impose a particular method
on implementors where the method is not required for
interoperability.

I don't see the practical problems this causes, sasl-3.2 requires cap-notify. Therefore a server can remove the sasl cap after authentication which must be supported by clients supporting sasl-3.2.

Not if one takes this requirement at face value. If reauthentication is a MUST, then you can't just disallow it because some other part of the specification allows you to do that. At least, that would be my interpretation of the required behavior - which just goes to show that having this sort of ambiguity in a specification is a bad thing.

As such, I would suggest that this text is simply removed. If you would instead prefer to specify the handling of this error case too, then there's two options judging by this thread:

  • ERR_SASLALREADY
  • Remove the cap

Either of these is ultimately fine by me too, although this may warrant a little more discussion.

It may be worthwhile to take a look at RFC 2119 point 6:

Imperatives of the type defined in this memo must be used with care
and sparingly. In particular, they MUST only be used where it is
actually required for interoperation or to limit behavior which has
potential for causing harm (e.g., limiting retransmisssions) For
example, they must not be used to try to impose a particular method
on implementors where the method is not required for
interoperability.

@rburchell

This comment has been minimized.

Show comment
Hide comment
@rburchell

rburchell Dec 23, 2015

mammon supports logging into a different account at any time with another SASL authentication, while logged in

I should clarify: the point here isn't "oh, supporting logging into a different account is such a hard problem, we can't implement that, whatever shall we do". From a technical standpoint, sure, reauthentication is easy. However, for some environments, reauthentication is a totally undesired behavior as a byproduct of how accounts work in that system.

Picture a system where authentication is tied to a corporate identity: their account tells you who that person is, and in such an environment, there is only a single account. "Joe" from accounting isn't suddenly going to become "James" the CEO. There's no way to create multiple accounts for the end user, and they certainly shouldn't be switching between them.

mammon supports logging into a different account at any time with another SASL authentication, while logged in

I should clarify: the point here isn't "oh, supporting logging into a different account is such a hard problem, we can't implement that, whatever shall we do". From a technical standpoint, sure, reauthentication is easy. However, for some environments, reauthentication is a totally undesired behavior as a byproduct of how accounts work in that system.

Picture a system where authentication is tied to a corporate identity: their account tells you who that person is, and in such an environment, there is only a single account. "Joe" from accounting isn't suddenly going to become "James" the CEO. There's no way to create multiple accounts for the end user, and they certainly shouldn't be switching between them.

@attilamolnar

This comment has been minimized.

Show comment
Hide comment
@attilamolnar

attilamolnar Jan 6, 2016

Member

I'm ok with changing or removing the sentence restriction in question (added in 04e0b87)

Member

attilamolnar commented Jan 6, 2016

I'm ok with changing or removing the sentence restriction in question (added in 04e0b87)

@Mikaela

This comment has been minimized.

Show comment
Hide comment
@Mikaela

Mikaela Jan 6, 2016

Contributor

If you remove it, you remove me whole SASL REAUTH, but changing it to SHOULD should probably keep IRCds that can do it doing it, so I am OK with change to SHOULD.

Contributor

Mikaela commented Jan 6, 2016

If you remove it, you remove me whole SASL REAUTH, but changing it to SHOULD should probably keep IRCds that can do it doing it, so I am OK with change to SHOULD.

@attilamolnar

This comment has been minimized.

Show comment
Hide comment
@attilamolnar

attilamolnar Jan 6, 2016

Member

Yes, the restriction should be removed not reauth itself

Member

attilamolnar commented Jan 6, 2016

Yes, the restriction should be removed not reauth itself

@DanielOaks

This comment has been minimized.

Show comment
Hide comment
@DanielOaks

DanielOaks Jan 8, 2016

Member

This should be resolved thanks to #204

Member

DanielOaks commented Jan 8, 2016

This should be resolved thanks to #204

@SaberUK

This comment has been minimized.

Show comment
Hide comment
@SaberUK

SaberUK Jan 30, 2016

Contributor

This has been resolved.

Contributor

SaberUK commented Jan 30, 2016

This has been resolved.

@SaberUK SaberUK closed this Jan 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment