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

tuf: add a method to retrieve rekor public keys #500

Closed
wants to merge 1 commit into from

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Jun 13, 2022

Signed-off-by: Asra Ali asraa@google.com

Summary

  • Adds a wrapper method to get rekor public keys, to use in cosign

I expect that with this method, we can now set sign/verify opts to include RekorPubKeys, so that people can define this themselves when they cosign as a library. This is how the fulcio roots works as well.

Ticket Link

Fixes

Release Note


return
}
for _, t := range targets {
rekorPubKey, err := cryptoutils.UnmarshalPEMToECDSAKey(t.Target)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would UnmarshalPEMToPublicKey work, or do we make more assumptions that the Rekor key is an ECDSA key?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I lost track of this comment -- yes! The methods that use Rekor public keys expect ecdsa keys: https://github.com/sigstore/cosign/blob/890cec1f43a8ec0754e0dd5a5d120847b63b6c4e/pkg/cosign/verify.go#L818

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, long-term, how hard would it be to remove the ECDSA requirement?

Signed-off-by: Asra Ali <asraa@google.com>

update

Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Jul 11, 2022

ping @haydentherapper or maybe this should just go in an internal cosign package?

@haydentherapper
Copy link
Contributor

haydentherapper commented Jul 11, 2022

I'd prefer this either live in Rekor or sigstore/sigstore. Given it's intertwined with TUF, I think sigstore/sigstore in the best option for now.

Long term, I think tlog verification should live in Rekor, certificate verification in Fulcio, TUF and crypto/signature/PEM/etc functions in sigstore/sigstore, OCI in Cosign.

return
}
for _, t := range targets {
rekorPubKey, err := cryptoutils.UnmarshalPEMToECDSAKey(t.Target)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, long-term, how hard would it be to remove the ECDSA requirement?

@@ -55,6 +55,19 @@ func UnmarshalPEMToPublicKey(pemBytes []byte) (crypto.PublicKey, error) {
return x509.ParsePKIXPublicKey(derBytes.Bytes)
}

// UnmarshalPEMToECDSAKey converts a PEM-encoded byte slice into an *ecdsa.PublicKey.
func UnmarshalPEMToECDSAKey(pemBytes []byte) (*ecdsa.PublicKey, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should make this a private function of rekorpubs? I'd prefer to encourage package users to use the more generic PEM to crypto.PublicKey method.


// GetRekorPubs returns a map of rekor public keys keyed by rekor log ID. Each key contains
// the ecdsa public key of the log and the status of the log (e.g. Active, Inactive).
func GetRekorPubs() (map[string]RekorPubKey, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also document that this caches the public keys. This could be an issue if used by a server that never restarts and never would fetch the latest keys for example.

Do you think we should have two functions, one for caching and one that doesn't cache?

@haydentherapper
Copy link
Contributor

@asraa Good to close in favor of sigstore-go?

Copy link
Contributor Author

@asraa asraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess it's not really quite needed yet anyway. sigstore-go will use TUF to populate the trust root proto

@asraa asraa closed this Dec 9, 2022
mtrmac pushed a commit to mtrmac/sigstore that referenced this pull request Mar 10, 2023
sigstore#500)

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants