-
Notifications
You must be signed in to change notification settings - Fork 86
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
Fix PEM decoding issue for some KMS keys #175
Conversation
@tarakby Can you please have a look here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there a few changes needed, some of them are related to the original function that existed before and I didn't notice.
Happy to help make the changes.
crypto/crypto.go
Outdated
@@ -257,11 +267,31 @@ func DecodePublicKeyPEM(sigAlgo SignatureAlgorithm, s string) (PublicKey, error) | |||
} | |||
|
|||
goPublicKey := publicKey.(*ecdsa.PublicKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't noticed this before, we should test the type assertion here as other types of public keys may be returned.
goPublicKey, ok := publicKey.(*ecdsa.PublicKey)
if !ok {
return PublicKey{}, fmt.Error("only ECDSA public keys are supported")
}
crypto/crypto.go
Outdated
var expectedPointByteLength = map[SignatureAlgorithm]int{ | ||
crypto.ECDSAP256: 32, | ||
} | ||
|
||
// DecodePublicKeyHex decodes a PEM public key with the given signature algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// DecodePublicKeyHex decodes a PEM public key with the given signature algorithm. | |
// DecodePublicKeyHex decodes a PEM ECDSA public key with the given curve. |
crypto/crypto.go
Outdated
var expectedPointByteLength = map[SignatureAlgorithm]int{ | ||
crypto.ECDSAP256: 32, | ||
} | ||
|
||
// DecodePublicKeyHex decodes a PEM public key with the given signature algorithm. | ||
func DecodePublicKeyPEM(sigAlgo SignatureAlgorithm, s string) (PublicKey, error) { | ||
block, _ := pem.Decode([]byte(s)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't have trailing data here.
block, _ := pem.Decode([]byte(s)) | |
block, rest := pem.Decode([]byte(s)) | |
if len(rest)>0 { | |
return PublicKey{}, fmt.Errorf("crypto: failed to parse PEM string: %w", err) | |
} |
goPublicKey.X.Bytes(), | ||
goPublicKey.Y.Bytes()..., | ||
) | ||
xBytes := goPublicKey.X.Bytes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using ParsePKIXPublicKey
only works in the case of curve P256. We will need to write a local ParsePKIXPublicKey
function that supports secp256k1. I can help writing it.
crypto/crypto.go
Outdated
) | ||
xBytes := goPublicKey.X.Bytes() | ||
yBytes := goPublicKey.Y.Bytes() | ||
expectedLength := expectedPointByteLength[sigAlgo] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to get that length this way:
expectedLength := expectedPointByteLength[sigAlgo] | |
expectedLength := bitsToBytes(goPublicKey.Params().P.BitLen()) |
With bitsToBytes
the function converting bit size into byte size:
func bitsToBytes(bits int) int {
return (bits + 7) >> 3
}
crypto/crypto.go
Outdated
yBytes := goPublicKey.Y.Bytes() | ||
expectedLength := expectedPointByteLength[sigAlgo] | ||
var rawPublicKey []byte | ||
if expectedLength > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this check and pad the expected length in all cases.
crypto/crypto.go
Outdated
// we make sure to left pad it to the right length. This is a mapping of the | ||
// signature algorithms to the expected length of the byte slices representing | ||
// the X and Y points | ||
var expectedPointByteLength = map[SignatureAlgorithm]int{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually, we won't need the map.
Closes: N/A
Description
Was running into a PEM decode issue when working with some KMS keys. Kept getting
cryptokms: failed to parse PEM public key: input has incorrect ECDSA_P256 key size
.The cause of this is sometimes, when calling
.Bytes()
on the X and Y points after decoding the PEM key, the corresponding byte slices were not the full 32 bytes, which was expected by the next function (expecting ECDSA_P256 keys to have a raw key of length 64)This adds left padding (since the bytes are returned big endian) up to the expected length for ECDSA_P256 only. Can add other known lengths as we start to use them more.
For contributor use:
master
branchFiles changed
in the Github PR explorer