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

crypto/x509: return informative error if wrong type passed to MarshalPKIXPublicKey #32640

Open
mathieudevos opened this issue Jun 16, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@mathieudevos
Copy link

commented Jun 16, 2019

What version of Go are you using (go version)?

$ go version
go1.12.5

Does this issue reproduce with the latest release?

Not applicable

What did you do?

What did you expect to see?

Expected to see an error stating it requires a pointer, or for it to resolve instead

What did you see instead?

panic: x509: only RSA and ECDSA public keys supported

My suggestion, in https://golang.org/src/crypto/x509/x509.go?s=3023:3081#L68 , the function marshalPublicKey add 2 cases with simple recursion:

case rsa.PublicKey:
    return marshalPublicKey(&pub)
case ecdsa.PublicKey
    return marshalPublicKey(&pub)

Or same as above, but return error that states that it must be a pointer.

@gopherbot gopherbot added this to the Proposal milestone Jun 16, 2019

@gopherbot gopherbot added the Proposal label Jun 16, 2019

@FiloSottile FiloSottile changed the title proposal: crypto/x509 MarshalPKIXPublicKey to handle non-pointer keys crypto/x509: return informative error if wrong type passed to MarshalPKIXPublicKey Jun 17, 2019

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

It's already confusing enough without letting some functions take the non-pointer type, but we should return a better error.

@FiloSottile FiloSottile modified the milestones: Proposal, Go1.14 Jun 17, 2019

@mathieudevos

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

It's already confusing enough without letting some functions take the non-pointer type, but we should return a better error.

Actually, the more I read about this, the more I'm confused, there are already Marshaling functions for public keys from RSA (see: MarshalPKCS1PublicKey). But the ECDSA one is overloaded.

Can we just add a function called MarshalECPublicKey (there is MarshalECPrivateKey) which throws an error if you give in a non-pointer type.

It seems that MarshalPKIXPublicKey is overloaded with both RSA and ECDSA public keys, while there is already a func to marshal RSA public keys.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

That's because RSA keys have an RSA-specific format, PKCS#1, while PKIX is a format that can encode both RSA and ECDSA. That is also a mess, but it's its own mess that has little to do with pointer types, and it's due to what's specified, not what's implemented in Go.

Leaving this issue to be about the better errors, see #32541 (comment) if you want to open a proposal for an EC-only encoding function.

@mathieudevos

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

Proposed to mark this as deprecated and rewrite it with the proposed new correctly-typed functions.

Proposal can be found here: #32657

Issue with increasing verbosity in this function is that we would need to use reflection to figure out what the user is actually sending us. That is way outside the scope of this function. The proposal aims to increase type-safety and usability.

Not sure which way we go with this, but if we go with the proposal, this can be closed.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

No need for reflection, just a type assertion will do.

@mathieudevos

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

In that case to make this slightly more verbose, I would propose the following:

- return nil, pkix.AlgorithmIdentifier{}, errors.New("x509: only RSA and ECDSA public keys supported")
+ return nil, pkix.AlgorithmIdentifier{}, fmt.Errorf("x509: unsupported type: %T, only RSA and ECDSA public key supported", pub)

This does not solve the issue that non-pointer values will cause confusion, as such I would either write custom error functions for them or use recursion to handle them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.