Parsing ECDSA keys with an unsupported curve returns undescriptive error
When parsing an ECDSA public key with a unknown named curve, the error returned by ParseECPublicKeyFromPEM is ans1: structure error: tags don't match (...). This is not helpful as it suggest an error in the encoding of the key, which is not the case.
This happens here:
var parsedKey interface{}
if parsedKey, err = x509.ParsePKIXPublicKey(block.Bytes); err != nil {
if cert, err := x509.ParseCertificate(block.Bytes); err == nil {
parsedKey = cert.PublicKey
} else {
return nil, err
}
}
The error returned by x509.ParsePKIXPublicKey(block.Bytes), which describes the actual problem (x509: unsupported elliptic curve) is never propagated. Instead, the one that occurred while trying to parse the key as a certificate is returned.
Something similar happens when a key with an explicit curve is passed (x509.ParsePKIXPublicKey(block.Bytes) returns x509: failed to parse ECDSA parameters as named curve, which points to the underlying problem, but ParseECPublicKeyFromPEM propagates ans1: structure error: tags don't match (...) back).
Simply always returning the error returned by x509.ParsePKIXPublicKey(block.Bytes) instead of x509.ParseCertificate(block.Bytes) is probably not a good solution, as the same problem would occur, just the other way round.
I think the best solution would be to prefer propagating errors from the x509 package (starting with "x509:") over those from the ans1 package (starting with "ans1:"). I recommend this, as errors in the ans1 package are caused by mistakes in the encoding of the data and most users will probably not deal with keys at a level where this is the root cause of their issue. Errors in the x509package on the other hand are caused by using a properly formatted, but invalid key. This is, I think, more likely to happen.
I would propose the following (rules in order of precedence):
- Exactly one of the errors starts with "x509:" - Return the error starting with "x509:"
- Exactly one of the errors starts with "ans1:" - Return the error not starting with "ans1:"
- If both (or neither) errors start with "x509:"/"ans1:" - Not sure what to do here. I think it should be find to just default to either one.
I can implement this, but I wanted to wanted to see if someone has a better solution or sees any issues with my solution.
PS: #231 was probably a similar problem.