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: ability to get underlying crypto.PublicKey from ssh.PublicKey #15989

Closed
mjgarton opened this issue Jun 7, 2016 · 13 comments
Closed
Milestone

Comments

@mjgarton
Copy link
Contributor

@mjgarton mjgarton commented Jun 7, 2016

I am using the ssh protocol in an atypical scenario and would like to perform additional checking in my PublicKeyCallback function. I would like to have access to the underlying crypto.PublicKey that sits behind the ssh.PublicKey. I can't find a clean way to achieve this currently.

Would it be acceptable to expose this in come way? I'm happy to submit a CL if so.

The two ways I can see would be a new method on ssh.PublicKey to expose it directly, or to export various types that implement ssh.PublicKey so that I can type assert to them and act accordingly.

Alternatively, if this is seen as breaking the intended encapsulation of ssh.PublicKey, I'd be interested in knowing other (evil, hacky) ways I might do so without modifying golang.org/x/crypto/ssh ?

@ianlancetaylor ianlancetaylor changed the title golang.org/x/crypto/ssh: Ability to get underlying crypto.PublicKey from ssh.PublicKey x/crypto/ssh: ability to get underlying crypto.PublicKey from ssh.PublicKey Jun 7, 2016
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Jun 7, 2016
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 7, 2016

@hanwen
Copy link
Contributor

@hanwen hanwen commented Jun 8, 2016

Can you tell me a little more of what you're trying to do?

The easiest solution is to call key.Marshal() and then deserialize the data yourself again.

@mjgarton
Copy link
Contributor Author

@mjgarton mjgarton commented Jun 9, 2016

I have an abstraction where the crypto.PublicKey forms the identity of hosts in my system. The fact that I'm using the ssh protocol internally to achieve a secure channel (and multiplexing) is intended to be hidden from most of the rest of my application, which wants to be told about crypto.PublicKeys and doesn't care about ssh at all.

I considered Marshal and deserialising myself, and I'll probably end up doing that if we think exposing the crypto.PublicKey is a bad idea, although it will mean duplicating some of the deserialisation logic.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Jun 9, 2016

ssh.PublicKey and ssh.Signer predate crypto.{PublicKey,Signer} (and probably were an inspiration.)

Previously, there wasn't a nice way to do this, but I think we could do this today. In fact, if we could implement it today NewSignerFromKey() should probably take a crypto.PrivateKey.

I think in principle it is OK to add a Key method to ssh.PublicKey returning the crypto.PublicKey.

However, since PublicKey is an interface, this potentially breaks compatibility with existing code. It probably doesn't in practice (since I think you can't write useful key implementations outside the SSH package). If you can make a case (with data) that this won't break anyone, I'm happy to accept such a change.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 9, 2016

I don't know if this is worth doing, but you can add a Key method to the underlying types without adding it to ssh.PublicKey. Then you can write something like (I don't know what the right names are here):

type UnderlyingKey interface {
    Key() interface{}
}

and callers can write publicKey.(ssh.UnderlyingKey).Key().

@mjgarton
Copy link
Contributor Author

@mjgarton mjgarton commented Jun 9, 2016

The trouble with NewSignerFromKey taking a crypto.PrivateKey is that it can currently accept dsa.PrivateKey which does not and can not implement crypto.Signer because it already has a Sign() method but with a different signature.
Because of this issue, which of course cannot be fixed because it's in the core libaries, I'm unsure what we can do here?

I guess ssh.PublicKey can still have a Key() method but it would have to return interface{} and leave it up to the caller to worry about its type? (if it doesn't break existing code of course)

@hanwen
Copy link
Contributor

@hanwen hanwen commented Jun 9, 2016

we don't have to do anything with privatekeys/signers, right? I was just giving an example.

crypto.PublicKey is interface{} so there's isn't a real difference between the two.

@mjgarton
Copy link
Contributor Author

@mjgarton mjgarton commented Jun 9, 2016

Ok.

What would you accept as "data" that this won't break anyone? If you give me some hints, I'd be happy to try to gather that info and submit a CL.

Also, @hanwen what do you think of @ianlancetaylor idea? Even if it doesn't end up being a solution here, thanks anyway @ianlancetaylor - that's a neat appoach.

@mjgarton
Copy link
Contributor Author

@mjgarton mjgarton commented Jun 10, 2016

I am going to work on a CL for this, since it's a pretty simple change. I'd still appreciate advice on how I might collect data to show that this change will not break anything.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Jun 10, 2016

doesn't godoc let you follow references? You could try to browse around callers of functions that take ssh.PublicKey and see if anybody creates their own. Or study the source code, and argue that it is impossible to create a useful public key outside of the SSH package (I think that might be true, because the SSH package has to deserialize keys when it receives them)

@mjgarton
Copy link
Contributor Author

@mjgarton mjgarton commented Jun 10, 2016

I agree with your logic, but I actually found 1 example in the wild that would break with my change: https://github.com/fudanchii/edssh

However, what it does is redundant and the only reason it implements ssh.PublicKey is to do something that more properly belongs in x/crypto/ssh and in fact is now there anyway, probably making github.com/fudanchii/edssh redundant now.

Other than that I could not find any implementations of ssh.PublicKey in the wild.

@dmitris
Copy link
Contributor

@dmitris dmitris commented Jan 17, 2017

https://go-review.googlesource.com/#/c/23974/ was merged and https://godoc.org/golang.org/x/crypto/ssh#CryptoPublicKey has been available for a while, let's close the issue.

\cc @mjgarton @hanwen

@agl agl closed this Jan 17, 2017
@golang golang locked and limited conversation to collaborators Jan 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.