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

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

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

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.

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

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 }

@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
}

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.

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: ...,
	}

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

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

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.

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

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

Any updates here?

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

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
}

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
}

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

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
}