Permissions and PublicKey passing cleanup

[belak]

This makes two modifications:

1. It makes sure that Permissions objects are not shared between auth callbacks. This avoids users accidentally setting information in a PublicKey callback for a private key the user doesn't actually have.
2. The SSH key is now passed via Permissions.Extensions, to avoid leaking ssh keys which the user did not actually authenticate with into the Context.

Because this is a behavior change (even if it's only a change when used incorrectly), this counts as a breaking change and will involve a minor version bump.

[wxiaoguang]

I think maybe it needs one more check: before calling Server.Handler (session handler), it should make sure the public key is verified? Otherwise there will still be abuse: developers set some context value in "public key handler", and use them in "session handler".

[wxiaoguang]

The ctx.Permissions() seems quite fragile and it should be dropped IMO.

The reason is that it only reads the "permission in current context", which change a lot. It might not match the ssh connection permission in the session handler.

The ideal design should be exposing ssh connection's Permissions directly.

[belak]

I agree with your comment on ctx.Permissions() - it's been a longer term goal of mine to look into cleaning up the API of this package in general, but I haven't had the time for it.

[belak]

I think maybe it needs one more check: before calling Server.Handler (session handler), it should make sure the public key is verified? Otherwise there will still be abuse: developers set some context value in "public key handler", and use them in "session handler".

@wxiaoguang Could you clarify what you mean by this? I don't think it's accurate. With the fix from x/crypto, the PublicKeyHandler will be called an additional time if the last submitted key doesn't match the one they're actually using, which should resolve the issue.

[wxiaoguang]

I think maybe it needs one more check: before calling Server.Handler (session handler), it should make sure the public key is verified? Otherwise there will still be abuse: developers set some context value in "public key handler", and use them in "session handler".

Could you clarify what you mean by this? I don't think it's accurate. With the fix from x/crypto, the PublicKeyHandler will be called an additional time if the last submitted key doesn't match the one they're actually using, which should resolve the issue.

I was thinking about this case: if the PasswordHandler does some remote network access to get the public key,
assume there are 2 keys: A: the right one, B: the wrong one (only pub key), then the calls are:

PasswordHandler(A): network succeeds and verify succeeds
PasswordHandler(B): verify fails
PasswordHandler(A): network fails and verify fails

Then what would happen? (Maybe it is not a problem if the last PasswordHandler(A) stops the auth, but I am not sure)

[belak]

In that instance, the last public key handler would cause the auth to fail. Auth doesn't succeed until the user can actually prove they own that public key, by signing some information about the connection with the corresponding private key (See RFC4252, Section 7).

If I'm understanding right, as long as x/crypto/ssh was used correctly, previous versions wouldn't have been vulnerable either - the only thing they changed is to make the number of cached permissions responses be 1 rather than 16, having the effect that the last time the PublicKeyHandler called, the actual key used for auth is included.

The relevant code is here: https://github.com/golang/crypto/blob/v0.31.0/ssh/server.go#L628-L704. It essentially does the following:

1. Check if the public key handler result is in the cache
2. If not, run the PublicKeyHandler and cache the result
3. If this was a query (SSH_MSG_USERAUTH_REQUEST in the RFC above), tell the user about the result and continue to the next auth attempt
4. Otherwise, ensure the signature matches the public key. If successful, auth was successful and the perms associated with this public key are returned.

Because of that I don't think we need to additionally make sure the public key is verified before calling the server handler. At the point where the server handler is called, it has already been verified.

[cds2-stripe]

Hey @belak ! Thanks for your work on this. Any idea when this might make it into a new release? We'd love to pick up these changes.

[belak]

I was swamped with some work stuff, and really needed a break.

I'd be happy to merge this, but I'm a bit unsure if it would break any applications, and as it's a behavior change rather than an API change, I'm a bit worried it would have negative effects people wouldn't be able to easily find, especially since I've seen applications which rely on this broken behavior in the past.

Longer term, I'd love to give this package a larger re-write, but I'm not sure when or if I'll have the time, especially given how I don't get to use this package for my day job.