jwt-go icon indicating copy to clipboard operation
jwt-go copied to clipboard

Parsing ECDSA keys with an unsupported curve returns undescriptive error

Open LogicalOverflow opened this issue 5 years ago • 0 comments

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.

LogicalOverflow avatar Jun 04 '20 12:06 LogicalOverflow