jwt icon indicating copy to clipboard operation
jwt copied to clipboard

Code comment for jwt.ParseRSAPublicKeyFromPEM incorrect

Open twocs opened this issue 4 years ago • 6 comments

The code comment for rsa_utils.go says it's for a PKCS1 or PKCS8 public key (ParseRSAPublicKeyFromPEM parses a PEM encoded PKCS1 or PKCS8 public key ParseRSAPublicKeyFromPEM parses a PEM encoded PKCS1 or PKCS8 public key), but the function uses x509.ParsePKIXPublicKey instead. I was unable to get a PKCS1 public key despite the code comment, and basically just copied the entire function and swapped it from x509.ParsePKIXPublicKey to x509.ParsePKCS1PublicKey, which works. I am not clear but it seems that it's not matching the code comment and it took me quite some time to figure out why I couldn't parse my RSA Public Key (it wasn't accepted on jwt.io).

I suspect that the problem is not with the code comment, but with the implementation. Or am I doing something wrong that the parsing of a pem with PKCS1 encoding doesn't work using x509.ParsePKIXPublicKey but does work with x509.ParsePKCS1PublicKey?

https://github.com/golang-jwt/jwt/blob/2ebb50f957d606de5909fcf9ed49f9af3bc35e97/rsa_utils.go#L79

// ParseRSAPublicKeyFromPEM parses a PEM encoded PKCS1 or PKCS8 public key
func ParseRSAPublicKeyFromPEM(key []byte) (*rsa.PublicKey, error) {
	var err error

	// Parse PEM block
	var block *pem.Block
	if block, _ = pem.Decode(key); block == nil {
		return nil, ErrKeyMustBePEMEncoded
	}

	// Parse the key
	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
		}
	}

If we compare to https://github.com/golang-jwt/jwt/blob/2ebb50f957d606de5909fcf9ed49f9af3bc35e97/rsa_utils.go#L17 private key explicitly tries both functions.

	if parsedKey, err = x509.ParsePKCS1PrivateKey(block.Bytes); err != nil {
		if parsedKey, err = x509.ParsePKCS8PrivateKey(block.Bytes); err != nil {
			return nil, err
		}
	}

twocs avatar Nov 04 '21 23:11 twocs

I think the comment of this function is wrong. If you look into the implementation of ParsePKIXPublicKey it actually refers to ParsePKCS1PublicKey if it fails to parse the key.

https://cs.opensource.google/go/go/+/refs/tags/go1.17.3:src/crypto/x509/x509.go;drc=ec4efa42089415b0427a4d30b317cfd7e4a0fe75;l=58

So if you want to support both, we probably need to do something similar to ParseRSAPrivateKeyFromPEM.

Would you be interesting in supplying a PR to support this?

oxisto avatar Nov 05 '21 09:11 oxisto

Thanks for the feedback. I'll submit a PR.

twocs avatar Nov 05 '21 11:11 twocs

What if we leveraged this crypto/ssh.ParseRawPrivateKey function for parsing a private key? Which does the pem decoding and then switches on the type.

https://pkg.go.dev/golang.org/x/crypto/ssh#ParseRawPrivateKey

Source code: https://cs.opensource.google/go/x/crypto/+/089bfa56:ssh/keys.go;l=1117-1145

mfridman avatar Nov 05 '21 12:11 mfridman

What if we leveraged this crypto/ssh.ParseRawPrivateKey function for parsing a private key? Which does the pem decoding and then switches on the type.

https://pkg.go.dev/golang.org/x/crypto/ssh#ParseRawPrivateKey

Source code: https://cs.opensource.google/go/x/crypto/+/089bfa56:ssh/keys.go;l=1117-1145

It's the public key parsing we need :)

oxisto avatar Nov 05 '21 13:11 oxisto

What if we leveraged this crypto/ssh.ParseRawPrivateKey function for parsing a private key? Which does the pem decoding and then switches on the type. https://pkg.go.dev/golang.org/x/crypto/ssh#ParseRawPrivateKey Source code: https://cs.opensource.google/go/x/crypto/+/089bfa56:ssh/keys.go;l=1117-1145

It's the public key parsing we need :)

Ugh, apologies, should have read the issue a bit closer. 😊

mfridman avatar Nov 05 '21 13:11 mfridman

The test of jwt.ParseRSAPublicKeyFromPEM uses file https://github.com/golang-jwt/jwt/blob/main/test/sample_key.pub

Switching to x509.ParsePKCS1PublicKey results in the following failure: 2021/11/06 20:49:32 x509: failed to parse public key (use ParsePKIXPublicKey instead for this key format)

Looks like the unit test relies on PKIX public key so need to fix both the comment and add PKCS1 support.

twocs avatar Nov 06 '21 12:11 twocs

I was lucky to have found sample code for my use case, keycloak, here.

Without that, odds are I'd still be trying to get things working. (not enough comments to understand for a new user)

When I post a token at jwt.io it says everything is good to go, signature verified, but this library seems to require a key (certificate). Why is it jwt.io is ok with verifying the signature but this library is unable to without a key? Is this key just the standard onprem ca root certificate?

lknite avatar Mar 10 '24 20:03 lknite

Closed by https://github.com/golang-jwt/jwt/pull/120

oxisto avatar Mar 11 '24 14:03 oxisto

I was lucky to have found sample code for my use case, keycloak, here.

Without that, odds are I'd still be trying to get things working. (not enough comments to understand for a new user)

When I post a token at jwt.io it says everything is good to go, signature verified, but this library seems to require a key (certificate). Why is it jwt.io is ok with verifying the signature but this library is unable to without a key? Is this key just the standard onprem ca root certificate?

We are currently working on a documentation site: https://golang-jwt.github.io/jwt/usage/create/.

oxisto avatar Mar 11 '24 14:03 oxisto

I was lucky to have found sample code for my use case, keycloak, here.

Without that, odds are I'd still be trying to get things working. (not enough comments to understand for a new user)

When I post a token at jwt.io it says everything is good to go, signature verified, but this library seems to require a key (certificate). Why is it jwt.io is ok with verifying the signature but this library is unable to without a key? Is this key just the standard onprem ca root certificate? https://github.com/golang-jwt/jwt/issues/119#issuecomment-1987355945

Would be great to have a full example of best practices for that in the docs. However, JWT supports a lot of encryption algorithms, and there are also a number of potential security vulnerabilities (99% of OWASP JWT for Java is applicable to this library). In my experience, JWT is deceptively complicated for something so simple: cryptographically signed claims.

Actually jwt.io is also looking for the keys: on https://jwt.io and set the Algorithm to RS256 (matches your Keycloak config) then look at the Verify Signature box (in Decoded section). It has two fields, the PUBLIC KEY and the PRIVATE KEY. You should not use the default signature fields from https://jwt.io, you should generate your own. I have found that certificates can be tricky, because they can be encoded (e.g. as a PEM). The jwt function jwt.ParseRSAPublicKeyFromPEM can be used to convert a .pem file into the RSA Public Key that's then used for the decoding.

As for certificates, I am not sure exactly if there's a one-size-fits-all solution. Either you have a certificate already and want to use it for JWT signing (maybe your ca root certificate). Or you can generate one, e.g. openssl genrsa -out jwtRSA256-private.pem 2048 in the terminal. Or in golang, we can leverage libraries such as "crypto/rsa", "crypto/rand" and "crypto/x509"; for example, to generate a private key:

privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
publicKey = privateKey.PublicKey

twocs avatar Mar 13 '24 03:03 twocs