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.

Sign up for GitHub

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

Jump to bottom

x/crypto/ssh: allow to configure public key authentication algorithms on the server side #61244

Closed
drakkan opened this issue Jul 9, 2023 · 8 comments
Closed

x/crypto/ssh: allow to configure public key authentication algorithms on the server side #61244

drakkan opened this issue Jul 9, 2023 · 8 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone
Backlog

Comments

@drakkan
Copy link
Member

drakkan commented Jul 9, 2023 •
edited

Currently supportedPubKeyAuthAlgos is hard-coded and contains all supported public key authentication algorithms.

Algorithms and signature formats not included in that list are not accepted by our server implementation.

I propose to make it configurable to allow to disable the weaker algorithms, for example ssh-dss or ssh-rsa.

A simple implementation would be to add them to the ServerConfig struct:

// ServerConfig holds server specific configuration data.
type ServerConfig struct {
	// Config contains configuration shared between client and server.
	Config
 
  // PublicKeyAuthAlgorithms specifies the supported client public key
  // authentication algorithms. Note that this should not include certificate
  // types since those use the underlying algorithm. This list is sent to the
  // client if it supports the server-sig-algs extension. Order is irrelevant.
  // If unspecified then a default set of algorithms is used.
  PublicKeyAuthAlgorithms []string
  ....
}

This is consistent with the way we currently allow to customize KeyExchanges, Ciphers and MACs.

An alternative to consider is to add a callback to the ServerConfig struct, for example

// ServerConfig holds server specific configuration data.
type ServerConfig struct {
   ...
   
   PublicKeyAuthAlgorithmsCallback func(ConnMetadata) []string
   ...
}

this would allow for more flexibility, for example allowing algorithms based on the client version, but if we are to go this route we should probably allow the same for KeyExchanges, Ciphers and MACs as well and it is probably better to think about something more generic using a single callback for all the customizable algorithms.
@gopherbot gopherbot added this to the Proposal milestone Jul 9, 2023
@ianlancetaylor
Copy link
Contributor

CC @golang/security

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

For #56561. Not a fan of yet another knob, but I don't have a better idea and LGTM.

@gopherbot
Copy link

Change https://go.dev/cl/510775 mentions this issue: ssh: allow to configure public key auth algorithms on the server side

@rsc
Copy link
Contributor

rsc commented Jul 19, 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

@rsc
Copy link
Contributor

rsc commented Jul 26, 2023

Are people generally in favor of this? Have all remaining concerns been addressed?

@hanwen
Copy link
Contributor

hanwen commented Jul 27, 2023

In short, I agree with Filippo: not pretty, but OK to add.

I propose to make it configurable to allow to disable the weaker algorithms, for example ssh-dss or ssh-rsa.

This is only about disabling RSA signatures on SHA1 digests? (You can disable ssh-dss by rejecting any keys in the PublicKeyCallback).

If I could redesign this from scratch, I'd pass the algorithm as an extra parameter to ServerConfig.PublicKeyCallback, then we wouldn't need this extra knob. The extra knob does have a slight performance advantage, because it also avoids an extra roundtrip where the client offers a public key only to have it be rejected.

@rsc
Copy link
Contributor

rsc commented Aug 9, 2023

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

@rsc
Copy link
Contributor

rsc commented Aug 16, 2023

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: allow to configure public key authentication algorithms on the server side x/crypto/ssh: allow to configure public key authentication algorithms on the server side Aug 16, 2023
@rsc rsc modified the milestones: Proposal, Backlog Aug 16, 2023
@drakkan drakkan mentioned this issue Sep 29, 2023
Open
@reedloden reedloden mentioned this issue Jan 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Proposals
Status: Accepted
Milestone
Backlog
Development

No branches or pull requests
6 participants
@hanwen @rsc @drakkan @FiloSottile @ianlancetaylor @gopherbot