x/crypto/ssh: server side multi-step authentication

[drakkan]

RFC 4252 section 5.1 defines the partial success boolean for failed authentication requests.
If set, it means the authentication method is ok but one or more steps are required to complete the authentication.

We currently support multi-step authentication on the client side but not on the server side.

Server side support is a common request, see #17889.

Several patches are already available, basically we have to return a partial success error and the method/s allowed to continue.
Additionally, according to RFC 4252 section 5:

The 'user name' and 'service name' are repeated in every new
authentication attempt, and MAY change. The server implementation
MUST carefully check them in every message, and MUST flush any
accumulated authentication states if they change. If it is unable to
flush an authentication state, it MUST disconnect if the 'user name'
or 'service name' changes.

I would like to discuss a few different solutions for adding this support in x/crypto/ssh.

First approach

Based on some patches discussed in #17889.

Export ErrPartialSuccess like this

// ErrPartialSuccess defines the error that authentication
// callbacks must return for multi-step authentication when a
// specific authentication step succeed
var ErrPartialSuccess = errors.New("ssh: authenticated with partial success")

Add the following to ConnMetadata interface

// PartialSuccessMethods returns the ordered list of
// authentication methods that returned ErrPartialSuccess.
// It can be used inside callbacks to find if a multi-step
// authentication is done using the correct sequence and to
// return the authentication methods that can continue
PartialSuccessMethods() []string

so we keep the ordered list of authentication methods that returned ErrPartialSuccess.

If the username changes we can reset this list or disconnet the client.

Add the following to ServerConfig struct

// NextAuthMethodsCallback, if not-nil, is called when another
// authentication callback returns ErrPartialSuccess or if, after an
// initial partial success, an authentication step fails.
// This callback must return the list of authentications methods
// that can continue.
// An empty list means no supported methods remain and so the
// multi-step authentication will fail
NextAuthMethodsCallback func(conn ConnMetadata) []string

This is the flow:

1. the application returns ErrPartialSuccess
2. the NextAuthMethodsCallback is called and the application returns the allowed authentication methods, the application can use PartialSuccessMethods to get the ordered list of authentication methods that returned ErrPartialSuccess

Limitations: we can't implement very specific use cases, such as enabling multi-step authentication for a specific public key (not sure if this is a real use case) or things like that. We just store the list of auth methods that returned ErrPartialSuccess

Second approach

Based on CL 399075 (cc @peterverraedt), export a PartialSuccess struct implementing the Error interface like this

// PartialSuccess might be returned from any of the authentication methods
// to indicate that the authentication is in progress, but more steps must be
// done. It should contain the authentication methods to offer in further
// authentication.
type PartialSuccess struct {
	// PasswordCallback, if non-nil, is called when a user
	// attempts to authenticate using a password.
	PasswordCallback func(conn ConnMetadata, password []byte) (*Permissions, error)

	// PublicKeyCallback, if non-nil, is called when a client
	// offers a public key for authentication. It must return a nil error
	// if the given public key can be used to authenticate the
	// given user. For example, see CertChecker.Authenticate. A
	// call to this function does not guarantee that the key
	// offered is in fact used to authenticate. To record any data
	// depending on the public key, store it inside a
	// Permissions.Extensions entry.
	PublicKeyCallback func(conn ConnMetadata, key PublicKey) (*Permissions, error)

	// KeyboardInteractiveCallback, if non-nil, is called when
	// keyboard-interactive authentication is selected (RFC
	// 4256). The client object's Challenge function should be
	// used to query the user. The callback may offer multiple
	// Challenge rounds. To avoid information leaks, the client
	// should be presented a challenge even if the user is
	// unknown.
	KeyboardInteractiveCallback func(conn ConnMetadata, client KeyboardInteractiveChallenge) (*Permissions, error)

	// GSSAPIWithMICConfig includes gssapi server and callback, which if both non-nil, is used
	// when gssapi-with-mic authentication is selected (RFC 4462 section 3).
	GSSAPIWithMICConfig *GSSAPIWithMICConfig
}

func (p *PartialSuccess) Error() string {
	return "ssh: partial success"
}

The application using the library can implement the multi-step logic using closures.
I have a slight preference for this solution.

cc @golang/security

[FiloSottile]

Add the following to ConnMetadata interface

Adding methods to interfaces is ordinarily not a backwards compatible change, with rare exceptions where interfaces were used somewhere they shouldn't have, and there is no plausible application-side implementation. Is there precedent for extending ConnMetadata? Do applications reimplement it, for example to shim some methods?

---

In general, I am not a fan of state management in the first approach: if an application needs to keep any other state than "this method was attempted and succeeded" they will do it in some value or closure that we can't reach to reset if the username changes.

It's not great that PartialSuccess needs to replicate all the callbacks from ServerConfig, but it does make application behavior for second steps more explicit. If no one has a better idea, I am happy with it.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[drakkan]

The first approach was backward incompatible, sorry for that. We can achieve the same in a different way and also without adding NextAuthMethodsCallback callback.

We can export an error struct for the partial error

// PartialSuccessError defines the error that authentication callbacks must
// return for multi-step authentication when a specific authentication step
// succeed.
type PartialSuccessError struct {
	// Method is the authentication method name, as defined in RFC 4252 that
	// completed with partial success.
	Method string
	// NextMethods defines the list of authentications methods that can
	// continue. An empty list means no supported methods remain and so the
	// multi-step authentication will fail. Method names are defined in RFC
	// 4252.
	NextMethods []string
}

and we can extend the ConnMetadata so the change is backward compatible

// PartialSuccessConnMetadata is a ConnMetadata that supports returning the list
// of authentication methods completed with partial success.
type PartialSuccessConnMetadata interface {
	ConnMetadata

	// PartialSuccessErrors returns the ordered list of the returned
	// ErrPartialSuccess. It can be used inside callbacks to find if a
	// multi-step authentication is done using the correct sequence.
	PartialSuccessErrors() []PartialSuccessError
}

The applications using the library return the authentication methods allowed to continue within the error and, in the authentication callbacks, can get the list of partial success errors to check the sequence.
However there is still minimal state handling with this approach.

[drakkan]

Hello,

some proof of concept implementations here:

https://github.com/drakkan/crypto/tree/multi_step1
https://github.com/drakkan/crypto/tree/multi_step2

I will continue to compare the proposed alternatives in the coming days.

[drakkan]

I did some comparative tests using both proposed implementations in SFTPGo. The closures approach also simplifies application code. I am very happy with it. I think this is the way to go.

[gopherbot]

Change https://go.dev/cl/516355 mentions this issue: ssh: add server side multi-step authentication

[rsc]

@hanwen, do you have any thoughts about this? Anyone else?

[hanwen]

I agree that approach 2 (golang/crypto@master...drakkan:crypto:multi_step2) looks better, because the next auth steps are explicit, and no need to extend ConnMetadata interface.

The dup of the callbacks is unfortunate, but I don't see any way to better without breaking backward compat.

[hanwen]

note to @drakkan : you put your fork of golang/crypto under AGPLv3, and looking at the code to possibly put it into upstream go gives me the beejeebies.

[drakkan]

note to @drakkan : you put your fork of golang/crypto under AGPLv3, and looking at the code to possibly put it into upstream go gives me the beejeebies.

This was done before I was a member of the project. I've explicitly granted a license exception for the upstream inclusion, if that's not enough I can remove the license change or limit it to code that will never be merged upstream (e.g. server-side diffie-hellman-group-exchange)

[tg123]

i am happy if https://github.com/tg123/sshpiper.crypto can be merged to upstream
it is private fork of ssh to support https://github.com/tg123/sshpiper to do 2fa and some other routing

let me know if anything i can help

[drakkan]

Hello @tg123,

thank you for your contribution.

Can you please provide more details on what is implemented in your crypto fork related to this proposal?

This proposal is about server-side multi step authentication (e.g. public key and then password).

Multi-factor authentication, for example password and then TOTP code as per RFC 6238, can already be implemented using keyboard interactive authentication.

[tg123]

the private fork impl first approach by adding

CreateCtx
and
NextAuthMethodsCallback(ctx)

it allows users to create a ctx when connection starts

auth callback simply return err and put some flag into ctx
nextauth will see the flag inside it and decide what methods to return.

i believe problem can be solved by introducing createctxcallback

[drakkan]

please be more explicit so that your proposal can be better evaluated.

In your fork I see these API changes:

• a new ChallengeContext interface

type ChallengeContext interface {
	Meta() interface{}

	ChallengedUsername() string
}

• you have a PiperConfig struct with methods similar to the upstream ServerConfig, do you propose to add the following to ServerConfig?

CreateChallengeContext func(conn ConnMetadata) (ChallengeContext, error)

NextAuthMethods func(conn ConnMetadata, challengeCtx ChallengeContext) ([]string, error)

and also change every callback to include ChallengeContext like this?

PublicKeyCallback func(conn ConnMetadata, key PublicKey, challengeCtx ChallengeContext) (*Permissions, error)

Thank you

[tg123]

the bottom line of my fork is to restrict all changes inside drop-in file sshpiper.go, no original ssh code will be touched to avoid conflict.

allow sdk user to store something with connection, so things can be shared across callbacks

git diff connection.go
diff --git a/ssh/connection.go b/ssh/connection.go
index 8f345ee..0dc587a 100644
--- a/ssh/connection.go
+++ b/ssh/connection.go
@@ -41,6 +41,12 @@ type ConnMetadata interface {
 
        // LocalAddr returns the local address for this connection.
        LocalAddr() net.Addr
+
+       // SetContextValue put some user defined value associated with the connection.
+       SetContextValue(key, val any)
+
+       // GetContextValue get value put by SetContextValue.
+       GetContextValue(key any) any
 }

add 2 new callbacks like approach 1.

diff --git a/ssh/server.go b/ssh/server.go
index 727c71b..808106b 100644
--- a/ssh/server.go
+++ b/ssh/server.go
@@ -66,6 +66,13 @@ type ServerConfig struct {
 
        hostKeys []Signer
 
+       // NewConnectionCallback, if non-nil, is called after key exchange completed but before authentication.
+       // server could use this callback to prepare auth context.
+       NewConnectionCallback func(conn ConnMetadata) (error)
+
+       // NextAuthMethodsCallback, if non-nil, return the list of authentications methods that can continue.
+       NextAuthMethodsCallback func(conn ConnMetadata) ([]string, error)
+
        // NoClientAuth is true if clients are allowed to connect without
        // authenticating.
        // To determine NoClientAuth at runtime, set NoClientAuth to true

example code

nextauthcallback = func(connmeta) {
if connmeta.GetContextValue("password") == "done" {
return "publickey"
}

return "password"
}

passwordcallback = func(connmeta, pass) {
 if check(pass) {
   connmeta.SetContextValue("password", "done")
 }
}

publickeycallback = func(connmeta, key) {
 if check(key) && connmeta.GetContextValue("password") == "done" {
    auth succ
 }
}

[drakkan]

Thank you @tg123, it's clear what you mean now. So we need to extend ConnMetadata and add:

      // SetContextValue put some user defined value associated with the connection.
       SetContextValue(key, val any)

       // GetContextValue get value put by SetContextValue.
       GetContextValue(key any) any

I think NewConnectionCallback func(conn ConnMetadata) (error) is not required once CL 51906 is merged, you should be able to set user defined data in ConfigForClientCallback.

I think allowing adding user-defined data to ConnMetadata can be useful in general, but perhaps it should be discussed in a different proposal.

Your suggested approach is quite similar to what I have used in SFTPGo until now, personally I prefer the second approach now, but let see what other developers think about.

I understand that the second approach requires some code changes on your part, but other than that if it's not applicable to your use case, can you explain why? It would be useful to know before making a decision. Thank you!

[tg123]

my fork is to proxy ssh conn, so no change to me both 1 and 2
seems 1 is more flexible to me if user can define allowed methods via NextAuthMethodsCallback

[peterverraedt]

@tg123 what you mean is the initial presentation of allowed methods to the user, based on the username sent in the first "none" authentication? I.e. user "a" and user "b" could possibly receive another list of methods.

This is at this point also not included in current proposal 1 and I'm not entirely sure that such a scenario is covered by RFC4252 as the client is free to change both the username and the method in a next authentication attempt. This seems to suggest the list of useful authentication methods returned by the server probably needs to include all methods usable by all users.

Although I agree that I had another fork at some point to do exactly that: to reveal only pubkey authentication for a set of users, and password authentication for another set. But it might be better to cover this in a different proposal.

Btw user-defined data can be kept by calling ssh.NewServerConn per connection, and adding a context in all authentication closures. And piping an ssh connection is possible without forking by starting the outbound connection just before returing authentication success and to a higher-level piping of the ssh.Requests and ssh.NewChannels.

[hanwen]

The idea of context had me thinking of the following:

what if we extend ServerConfig to have stateful flavors of the callbacks?

type ConnState struct {

}
type ServerAuthResult struct {
   Method string
   Error error
   Permissions *Permissions
} 

// Return succeeded authentication methods
func (cc *ConnState) AuthResults() []ServerAuthResult

type ServerConfig struct {
    PasswordStateCallback func(state *ConnState, conn ConnMetadata, password []byte) (*Permissions, error)
}

We could document that the XxxxStateCallback override the XxxxCallback callbacks when provided.

This would let us insert a struct into the server auth process. It provides a mechanism to set state (eg. for multi-step auth) and gives us more flexibility for adding future extensions.

[drakkan]

While I agree that we need a way to allow library users to add some contextual data. I would add them to ConnMetadata which is already available in current callbacks, for example in a backward compatible way like this

type ConnMetadataContext interface {
    ConnMetadata
    SetContextValue(key, val interface{})
    GetContextValue(key interface{}) interface{}
}

but I think this should be discussed in a different proposal.

As for multi-step authentication, I think keeping the state within the library is more complex than the proposed callback approach because the library needs to store successful authentications and reset them if the username changes, also users of the library need to compare successful authentications with the expected flow and instruct crypto/ssh to return a partial success error and the authentication methods allowed to continue.

Using the callback approach the application just return a partial success error with the new callbacks.

For example, think of a flow where password authentication is only allowed after a successful public key authentication:

• Using the callback approach you can just set a PublicKey callback so password authentication is not even listed initially and therefore SFTP clients won't try it.
• Using an approach where authentication callbacks are static you have to define both password and public key callbacks, check the stored state, compare it to the desired flow, and return an error if a client attempts password authentication before a successful public key authentication.

[rsc]

No new comments here recently; have we converged on an API proposal?

[drakkan]

No new comments here recently; have we converged on an API proposal?

I have expressed my opinion and preference, however I suggest waiting a little longer to get other opinions, e.g. from @FiloSottile

[drakkan]

Here is the updated proposal after also considering #64974

// ServerAuthCallbacks defines server-side authentication callbacks.
type ServerAuthCallbacks struct {
	// PasswordCallback, if non-nil, is called when a user attempts to
	// authenticate using a password.
	PasswordCallback func(conn ConnMetadata, password []byte) (*Permissions, error)

	// PublicKeyCallback, if non-nil, is called when a client offers a public
	// key for authentication. It must return a nil error if the given public
	// key can be used to authenticate the given user. For example, see
	// CertChecker.Authenticate. A call to this function does not guarantee that
	// the key offered is in fact used to authenticate. To record any data
	// depending on the public key, store it inside a Permissions.Extensions
	// entry.
	PublicKeyCallback func(conn ConnMetadata, key PublicKey) (*Permissions, error)

	// KeyboardInteractiveCallback, if non-nil, is called when
	// keyboard-interactive authentication is selected (RFC 4256). The client
	// object's Challenge function should be used to query the user. The
	// callback may offer multiple Challenge rounds. To avoid information leaks,
	// the client should be presented a challenge even if the user is unknown.
	KeyboardInteractiveCallback func(conn ConnMetadata, client KeyboardInteractiveChallenge) (*Permissions, error)

	// GSSAPIWithMICConfig includes gssapi server and callback, which if both
	// non-nil, is used when gssapi-with-mic authentication is selected (RFC
	// 4462 section 3).
	GSSAPIWithMICConfig *GSSAPIWithMICConfig

	// NoClientAuthCallback, if non-nil, is called when a user
	// attempts to authenticate with auth method "none".
	// NoClientAuth must also be set to true for this be used, or
	// this func is unused.
	NoClientAuthCallback func(ConnMetadata) (*Permissions, error)
}

// PartialSuccessError might be returned from any of the authentication methods
// to indicate that the authentication is in progress, but more steps must be
// done. It should contain the authentication methods to offer in further
// authentications.
type PartialSuccessError struct {
	// Next defines the authentication callbacks that are allowed after a
	// partial success error.
	Next ServerAuthCallbacks
}

func (p *PartialSuccessError) Error() string {
	return "ssh: authenticated with partial success"
}

[rsc]

Any comments on the proposed API in #61447 (comment) ?

[awly]

Replied on the CL, but I like the API 👍
It fits the wire format and doesn't break compatibility.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Proposal details in #61447 (comment)

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Proposal details in #61447 (comment)