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: expose negotiated algorithms #58523

Open
hanwen opened this issue Feb 14, 2023 · 40 comments
Open

x/crypto/ssh: expose negotiated algorithms #58523

hanwen opened this issue Feb 14, 2023 · 40 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@hanwen
Copy link
Contributor

hanwen commented Feb 14, 2023

Folks running production servers are under opposing pressures, to both

  1. stop supporting obsolete or vulnerable algorithms and/or key sizes.
  2. keep serving customers and migrate them away from obsolete algorithms

they can only make informed decisions on what to do (reach out to customers, switch off algorithms etc.) if they have data which algorithms are actually in use.

For the TLS library, I think this can be achieved by inspecting https://pkg.go.dev/crypto/tls#ConnectionState.

The SSH library doesn't expose this information, more in particular:

  • Cipher, MAC, Compression in read/write directions
  • host key algorithm
  • key exchange algorithm.

this information is captured in type algorithms
https://cs.opensource.google/go/x/crypto/+/master:ssh/common.go;l=185;drc=6fad3dfc18918c2ac9c112e46b32473bd2e5e2f9

the proposal is to export type algorithms, and add

func Algorithms() Algorithms

to the ssh.Conn type.

This is technically an incompatible API change. However, the ssh.Conn (as well as ssh.Channel) are misdesigned, and should have been structs instead. They are not meant to be implemented by other package, so I think it's OK to add this method to the interface.

@gopherbot gopherbot added this to the Proposal milestone Feb 14, 2023
@seankhliao seankhliao changed the title proposal: x/crypto/ssh - expose negotiated algorithms. proposal: x/crypto/ssh: expose negotiated algorithms. Feb 14, 2023
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Feb 14, 2023
@ianlancetaylor ianlancetaylor changed the title proposal: x/crypto/ssh: expose negotiated algorithms. proposal: x/crypto/ssh: expose negotiated algorithms Feb 14, 2023
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@rsc
Copy link
Contributor

rsc commented Mar 29, 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 Apr 5, 2023

/cc @FiloSottile and @rolandshoemaker for thoughts

@rsc
Copy link
Contributor

rsc commented May 30, 2023

The compatible way to do this would be to define

type AlgorithmsConn interface {
    Conn
    Algorithms() Algorithms
}

@rsc
Copy link
Contributor

rsc commented May 30, 2023

Talked to @FiloSottile. In general we are supportive of expanding things here. The algorithms struct right now has lots of other unexported fields. It is unclear to us what exactly the public API for Algorithms would be.

@hanwen
Copy link
Contributor Author

hanwen commented Jun 5, 2023

my proposal is

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
	Compression string
}
type Algorithms struct {
	Kex     string
	HostKey string
	W       DirectionAlgorithms
	R       DirectionAlgorithms
}

Regarding

type AlgorithmsConn interface {
    Conn
    Algorithms() Algorithms
}

This is backward compatible, but runs the risk that we'd create a new interface for every bit of info we'd want to add down the line.

Maybe we could do func AlgorithmsOf(c Conn) *Algorithms instead?

@rsc
Copy link
Contributor

rsc commented Jun 7, 2023

What do W and R stand for and are those the right names?

@hanwen
Copy link
Contributor Author

hanwen commented Jun 12, 2023

Oh, sure. How about:

type Algorithms struct {
	Kex     string
	HostKey string
	Write       DirectionAlgorithms
	Read       DirectionAlgorithms
}

We could also do

type Algorithms struct {
	Kex     string
	HostKey string
	ClientToServer       DirectionAlgorithms
	ServerToClient       DirectionAlgorithms
}

which is probably a bit more self-explanatory for callers, but it means slightly more complexity on the implementation side.

Also, I think callers will typically only be implementing either the client or the server, so Read and Write are sufficiently unambiguous.

@rolandshoemaker
Copy link
Member

There does seem to be a bunch of stuff we'd like to expose down the road, and expanding the Conn interface each time seems unideal. I'd probably vote for something like func AlgorithmsFor(Conn) Algorithms instead.

In terms of the shape of Algorithms I think just exporting all of the currently private fields seems fine. I don't think the ClientToServer/ServerToClient names are particularly clearer. I think it'd probably be simpler to just expand w and r to Write and Read, which paired with the Direction part of DirectionAlgorithms I think makes it clear what they represent.

@rsc
Copy link
Contributor

rsc commented Jun 21, 2023

Re AlgorithmsConn vs AlgorithmsFor(Conn), we've established the AlgorithmsConn "extension interface" pattern in other contexts already, specifically the fs.FS interface and its extension interfaces, so I think that's probably okay. So it sounds like

type Algorithms struct {
	Kex     string
	HostKey string
	Read       DirectionAlgorithms
	Write       DirectionAlgorithms
}

type AlgorithmsConn interface {
	Conn
	Algorithms() Algorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
	Compression string
}

Do I have that right?

@rolandshoemaker
Copy link
Member

Yup, sounds right.

@hanwen
Copy link
Contributor Author

hanwen commented Jun 21, 2023

Let me look around the bug tracker to see if there are other kind of things we could potentially expose (one thing that comes to mind is wiring Context through the SSH package) to see if we could do a combined update.

@neild
Copy link
Contributor

neild commented Jun 21, 2023

This seems like a case where ssh.Conn should have been a concrete type to permit future expansion.

Perhaps we should add a concrete type, and say that the Conns returned by ssh package constructors can always be type asserted to *ssh.SSHConn? That would avoid the need to add a new interface every time we think of something that should be exposed.

@hanwen
Copy link
Contributor Author

hanwen commented Jun 21, 2023

This seems like a case where ssh.Conn should have been a concrete type to permit future expansion.

yes, I was a young and innocent Go developer :-)

Perhaps we should add a concrete type, and say that the Conns returned by ssh package constructors can always be type asserted to *ssh.SSHConn? That would avoid the need to add a new interface every time we think of something that should be exposed.

I like that idea better than the extended interfaces, but it will be a bit of work to add the type. Maybe ssh.Connection (to avoid the stutter.)

@rsc
Copy link
Contributor

rsc commented Jul 5, 2023

If we use #58523 (comment), have all concerns been addressed?

@hanwen
Copy link
Contributor Author

hanwen commented Jul 5, 2023

I wanted to do something like https://go-review.googlesource.com/c/crypto/+/507779

then we can add the algorithms as a normal method on Connection

@rsc
Copy link
Contributor

rsc commented Jul 12, 2023

Having both Conn and Connection doesn't sound right, even as a way to deal with legacy questions.

@hanwen
Copy link
Contributor Author

hanwen commented Jul 13, 2023

Having both Conn and Connection doesn't sound right, even as a way to deal with legacy questions.

aside from naming details, are you saying it is better that we should add a new interface for every method we add?

@rsc
Copy link
Contributor

rsc commented Jul 19, 2023

Are there a bunch more methods that will need to be added?

@rsc
Copy link
Contributor

rsc commented Jul 26, 2023

/cc @drakkan and @FiloSottile for thoughts

@hanwen
Copy link
Contributor Author

hanwen commented Jul 26, 2023

[edit, because little baby hands pressed return]

Are there a bunch more methods that will need to be added?

When I implemented this, I noted,

        // TODO(hanwen): consider exposing:
        //   RequestKeyChange
        //   Disconnect

Disconnect isn't very useful (just closing the network connection works just as well). RequestKeyChange shouldn't be necessary for end-users, as we schedule a key change according to predefined limits
for the negotiated algorithms. We could probably also implement by implementing a SendRequest message that is interpreted locally. That works but may be a bit hacky.

I'm more worried by things like #61447, which says

Add the following to ConnMetadata interface

In general, the server-side authentication (and logging of success/failure) was not thought out in depth, and I'm not sure we've seen the end of changes here.

Also, come to think of it, the algorithms are decided just before the user authentication, which is to say that the Algorithms should be part of ssh.ConnMetadata, not ssh.Conn.

@hanwen hanwen closed this as completed Jul 26, 2023
@hanwen hanwen reopened this Jul 26, 2023
@hanwen
Copy link
Contributor Author

hanwen commented Jul 27, 2023

The type should be type NegotiatedAlgorithms struct... to distinguish from algorithms supported by the package.

Does it make sense to treat all the discussions on algorithms together, that is:

@drakkan
Copy link
Member

drakkan commented Jul 27, 2023

The use case here is getting negotiated algorithms after successful authentication, and do something if a particular client is using an old/deprecated algorithm.
I think it doesn't makes much difference whether the algorithms are part of ssh.ConnMetadata or ssh.Conn as long as the application can get them.

Something interesting we could consider, since we are discussing about this part, is exposing an API to allow algorithms based on client/server version.

Take a look here for example.

It seems that when we call findAgreedAlgorithms we already have both the client and server version.
I think this is a fairly niche use case, but if you think it's interesting I can try to do a more in-depth analysis and open a new related proposal.

@drakkan
Copy link
Member

drakkan commented Aug 3, 2023

Is this one the final proposal?

type NegotiatedAlgorithms struct {
	Kex     string
	HostKey string
	Read    DirectionAlgorithms
	Write   DirectionAlgorithms
}

type NegotiatedAlgorithmsConn interface {
	Conn
	Algorithms() NegotiatedAlgorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
	Compression string
}

@hanwen
Copy link
Contributor Author

hanwen commented Aug 3, 2023

The use case here is getting negotiated algorithms after successful authentication, and do something if a particular client is using an old/deprecated algorithm.

if the algorithm choice is available at auth time, I am sure eventually someone will want to have it there. There's even an obvious use-case: you could show a "You are using a deprecated algortihm XYZ" banner as part of auth flow.

@drakkan
Copy link
Member

drakkan commented Aug 3, 2023

@hanwen you are right, so we should do something liks this

type NegotiatedAlgorithms struct {
	Kex     string
	HostKey string
	Read    DirectionAlgorithms
	Write   DirectionAlgorithms
}

type AlgorithmsConnMetadata interface {
	ConnMetadata
	Algorithms() NegotiatedAlgorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
	Compression string
}

do you agree?

@rsc
Copy link
Contributor

rsc commented Aug 9, 2023

@hanwen, what do you think of @drakkan's last message?

@hanwen
Copy link
Contributor Author

hanwen commented Sep 28, 2023

👍

(modulo nits: Kex vs KEX vs KeyExchange)

@gopherbot
Copy link

Change https://go.dev/cl/538235 mentions this issue: ssh: expose negotiated algorithms

@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

Have all remaining concerns about this proposal been addressed?

The proposal is to add

type NegotiatedAlgorithms struct {
	Kex     string // or maybe KEX
	HostKey string
	Read    DirectionAlgorithms
	Write   DirectionAlgorithms
}

type AlgorithmsConnMetadata interface {
	ConnMetadata
	Algorithms() NegotiatedAlgorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
	Compression string
}

The canonical casing of Kex vs KEX should be decided, and it should match the decision in #61537.

@drakkan
Copy link
Member

drakkan commented Nov 2, 2023

can we use KeyExchange instead of Kex as discussed in #61537 ?

@rsc
Copy link
Contributor

rsc commented Nov 10, 2023

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

The proposal is to add

type NegotiatedAlgorithms struct {
	KeyExhcange string 
	HostKey string
	Read    DirectionAlgorithms
	Write   DirectionAlgorithms
}

type AlgorithmsConnMetadata interface {
	ConnMetadata
	Algorithms() NegotiatedAlgorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
	Compression string
}

@drakkan
Copy link
Member

drakkan commented Nov 10, 2023

@rsc sorry for my late reply here. I think we should consider removing compression since we decided to remove it in #61537

If you agree the proposal will be

type NegotiatedAlgorithms struct {
	KeyExchange string 
	HostKey string
	Read    DirectionAlgorithms
	Write   DirectionAlgorithms
}

type AlgorithmsConnMetadata interface {
	ConnMetadata
	Algorithms() NegotiatedAlgorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
}

we can also implement as described above and Compression will be always none

@drakkan
Copy link
Member

drakkan commented Nov 14, 2023

Maybe we can also use HostKeyAlgorithm instead of HostKey. In #61537 we have HostKeyAlgorithms

@rsc
Copy link
Contributor

rsc commented Nov 14, 2023

Probably 61537 is wrong and we should drop Algorithms from the remaining struct fields in 61537. It is strange to have it in some fields but not others, and the overall struct is named Algorithms so having that word in the fields is redundant.

@drakkan
Copy link
Member

drakkan commented Nov 15, 2023

Probably 61537 is wrong and we should drop Algorithms from the remaining struct fields in 61537. It is strange to have it in some fields but not others, and the overall struct is named Algorithms so having that word in the fields is redundant.

We already have HostKeyAlgorithms in ClientConfig and PublicKeyAuthAlgorithms in ServerConfig, keeping the same name may avoid confusion even if Algorithm is redundant, anyway we can also improve docs to avoid any confusion

@rsc
Copy link
Contributor

rsc commented Nov 15, 2023

I still think we should drop Algorithms inside the Algorithms struct. It is defensible to have them in ClientConfig and ServerConfig since that is not ClientConfigAlgorithms and ServerConfigAlgorithms.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2023

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

The proposal is to add

type NegotiatedAlgorithms struct {
	KeyExchange string 
	HostKey string
	Read    DirectionAlgorithms
	Write   DirectionAlgorithms
}

type AlgorithmsConnMetadata interface {
	ConnMetadata
	Algorithms() NegotiatedAlgorithms
}

type DirectionAlgorithms struct {
	Cipher      string
	MAC         string
}

@rsc rsc changed the title proposal: x/crypto/ssh: expose negotiated algorithms x/crypto/ssh: expose negotiated algorithms Dec 4, 2023
@rsc rsc modified the milestones: Proposal, Backlog Dec 4, 2023
@drakkan
Copy link
Member

drakkan commented Dec 9, 2023

Thanks for accepting this proposal!
I rebased and adapted the CL that implements it. It depends on #61537 and approval of #54743 should also be discussed to avoid to have KEX algorithms supported only on the client side

@onemorezero
Copy link

Hi, this feature will be super useful, any updates on when it will be merged?

drakkan added a commit to drakkan/crypto that referenced this issue Feb 24, 2024
Fixes golang/go#58523

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

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

8 participants