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

Add parsing for NSS-wrapped Ed25519 keys #15742

Merged
merged 3 commits into from
Jun 6, 2022
Merged

Conversation

cipherboy
Copy link
Contributor

NSS wraps Ed25519 using the PKCS#8 standard structure. The Go standard
library as of Go 1.18.x doesn't support parsing this key type with the
OID used by NSS; it requires the 1.3.101.112/RFC 8410 format, rather
than the RFC 5915-esque structure supported here.

Co-authored-by: Rachel Culpepper <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>

NSS wraps Ed25519 using the PKCS#8 standard structure. The Go standard
library as of Go 1.18.x doesn't support parsing this key type with the
OID used by NSS; it requires the 1.3.101.112/RFC 8410 format, rather
than the RFC 5915-esque structure supported here.

Co-authored-by: Rachel Culpepper <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy added this to the 1.12.0-rc1 milestone Jun 1, 2022
@cipherboy cipherboy requested review from schultz-is and a team June 1, 2022 20:11
cipherboy and others added 2 commits June 1, 2022 16:11
Co-authored-by: Rachel Culpepper <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
Co-authored-by: Rachel Culpepper <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
@@ -1385,7 +1385,17 @@ func (p *Policy) Import(ctx context.Context, storage logical.Storage, key []byte
} else {
parsedPrivateKey, err := x509.ParsePKCS8PrivateKey(key)
if err != nil {
return fmt.Errorf("error parsing asymmetric key: %s", err)
if strings.Contains(err.Error(), "unknown elliptic curve") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little brittle against possible future changes from an error message?

Are there any reasons we don't just try to call ParsePKCS8Ed25519PrivateKey on any err?

Copy link
Contributor Author

@cipherboy cipherboy Jun 2, 2022

Choose a reason for hiding this comment

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

We might have a bad ASN.1 blob or something else, that'd trigger a non-unknown curve error. If that's the case, the Ed25519 parsing won't do anything, but give a weird, unrelated error message (if you say, had a corrupted RSA key, the Ed25519 would be weird).

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright I suppose the unit test will catch the change if they ever do update the error message.

Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

Overall this seems good to me, but another pair of eyes is highly recommended before merge.

@cipherboy cipherboy requested a review from a team June 3, 2022 13:45
Copy link
Collaborator

@sgmiller sgmiller left a comment

Choose a reason for hiding this comment

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

lgtm!

@cipherboy
Copy link
Contributor Author

Thanks all! Merging...

@cipherboy cipherboy merged commit da2fd89 into main Jun 6, 2022
@cipherboy cipherboy deleted the cipherboy-ed25519-nss-compat branch June 16, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants