proposal: x/crypto/ssh: verified public key callback and arbitrary data in Permissions

[espadolini]

Proposal Details

The ServerConfig.PublicKeyCallback callback gets called as part of the server-side SSH handshake whenever the client tries to authenticate with a public key or when the client asks whether or not a public key would be accepted by the server. No matter the order in which keys are presented, and notwithstanding the mitigation for CVE-2024-45337 (#70779), the callback is called before any signature from the client is verified (or potentially even received), which means that it's not practical or advisable to have the server make decisions on whether or not to continue with the authentication (potentially with a partial success) based on data and computations that require more effort than what can be allowed for an unauthenticated and untrusted client.

I propose the addition of a new, optional callback to ssh.ServerConfig:

type ServerConfig struct {
	...
	VerifiedPublicKeyCallback func(ConnMetadata, PublicKey, *Permissions) (*Permissions, error)
}

If PublicKeyCallback returns no error for a given public key, the client provides a SSH_MSG_USERAUTH_REQUEST message with a valid signature for that public key, and VerifiedPublicKeyCallback is non-nil, the VerifiedPublicKeyCallback will be called before the server sends a response to the client, passing in the same ConnMetadata and PublicKey that was passed to the PublicKeyCallback, and the *Permissions object that was returned by PublicKeyCallback.

The VerifiedPublicKeyCallback can then return the same or a new *Permissions to complete the handshake, a *PartialSuccessError to continue the handshake with a partial success and new authentication callbacks, or a different non-nil error to fail the authentication and let the client attempt to use a different authentication method (I believe that according to RFC 4252 the server is allowed to fail the authentication for a key that it has acknowledged as acceptable earlier in the handshake, but it might be worth checking the behavior of popular SSH clients in such a situation). VerifiedPublicKeyCallback is guaranteed to not be called again after returning success - in case of a *PartialSuccessError that includes PublicKeyCallback, it might be called again once more for a different public key.

To avoid ambiguous behavior, PublicKeyCallback is not allowed to return a *PartialSuccessError if VerifiedPublicKeyCallback is non-nil.

To better support passing data between the two callbacks - and to allow for extracting richer authentication data without risking the sort of misuse that led to CVE-2024-45337 - I also propose that we extend Permissions to include a single any field, for use by the authentication callbacks:

type Permissions struct {
	CriticalOptions map[string]string

	Extensions map[string]string

	ExtraData any
}

The ExtraData field will be available for inspection in ServerConn.Permissions after the handshake, and it can be used to pass some arbitrary data from PublicKeyCallback to VerifiedPublicKeyCallback.

[gabyhelp]

Related Issues

• x/crypto/ssh: allow returning arbitrary data from the server authorization callbacks #21689 (closed)
• x/crypto/ssh: interaction between PublicKeyCallback and public key acceptability queries is not obvious #20094 (closed)
• x/crypto/ssh: misuse of ServerConfig.PublicKeyCallback may cause authorization bypass #70779 (closed)
• proposal: x/crypto/ssh: add callbacks to dynamically change server and client configuration #61650

Related Discussions

• [security] Vulnerability in golang.org/x/crypto

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[ianlancetaylor]

CC @golang/security

[seankhliao]

why would you need both at the same time?

cc @drakkan

[espadolini]

why would you need both at the same time?

PublicKeyCallback gets called at least once* for each public key presented by the client (without actually proving ownership over the key), the proposed VerifiedPublicKeyCallback is called only once, after the client has committed to using a key and has proven to own it. I don't believe it would be possible to only use VerifiedPublicKeyCallback, and only having PublicKeyCallback is inadequate for certain uses. For example, you might want to change some state in the rest of your program based on the key used by the client (which, admittedly, is also going to be possible after the handshake completes with the proposed "extra data" field in Permissions - but only if the handshake actually succeeds!).

*: before the recent mitigation for the CVE in #70779 it was called exactly once, now it can get called a second time depending on the order in which public keys are queried before deciding on one

[seankhliao]

I think we need a stronger reason to allow both being set and for passing data with any, otherwise it's much simpler to say at most one can be set, like net/http/httputil.ReverseProxy's Rewrite/Director funcs.

[FiloSottile]

You can't have only one set. Only PublicKeyCallback is the status quo, with the problems that motivate this proposal. Only VerifiedPublicKeyCallback just doesn't work at the protocol level: the client will send a query asking for the acceptability of a public key before it sends a signature, and what do we do? We can't call VerifiedPublicKeyCallback with it, or it's just behaving like PublicKeyCallback. We always answer yes? That will probably break connections that expect signatures to always get accepted after a query was accepted.

[tsipinakis]

ExtraData any

I came here looking exactly for this, the current structure is very limiting to only string data. Having an arbitrary interface as a field would solve a lot of issues. When having structures that are not easily string-marshable it is a big pain to pass them with the current system.

I support both proposals on this issue

[gopherbot]

Change https://go.dev/cl/636335 mentions this issue: ssh: add VerifiedPublicKeyCallback

[drakkan]

I like this proposal, since we are discussing about a new PublicKey callback, I would like to propose to also add the public key algorithm, which is currently not avaible to library users, to this callback

type ServerConfig struct {
	...
	VerifiedPublicKeyCallback func(ConnMetadata, PublicKey, *Permissions, algo string) (*Permissions, error)
}

[espadolini]

I would like to propose to also add the public key algorithm, which is currently not avaible to library users, to this callback

Would that be to disambiguate between the different RSA signature schemes? Are there other situations in which the algorithm is not publicKey.Algo() or publicKey.(*ssh.Certificate).Key.Algo()?

[drakkan]

I would like to propose to also add the public key algorithm, which is currently not avaible to library users, to this callback

Would that be to disambiguate between the different RSA signature schemes? Are there other situations in which the algorithm is not publicKey.Algo() or publicKey.(*ssh.Certificate).Key.Algo()?

Yes, this is for RSA keys. You can allow for example ssh-rsa for compatibility reasons, but you may also want to know the users who use ssh-rsa so you can suggest them to switch to a more secure alternative.
For other keys the signature algorithm matches the public key type.

[drakkan]

Maybe ExtraData could be ExtraData map[string]any this should not remove any flexibility and at the same time enforce a pattern to name any extra data added here. This is also possible with the original proposal, but this way it is more explicit and also allows different applications to save different data using their own unique map keys.

[FiloSottile]

What happens if VerifiedPublicKeyCallback modifies the *Permissions but then returns a *PartialSuccessError? There are three layers of mutability here: the map fields are mutable, the Permissions pointer is mutable, and the function can return a different Permissions pointer. I would like to see spelled out how they interact.

Can we see the full proposed doc comment of VerifiedPublicKeyCallback?

[espadolini]

What happens if VerifiedPublicKeyCallback modifies the *Permissions but then returns a *PartialSuccessError?

A partial success error resets any previous authn state (except the attempt count I guess) and the only thing that ends up mattering for the future is the content of PartialSuccessError.Next (including anything that was closed over by the funcs inside of it). Even now, and even more so after this proposal, ownership of the *Permissions object is always moved from PublicKeyCallback to internal machinery [to VerifiedPublicKeyCallback to internal machinery] to the ServerConn.

Can we see the full proposed doc comment of VerifiedPublicKeyCallback?

Something like

type ServerConfig struct {
    ...
    // VerifiedPublicKeyCallback, if non-nil, is called after a client successfully
    // confirms having control over a key that was previously approved by PublicKeyCallback.
    // The permissions object passed to the callback is the one returned by PublicKeyCallback
    // for the given public key and its ownership is transferred to the callback. The returned
    // Permissions object can be the same object, optionally modified, or a completely new object.
    // If VerifiedPublicKeyCallback is non-nil, PublicKeyCallback is not allowed to return a
    // PartialSuccessError, which can instead be returned by VerifiedPublicKeyCallback.
    VerifiedPublicKeyCallback func(conn ConnMetadata, key PublicKey, permissions *Permissions, signatureAlgorithm string) (*Permissions, error)
}