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: add a specific error for algorithm negotiation issues/errors #61536

Open
drakkan opened this issue Jul 23, 2023 · 21 comments
Open
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@drakkan
Copy link
Member

drakkan commented Jul 23, 2023

A common requirement for applications using x/crypto/ssh is to report some errors differently than others.
We currently have ServerAuthError and ErrNoAuth which are useful for authentication related errors.

If the client and the server cannot negotiate a common algorithm for host key, KEX, cipher, MAC we have a generic error and can only search for the error string.
New algorithms are added over time and old algorithms are deprecated and disabled by default, so a well defined error allows applications using the library to better report errors to end users and allow them to fix the issue (e.g by enabling an old algorithm).

An algorithm negotiation error is typically returned in the findCommon method, but the same error should also be returned in other places, for example if pickHostKey fails or in enterKeyExchange so searching for an error string is quite fragile.

We can add something very simple like this

// ErrAlgorithmNegotiation is the error returned if the client and the server cannot agree
// on an algorithm for host key, KEX, cipher, MAC or if an unsupported algorithm 
// is configured.
var ErrAlgorithmNegotiation = errors.New("ssh: algorithm negotiation error")

and wrap this error when needed, for example in findCommon we could use

return "", fmt.Errorf("%w: ssh: no common algorithm for %s; client offered: %v, server offered: %v", ErrAlgorithm, what, client, server)

instead of

return "", fmt.Errorf("ssh: no common algorithm for %s; client offered: %v, server offered: %v", what, client, server)

and the same in other places where there is an algorithm error.

If you see CL 508398, this error may also be returned if NewSignerWithAlgorithms fails for example.

We could also evaluate something more complex, such as an error structure with an exported error code for each error category.
This way we could, for example, have specific error codes for MAC, KEX or cipher negotiation errors, I'm not sure if it's worth the effort to implement and maintain, but if you prefer this approach I can try to propose an API for that as well.

cc @golang/security

@gopherbot gopherbot added this to the Proposal milestone Jul 23, 2023
@FiloSottile
Copy link
Contributor

FiloSottile commented Jul 23, 2023 via email

@drakkan
Copy link
Member Author

drakkan commented Jul 23, 2023

@FiloSottile do you mean something like this?

// ErrAlgorithmNegotiation is the error returned if the client and the server cannot agree
// on an algorithm for host key, KEX, cipher, MAC or if an unsupported algorithm
// is configured.
var ErrAlgorithmNegotiation = &AlgorithmNegotiationError{}

type AlgorithmNegotiationError struct {
	err                 error
	SupportedAlgorithms []string
}

func (e *AlgorithmNegotiationError) Error() string {
	return e.err.Error()
}

func (e *AlgorithmNegotiationError) Is(target error) bool {
	_, ok := target.(*AlgorithmNegotiationError)
	return ok
}

func (e *AlgorithmNegotiationError) Unwrap() error {
	return e.err
}

so we can use

errors.Is(err, ErrAlgorithmNegotiation)

var algoNegotiationErr *AlgorithmNegotiationError
errors.As(err, &algoNegotiationErr)

@FiloSottile
Copy link
Contributor

I mean that if in #58523 we come up with an Algorithms type, we should maybe expose it as part of AlgorithmNegotiationError. (No need to have ErrAlgorithmNegotiation if we have AlgorithmNegotiationError, applications can just use errors.As.)

@drakkan
Copy link
Member Author

drakkan commented Jul 23, 2023

ok thanks.

So the struct approach should be fine

// AlgorithmNegotiationError defines the error returned if the client and the
// server cannot agree on an algorithm for host key, KEX, cipher, MAC or if an
// unsupported algorithm is configured.
type AlgorithmNegotiationError struct {
	err                 error
	SupportedAlgorithms []string
}

func (e *AlgorithmNegotiationError) Error() string {
	return e.err.Error()
}

func (e *AlgorithmNegotiationError) Is(target error) bool {
	_, ok := target.(*AlgorithmNegotiationError)
	return ok
}

func (e *AlgorithmNegotiationError) Unwrap() error {
	return e.err
}

for now the algorithms are strings. If this changes in the future, we can use of the Algorithm type instead of strings.

However, it's probably better to wait for an accepted proposal for #58523 before proceeding here

@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jul 25, 2023
@hanwen
Copy link
Contributor

hanwen commented Jul 26, 2023

for now the algorithms are strings. If this changes in the future, we can use of the Algorithm type instead of strings.

we can't change the type for things like "aes128-ctr" from string to something else. That would break backward compatibility.

@rsc
Copy link
Contributor

rsc commented Jul 26, 2023

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

@hanwen
Copy link
Contributor

hanwen commented Jul 27, 2023

it should be possible to make this play well with #58523, at least to expose the remote’s supported algorithms,

not sure: that bug is about exposing algorithms in a successful connection setup (single algorithm for hostkey, kex, mac & cipher), where this issue is about unsuccessful connection setup (list of algorithms for one of {kex,mac,cipher,hostkey}). For backward compat reasons, the algorithms have to be string, so I don't think you can do much better than a proposal of struct AlgorithmError { What string; Algorithms []string }

@drakkan
Copy link
Member Author

drakkan commented Jul 27, 2023

@hanwen if we want to add What string perhaps we should also consider exposing these constants.

Other than that, what do you think about this?

// AlgorithmNegotiationError defines the error returned if the client and the
// server cannot agree on an algorithm for host key, KEX, cipher, MAC or if an
// unsupported algorithm is configured.
type AlgorithmNegotiationError struct {
	err                 error
	What                string
	RequestedAlgorithms []string
	SupportedAlgorithms []string
}

@hanwen
Copy link
Contributor

hanwen commented Jul 27, 2023

I don't understand what the inner error is good for.

other than that, Requested+Supported is likely needed to make sure the error string doesn't change, so LGTM.

@drakkan
Copy link
Member Author

drakkan commented Jul 27, 2023

I don't understand what the inner error is good for.

other than that, Requested+Supported is likely needed to make sure the error string doesn't change, so LGTM.

the inner error is to encapsulate the current error and thus keep the same error string for people who, copying our test cases, do things like strings.Contains(err.Error(), "common algorithm").

An example of usage in findCommon would be

return "", &AlgorithmNegotiationError{
		err:                 fmt.Errorf("ssh: no common algorithm for %s; client offered: %v, server offered: %v", what, client, server),
		What:                what,
		SupportedAlgorithms: ..., // need to pass if we are the client or the server
		RequestedAlgorithms: ...,
	}

@rsc
Copy link
Contributor

rsc commented Aug 2, 2023

@drakkan I still don't see why you need err. The AlgorithmNegotiationError can implement a Error method returning that string directly, no?

@drakkan
Copy link
Member Author

drakkan commented Aug 2, 2023

@drakkan I still don't see why you need err. The AlgorithmNegotiationError can implement a Error method returning that string directly, no?

This error can also be used in other places than findCommon and we may not always have a client and a server. For example this error is also appropriate for NewSignerWithAlgorithms, see this CL. We could use it also in pickHostKey and in enterKeyExchange and the error strings are all different. The (private) inner error is a way to allow to customize the error string and keep backward compatibility.

@hanwen
Copy link
Contributor

hanwen commented Aug 3, 2023

I agree with Russ that the inner error seems out of place. Can we change the details of how the error string is formatted, or is that subject to a compat promise too?

If we want to keep the string completely intact, it's a little annoying, because it says "server" and "client", so we'd have to add an IsServer or IsClient boolean.

re. other places: worst case, we can introduce different error types for the different types of negotiation failures.

@drakkan
Copy link
Member Author

drakkan commented Aug 3, 2023

I agree with Russ that the inner error seems out of place. Can we change the details of how the error string is formatted, or is that subject to a compat promise too?

I'm not sure about the compat promise for error strings, maybe there is no compat promise here.

If we want to keep the string completely intact, it's a little annoying, because it says "server" and "client", so we'd have to add an IsServer or IsClient boolean.

yes, this is the point, if we can change the error string it would be simpler. In our test case we do this. I also do something similar in SFTPGo because an algorithm negotiation error must be reported differently from other errors. Maybe we could just leave ssh: no common algorithm in the error string and remove the inner error.

re. other places: worst case, we can introduce different error types for the different types of negotiation failures.

we can but even there we have a list of supported algos and a list of requested algos so this error would fit perfectly

@rsc
Copy link
Contributor

rsc commented Dec 20, 2023

No new comments here since August. Have we converged on an API proposal?

@rsc
Copy link
Contributor

rsc commented Jan 10, 2024

Any updates here?

@drakkan
Copy link
Member Author

drakkan commented Jan 10, 2024

Sorry for the delay. I think we can simplify the proposal like this

// AlgorithmNegotiationError defines the error returned if the client and the
// server cannot agree on an algorithm for key exchange, host key, cipher, MAC.
type AlgorithmNegotiationError struct {
	What                string
	RequestedAlgorithms []string
	SupportedAlgorithms []string
}

we will discuss the structure's private fields and other implementation details in the CL. I think it's reasonable to use this error only for client/server negotiation errors and not other places too and we can keep the current error string to avoid any issues to our users

@rsc
Copy link
Contributor

rsc commented Jan 19, 2024

Have all remaining concerns about this proposal been addressed?

Proposal is to add:

// AlgorithmNegotiationError defines the error returned if the client and the
// server cannot agree on an algorithm for key exchange, host key, cipher, MAC.
type AlgorithmNegotiationError struct {
	What                string
	RequestedAlgorithms []string
	SupportedAlgorithms []string
}

@rsc
Copy link
Contributor

rsc commented Jan 26, 2024

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

Proposal is to add:

// AlgorithmNegotiationError defines the error returned if the client and the
// server cannot agree on an algorithm for key exchange, host key, cipher, MAC.
type AlgorithmNegotiationError struct {
	What                string
	RequestedAlgorithms []string
	SupportedAlgorithms []string
}

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/559056 mentions this issue: ssh: add AlgorithmNegotiationError

@rsc
Copy link
Contributor

rsc commented Feb 1, 2024

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

Proposal is to add:

// AlgorithmNegotiationError defines the error returned if the client and the
// server cannot agree on an algorithm for key exchange, host key, cipher, MAC.
type AlgorithmNegotiationError struct {
	What                string
	RequestedAlgorithms []string
	SupportedAlgorithms []string
}

@rsc rsc changed the title proposal: x/crypto/ssh: add a specific error for algorithm negotiation issues/errors x/crypto/ssh: add a specific error for algorithm negotiation issues/errors Feb 1, 2024
@rsc rsc modified the milestones: Proposal, Backlog Feb 1, 2024
drakkan added a commit to drakkan/crypto that referenced this issue Feb 24, 2024
Fixes golang/go#61536

Change-Id: Id38cc6d46879dbe2bdea04dec061596387ec6cfe
drakkan added a commit to drakkan/crypto that referenced this issue Mar 7, 2024
Fixes golang/go#61536

Change-Id: Id38cc6d46879dbe2bdea04dec061596387ec6cfe
drakkan added a commit to drakkan/crypto that referenced this issue Apr 9, 2024
Fixes golang/go#61536

Change-Id: Id38cc6d46879dbe2bdea04dec061596387ec6cfe
drakkan added a commit to drakkan/crypto that referenced this issue May 7, 2024
Fixes golang/go#61536

Change-Id: Id38cc6d46879dbe2bdea04dec061596387ec6cfe
drakkan added a commit to drakkan/crypto that referenced this issue May 9, 2024
Fixes golang/go#61536

Change-Id: Id38cc6d46879dbe2bdea04dec061596387ec6cfe
drakkan added a commit to drakkan/crypto that referenced this issue May 23, 2024
Fixes golang/go#61536

Change-Id: Id38cc6d46879dbe2bdea04dec061596387ec6cfe
drakkan added a commit to drakkan/crypto that referenced this issue Jun 4, 2024
Fixes golang/go#61536

Change-Id: Id38cc6d46879dbe2bdea04dec061596387ec6cfe
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