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
Comments
CC @golang/security |
This proposal has been added to the active column of the proposals project |
/cc @FiloSottile and @rolandshoemaker for thoughts |
The compatible way to do this would be to define
|
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. |
my proposal is
Regarding
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 |
What do W and R stand for and are those the right names? |
Oh, sure. How about:
We could also do
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 |
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 In terms of the shape of |
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
Do I have that right? |
Yup, sounds right. |
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. |
This seems like a case where Perhaps we should add a concrete type, and say that the |
yes, I was a young and innocent Go developer :-)
I like that idea better than the extended interfaces, but it will be a bit of work to add the type. Maybe |
If we use #58523 (comment), have all concerns been addressed? |
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 |
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? |
Are there a bunch more methods that will need to be added? |
/cc @drakkan and @FiloSottile for thoughts |
[edit, because little baby hands pressed return]
When I implemented this, I noted,
Disconnect isn't very useful (just closing the network connection works just as well). I'm more worried by things like #61447, which says
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. |
The type should be Does it make sense to treat all the discussions on algorithms together, that is: |
The use case here is getting negotiated algorithms after successful authentication, and do something if a particular client is using an old/deprecated algorithm. 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 |
Is this one the final proposal?
|
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. |
@hanwen you are right, so we should do something liks this
do you agree? |
👍 (modulo nits: Kex vs KEX vs KeyExchange) |
Change https://go.dev/cl/538235 mentions this issue: |
Have all remaining concerns about this proposal been addressed? The proposal is to add
The canonical casing of Kex vs KEX should be decided, and it should match the decision in #61537. |
can we use |
Based on the discussion above, this proposal seems like a likely accept. The proposal is to add
|
@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
we can also implement as described above and |
Maybe we can also use |
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 |
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. |
No change in consensus, so accepted. 🎉 The proposal is to add
|
Hi, this feature will be super useful, any updates on when it will be merged? |
Fixes golang/go#58523 Change-Id: Ic64bd2fdd6e9ec96acac3ed4be842e2fbb15231d
Fixes golang/go#58523 Change-Id: Ic64bd2fdd6e9ec96acac3ed4be842e2fbb15231d
Folks running production servers are under opposing pressures, to both
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:
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 addfunc 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
struct
s instead. They are not meant to be implemented by other package, so I think it's OK to add this method to the interface.The text was updated successfully, but these errors were encountered: