Skip to content
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

x/crypto/ssh: allow to change allowed authentication methods #66815

Open
drakkan opened this issue Apr 14, 2024 · 15 comments
Open

x/crypto/ssh: allow to change allowed authentication methods #66815

drakkan opened this issue Apr 14, 2024 · 15 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@drakkan
Copy link
Member

drakkan commented Apr 14, 2024

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 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.

@gopherbot gopherbot added this to the Proposal milestone Apr 14, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/578735 mentions this issue: ssh: allow to dynamically change authentication methods

@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Apr 14, 2024
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@awly
Copy link
Contributor

awly commented Apr 17, 2024

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.
type ChangeAuthMethods interface {
    NextAuthMethods() ServerAuthCallbacks
}

func (e *PartialSuccessError) NextAuthMethods() ServerAuthCallbacks { return e.Next }
func (e *ChangeAuthMethodsError) NextAuthMethods() ServerAuthCallbacks { return e.Next }

An exported interface has the benefit of using error values to affect other behaviors, like sending banner messages.

@FiloSottile
Copy link
Contributor

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).

@awly
Copy link
Contributor

awly commented May 3, 2024

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?

@FiloSottile
Copy link
Contributor

I think Unwrap makes sense on errors that are likely to be wrappers, and it feels like BannerError is much more likely to be a wrapper ("something else went wrong and I want to send you a banner to explain it") than PartialSuccessError and ChangeAuthMethodsError (which often will just be "everything is fine, I just learned more about you and want you to continue with these auth methods").

Also, is a PartialSuccessError that wraps a non-nil error... not a partial success? It'd be a bit confusing.

The nice thing about errors.As is that if you do need to make PartialSuccessError wrap something, you can implement your own partialSuccessErrorWrapper type with a As(interface{}) bool method (or your own error type that returns a PartialSuccessError and another error from an Unwrap() []error method).

@rsc
Copy link
Contributor

rsc commented May 15, 2024

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

@rsc
Copy link
Contributor

rsc commented May 23, 2024

@drakkan and @FiloSottile what is the current status of this discussion. Is there a consensus for something? If so, what specifically?

@drakkan
Copy link
Member Author

drakkan commented May 24, 2024

@rsc I think we should implement the original proposal and not add Unwrap to ChangeAuthMethodsError and PartialSuccessError.

I think it makes sense to check for both ChangeAuthMethodsError and PartialSuccessError using errors.As. The use case is if you want to return a partial success or change the allowed authentication methods and at the same time you want to send a banner with some explanations to the user: in this case the authentication callback can return a BannerError wrapping ChangeAuthMethodsError or PartialSuccessError. However this is an implementation detail we can discuss during the CL review

@drakkan
Copy link
Member Author

drakkan commented May 26, 2024

@awly I rebased the CL 578735 and added some test cases for PartialSuccessError and ChangeAuthMethodsError wrapped in a BannerError like this

err := &BannerError{
	Err: &PartialSuccessError{
	     ....					
	},
	Message: banner1,
}
err := &BannerError{
	Err: &ChangeAuthMethodsError{
	     ....					
	},
	Message: banner2,
}

So, by implementing the original proposal, it is possible to both dynamically change the allowed authentication methods (with or without returning partial success) and return a customized banner at the same time.

Please let us know if this is enough for your use case. Thank you

@awly
Copy link
Contributor

awly commented May 27, 2024

@drakkan yep, the original proposal works for us. My suggestions were mostly aimed at making the implementation simpler, but the API in this proposal looks good too. No objections here!

@rsc
Copy link
Contributor

rsc commented May 29, 2024

It sounds like the new API is:

// 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"
}

and nothing else. Do I have that right?

@drakkan
Copy link
Member Author

drakkan commented May 29, 2024

It sounds like the new API is:

// 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"
}

and nothing else. Do I have that right?

I confirm, thank you!

@rsc
Copy link
Contributor

rsc commented Jun 5, 2024

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

The new API is:

// 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"
}

and nothing else.

@rsc
Copy link
Contributor

rsc commented Jun 12, 2024

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

The new API is:

// 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"
}

and nothing else.

@rsc rsc changed the title proposal: x/crypto/ssh: allow to change allowed authentication methods x/crypto/ssh: allow to change allowed authentication methods Jun 12, 2024
@rsc rsc modified the milestones: Proposal, Backlog Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

6 participants