You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Now that CL 516355 has been merged a server can dynamically change allowed authentication methods for a given user by returning a PartialSuccessError.
When a PartialSuccessError is returned, the "partial success" boolean field in SSH_MSG_USERAUTH_FAILURE is set to true.
I propose adding a new error that allows to change authentication methods without returning a partial success error
// ChangeAuthMethodsError can be returned by any of the [ServerConfig]
// authentication callbacks to change the allowed authentication methods.
type ChangeAuthMethodsError struct {
Next ServerAuthCallbacks
}
func (e *ChangeAuthMethodsError) Error() string {
// We return a generic error string.
return "ssh: authentication failed"
}
This is a cleaner approach to allowing modification of authentication methods and will reuse the logic already implemented for PartialSuccessError.
The text was updated successfully, but these errors were encountered:
The idea behind the proposal is sound: having a way to switch auth methods without returning PartialSuccess.
I suggest creating an interface (internal or exported) that both PartialSuccessError and ChangeAuthMethodsError implement to use in the type assertion here.
// ChangeAuthMethods can be implemented by any error returned from auth callbacks to // change the set of auth callbacks used for further attempts on this connection.typeChangeAuthMethodsinterface {
NextAuthMethods() ServerAuthCallbacks
}
func (e*PartialSuccessError) NextAuthMethods() ServerAuthCallbacks { returne.Next }
func (e*ChangeAuthMethodsError) NextAuthMethods() ServerAuthCallbacks { returne.Next }
An exported interface has the benefit of using error values to affect other behaviors, like sending banner messages.
I don't think a ChangeAuthMethods interface composes well with BannerError for example, because we'd have to add NextAuthMethods to BannerError ourselves.
Instead, we already have a mechanism to nest error types, which is Unwrap.
We can document that we check for both BannerError, PartialSuccessError, and ChangeAuthMethodsError with errors.As, and then implementations can just compose them by nesting a ChangeAuthMethodsError in a BannerError (or more complex custom nesting by using their own type which Unwraps to all of those).
Good point, Unwrap is a better way to combine errors. Should we implement Unwrap for all 3 proposed error types, so they can be combined in an arbitrary order?
Proposal Details
Now that CL 516355 has been merged a server can dynamically change allowed authentication methods for a given user by returning a
PartialSuccessError
.When a
PartialSuccessError
is returned, the "partial success" boolean field in SSH_MSG_USERAUTH_FAILURE is set totrue
.I propose adding a new error that allows to change authentication methods without returning a partial success error
This is a cleaner approach to allowing modification of authentication methods and will reuse the logic already implemented for
PartialSuccessError
.The text was updated successfully, but these errors were encountered: