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 ServerConfig.HostKeyAlgorithms #52132

Open
Tracked in #49952
drakkan opened this issue Apr 4, 2022 · 20 comments
Open
Tracked in #49952

x/crypto/ssh: add ServerConfig.HostKeyAlgorithms #52132

drakkan opened this issue Apr 4, 2022 · 20 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal-FinalCommentPeriod
Milestone

Comments

@drakkan
Copy link

@drakkan drakkan commented Apr 4, 2022

Currently Go's SSH package doesn't permit a server to disable some host keys algorithms, for example if you provide an RSA host key we can't disable ssh-rsa which use sha1 and is disabled by default in recent versions of OpenSSH.

I propose adding a HostKeyAlgorithms string list to the ServerConfig similar to the one already available for the ClientConfig:

// A list of enabled host key algorithms. If unspecified then a sensible
// default is used.
HostKeyAlgorithms []string

I have submitted a PR that implement this new feature. This is the proposal for the API change.

@gopherbot gopherbot added this to the Proposal milestone Apr 4, 2022
@ianlancetaylor ianlancetaylor added the Proposal-Crypto label Apr 6, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 6, 2022

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Apr 6, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Apr 13, 2022

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 rsc moved this from Incoming to Active in Proposals Apr 13, 2022
@rsc
Copy link
Contributor

@rsc rsc commented May 4, 2022

cc @golang/security

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented May 4, 2022

Adding support for restricting the algorithms seems reasonable. I'd err on the side of doing this at the point of adding a host key, rather than adding a configurable list in ServerConfig though, since it would allow us to prevent the circumstance where the user configures a server which has no supported algorithms for the set of configured host keys.

Something along the lines of func (s *ServerConfig) AddHostKeyWithAlgos(key Signer, []string) error would allow adding a host key with only a specific set of algorithms enabled, and would allow us to verify that at least one viable algorithm was configured. The implementation would be somewhat more complex (although not terribly so), but would be somewhat harder to use incorrectly.

cc @FiloSottile who most recently mucked around in the SSH code, and likely has stronger opinions about the cleanest way to do this.

@drakkan
Copy link
Author

@drakkan drakkan commented May 5, 2022

@rolandshoemaker only for info, currently the library does not perform any consistency check, for example we can start an ssh server without host keys (well get the server has no host keys error at runtime ) or we can set an unsupported MAC (with run-time panic if the client selects it).

I could try to write a patch to add the func (s *ServerConfig) AddHostKeyWithAlgos(key Signer, []string) error method and I can populate the allowed host key algorithms there.

@FiloSottile do you have other suggestions? Thanks

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented May 6, 2022

The core issue is that Signer doesn't advertise what algorithms it supports. This is also a backwards compatibility issue because if we add support for an algorithm, all Signers with a compatible public key need to get updated or they'll break.

A better solution I think would be to extend the AlgorithmSigner interface with another interface upgrade (MultiAlgorithmSigner? AlgorithmSignerWithAlgorithms?) that includes a method that reports the algorithms supported by that signer.

It would also solve #36261 because the order of the returned algorithms would allow specifying a preference order. This previously came up in #39885 (comment). See also https://github.com/golang/crypto/blob/2c7772ba30643b7a2026cbea938420dce7c6384d/ssh/handshake.go#L461-L469 and golang/crypto@1baeb1c.

@drakkan
Copy link
Author

@drakkan drakkan commented May 11, 2022

Thanks,

so we could add a new interface like this:

// MultiAlgorithmSigner is an AlgorithmSigner that also reports the algorithms
// supported by that signer
type MultiAlgorithmSigner interface {
	AlgorithmSigner

	// Algorithms returns the available algorithms
	Algorithms() []string
}

and a new method to get an AlgorithmSigner restricted to the specified algos

// NewSignerWithAlgos returns a signer restricted to the specified algorithms.
// It returns an error if the specified algorithms are incompatible with the
// public key type
func NewSignerWithAlgos(signer AlgorithmSigner, algorithms []string) (Signer, error) 

library users who want to restrict algorithms can use this new method and pass the returned signer to the existing AddHostKey method.

We could also implement the Algorithms() []string method for all the existing AlgorithmSigner implementations returning all the supported algos for the specified key types.

This way in the server handshake we should be able do something like this:

if signer, ok := k.(MultiAlgorithmSigner); ok {
    msg.ServerHostKeyAlgos = append(msg.ServerHostKeyAlgos, signer.Algorithms()...)
} else {
   msg.ServerHostKeyAlgos = append(msg.ServerHostKeyAlgos, keyFormat)
}

alternatively we can check for both MultiAlgorithmSigner and AlgorithmSigner here.

If this is ok I can send a new patch, thanks

@rsc
Copy link
Contributor

@rsc rsc commented May 25, 2022

I'm not sure this discussion is converging. @rolandshoemaker or @FiloSottile any thoughts on whether we want to add more implicit interfaces here?

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented May 25, 2022

#52132 (comment) is consistent with what I was suggesting in #52132 (comment) and I like the NewSignerWithAlgos addition. We should make sure it also solves #36261, but I think it should.

@drakkan
Copy link
Author

@drakkan drakkan commented May 28, 2022

@FiloSottile thanks.

A patch is here:

https://go-review.googlesource.com/c/crypto/+/409215

it would be great if someone could also review my patch that adds server side support for RFC 8308:

https://go-review.googlesource.com/c/crypto/+/396714

This patch is included in SFTPGo since v2.2.3, no complaints so far. Thanks!

@rsc
Copy link
Contributor

@rsc rsc commented Jun 1, 2022

It's a little odd to see both Algorithms and Algos (spellings) in the API. There are no "Algos" in the current go/api/*.txt files. Probably we should spell it out?

Otherwise, it sounds like people are OK with #52132 (comment)?

@drakkan
Copy link
Author

@drakkan drakkan commented Jun 3, 2022

@rsc thanks. I have renamed NewSignerWithAlgos to NewSignerWithAlgorithms. Please let me know if any other changes are needed

@rsc
Copy link
Contributor

@rsc rsc commented Jun 8, 2022

@drakkan I don't see where you renamed NewSignerWithAlgos. #52132 (comment) still says that. Can you post a new comment below with the full list of API changes? Thanks!

@drakkan
Copy link
Author

@drakkan drakkan commented Jun 8, 2022

@rsc I updated the patch implementing this proposal

https://go-review.googlesource.com/c/crypto/+/409215

here are the (public) API changes

// MultiAlgorithmSigner is an AlgorithmSigner that also reports the algorithms
// supported by that signer
type MultiAlgorithmSigner interface {
	AlgorithmSigner

	// Algorithms returns the available algorithms in preference order
	Algorithms() []string
}

// NewSignerWithAlgorithms returns a signer restricted to the specified algorithms.
// The algorithms must be set in preference order.
// An error is returned if the specified algorithms are incompatible with the
// public key type.
func NewSignerWithAlgorithms(signer AlgorithmSigner, algorithms []string) (Signer, error)

thanks

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Jun 9, 2022

Any reason for NewSignerWithAlgorithms not to return a MultiAlgorithmSigner?

@drakkan
Copy link
Author

@drakkan drakkan commented Jun 9, 2022

Any reason for NewSignerWithAlgorithms not to return a MultiAlgorithmSigner?

No particular reason. I will update the patch, thanks

@drakkan
Copy link
Author

@drakkan drakkan commented Jun 9, 2022

@FiloSottile patch updated, here are the updated API:

// MultiAlgorithmSigner is an AlgorithmSigner that also reports the algorithms
// supported by that signer
type MultiAlgorithmSigner interface {
	AlgorithmSigner

	// Algorithms returns the available algorithms in preference order
	Algorithms() []string
}

// NewSignerWithAlgorithms returns a signer restricted to the specified algorithms.
// The algorithms must be set in preference order.
// An error is returned if the specified algorithms are incompatible with the
// public key type.
func NewSignerWithAlgorithms(signer AlgorithmSigner, algorithms []string) (MultiAlgorithmSigner, error)

@rsc
Copy link
Contributor

@rsc rsc commented Jun 15, 2022

Thanks @drakkan. Does anyone object to this API?

@rsc rsc moved this from Active to Likely Accept in Proposals Jun 22, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jun 22, 2022

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

@rsc rsc moved this from Likely Accept to Accepted in Proposals Jul 1, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jul 1, 2022

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

@rsc rsc changed the title proposal: x/crypto/ssh: add ServerConfig.HostKeyAlgorithms x/crypto/ssh: add ServerConfig.HostKeyAlgorithms Jul 1, 2022
@rsc rsc removed this from the Proposal milestone Jul 1, 2022
@rsc rsc added this to the Backlog milestone Jul 1, 2022
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-FinalCommentPeriod
Projects
Proposals
Accepted
Development

No branches or pull requests

6 participants