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: implement CryptoPublicKey on sk keys #62518

Closed
maraino opened this issue Sep 8, 2023 · 11 comments
Closed

x/crypto/ssh: implement CryptoPublicKey on sk keys #62518

maraino opened this issue Sep 8, 2023 · 11 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone
Unreleased

Comments

@maraino
Copy link

maraino commented Sep 8, 2023

The public keys in the x/crypto/ssh package implement the following interface:

type CryptoPublicKey interface {
	CryptoPublicKey() crypto.PublicKey
}

These methods return the underlying key for RSA, ECDSA, Ed25519, and DSA public keys. However, the ecdsa-sk and ed25519-sk keys do not implement that interface, forcing a user to unmarshal the serialized data to a struct to be able to get the underlying keys.

As the skECDSAPublicKey and skEd25519PublicKey already contain the key, the implementation would look like this:

func (k *skECDSAPublicKey) CryptoPublicKey() crypto.PublicKey {
	return &k.PublicKey
}

func (k *skEd25519PublicKey) CryptoPublicKey() crypto.PublicKey {
	return k.PublicKey
}

To be consistent with the rest of the CryptoPublicKey implementations, these methods will return the actual key, instead of a copy of it.
@gopherbot gopherbot added this to the Proposal milestone Sep 8, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/526875 mentions this issue: ssh: implement CryptoPublicKey on sk keys

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Sep 8, 2023
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Sep 8, 2023
@ianlancetaylor
Copy link
Member

CC @golang/security @drakkan

@drakkan
Copy link
Member

drakkan commented Sep 12, 2023

It seems ok to me

@hanwen
Copy link
Contributor

hanwen commented Sep 21, 2023

LGTM

@drakkan
Copy link
Member

drakkan commented Sep 21, 2023

Maybe we can do the same for the agent as well.

If you agree I can send a separate CL for this.

@maraino
Copy link
Author

maraino commented Sep 21, 2023 •
edited
Loading

Maybe we can do the same for the agent as well.

If you agree I can send a separate CL for this.

Although it shouldn't, the main reason I haven't done it is because ssh.ParsePublicKey(k.Blob) can fail

@drakkan
Copy link
Member

drakkan commented Sep 24, 2023

Maybe we can do the same for the agent as well.
If you agree I can send a separate CL for this.

Although it shouldn't, the main reason I haven't done it is because ssh.ParsePublicKey(k.Blob) can fail

I mean something like this

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

however I don't want to block the proposal for this, we can discuss the above CL separately

haydentherapper added a commit to haydentherapper/rekor that referenced this issue Sep 26, 2023
@haydentherapper
Fix panic when parsing SSH SK pubkeys
cd3af98 
This is because these pubkey types do not implement
ssh.CryptoPublicKey, which causes a panic when we try to do
a type assertion.

Also realized we weren't handling SSH certs, so now we extract the
pubkey from the cert before trying to parse it.

Had to reimplement parsing the SK pubkeys because there is no other way
to extract the raw pubkey from it. After golang/go#62518,
this will get cleaned up.

Signed-off-by: Hayden Blauzvern <[email protected]>
@haydentherapper haydentherapper mentioned this issue Sep 26, 2023
Merged
haydentherapper added a commit to haydentherapper/rekor that referenced this issue Sep 26, 2023
@haydentherapper
Fix panic when parsing SSH SK pubkeys
55389da 
This is because these pubkey types do not implement
ssh.CryptoPublicKey, which causes a panic when we try to do
a type assertion.

Also realized we weren't handling SSH certs, so now we extract the
pubkey from the cert before trying to parse it.

Had to reimplement parsing the SK pubkeys because there is no other way
to extract the raw pubkey from it. After golang/go#62518,
this will get cleaned up.

Signed-off-by: Hayden Blauzvern <[email protected]>
bobcallaway pushed a commit to sigstore/rekor that referenced this issue Sep 26, 2023
@haydentherapper
Fix panic when parsing SSH SK pubkeys (#1712)
21102e6 
This is because these pubkey types do not implement
ssh.CryptoPublicKey, which causes a panic when we try to do
a type assertion.

Also realized we weren't handling SSH certs, so now we extract the
pubkey from the cert before trying to parse it.

Had to reimplement parsing the SK pubkeys because there is no other way
to extract the raw pubkey from it. After golang/go#62518,
this will get cleaned up.

Signed-off-by: Hayden Blauzvern <[email protected]>
@FiloSottile
Copy link
Contributor

This looks extremely uncontroversial, ignoring the agent implementation for now.

@golang/proposal-review, can we get it straight into likely accept maybe?

@rsc
Copy link
Contributor

rsc commented Mar 8, 2024

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 Mar 8, 2024
@rsc rsc changed the title proposal: x/crypto/ssh: Implement CryptoPublicKey on sk keys proposal: x/crypto/ssh: implement CryptoPublicKey on sk keys Mar 13, 2024
@rsc rsc moved this from Active to Likely Accept in Proposals Mar 15, 2024
@rsc
Copy link
Contributor

rsc commented Mar 15, 2024

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

The proposal details are in #62518 (comment).

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 27, 2024
@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

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

The proposal details are in #62518 (comment).

@rsc rsc changed the title proposal: x/crypto/ssh: implement CryptoPublicKey on sk keys x/crypto/ssh: implement CryptoPublicKey on sk keys Mar 27, 2024
@rsc rsc modified the milestones: Proposal, Backlog Mar 27, 2024
@dmitshur dmitshur modified the milestones: Backlog, Unreleased May 30, 2024
@gabyhelp gabyhelp mentioned this issue Oct 16, 2024
Open
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
Unreleased
Development

No branches or pull requests
8 participants
@hanwen @rsc @maraino @drakkan @FiloSottile @dmitshur @ianlancetaylor @gopherbot