JWK: decode a RSA private key into JWK format. #3783

cris-he · 2021-09-13T15:26:48Z

Currently the JWT sign of the OPA it only accepts the RSA key in the JWK format, this PR adds a decoder that converts the RSA private key into JWK format.

End users could now use crypto.x509.parse_rsa_private_key to decode the RSA private key, and the builtinCryptoX509ParseRSAPrivateKey will return the JWK containing the parameters of "n, e, d, p, q, dp, dq, qi" which can be directly used in the io.jwt.encode_sign

Example usage in Rego:

    privateKey := crypto.x509.parse_rsa_private_key("$<YOUR_PRIVATE_KEY>")
    jwt := io.jwt.encode_sign({"alg":"RS256"}, {"iss":"hello"}, privateKey)

ashutosh-narkar

Thanks for the contribution @cris-he . It would be great if we can update the docs and add a test case. More info on that here.

ashutosh-narkar · 2021-09-14T00:39:42Z

topdown/crypto.go

+	if str := string(input); !strings.HasPrefix(str, "-----BEGIN RSA PRIVATE KEY-----") {
+		bytes, err = base64.StdEncoding.DecodeString(str)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// decode the pem into the Block struct
+	p, _ := pem.Decode(bytes)
+	if p != nil && p.Type != blockTypeRSAPrivateKey {
+		return nil, fmt.Errorf("invalid PEM-encoded certificate signing request")
+	}
+	if p != nil {
+		// generally you want your key can hit this following statement,
+		// because it means your key is in a good shape (parse-able).
+		bytes = p.Bytes
+	}
+


Can this block be simplified ? Something like:

block, _ := pem.Decode([]byte(input)) if block == nil { return nil, fmt.Errorf("failed to parse PEM block containing the key") } priv, err := x509.ParsePKCS1PrivateKey(block.Bytes) if err != nil { // handle error }

This part of the code are now split into 2 new functions (which is similar to the "getX509CertsFromString" and "getX509CertsFromPem" in the "builtinCryptoX509ParseAndVerifyCertificates")

getRSAPrivateKeyFromString

getRSAPrivateKeyFromPEM

topdown/crypto.go

cris-he · 2021-09-14T17:31:13Z

Thanks for the contribution @cris-he . It would be great if we can update the docs and add a test case. More info on that here.

The doc is updated and test cases were added, please let me know if they are in a good shape so I can then start squashing the commits.

srenatus

Got a few nitpicks, please bear with me 🙃

But this looks like an outstanding "first contribution" 👏 thank you

srenatus · 2021-09-15T10:22:36Z

topdown/crypto.go

+		return err
+	}
+
+	invalid := ast.BooleanTerm(false)


Hmm I think we might better return an error here? That way, the builtin call becomes undefined, and the error will be visible if running with strict-builtin-errors, or when #3742 is implemented.

do you mean instead of using "ast.BooleanTerm", to use a "ast.StringTerm" with a more cleared error message in the following err condition block?

Not exactly, I mean the built-in should fail. The common way to do that is to return an error. If it's not a halt error, evaluation will continue, the call's value is undefined.

I see, so instead of returning the ast boolean term, return the err directly as "return err" in the following blocks. Will update this shortly.

srenatus · 2021-09-15T10:25:42Z

topdown/crypto.go

+	jsonKey, err := json.Marshal(rsaPrivateKey)
+	if err != nil {
+		return iter(invalid)
+	}
+
+	var x interface{}
+	if err := util.UnmarshalJSON(jsonKey, &x); err != nil {
+		return err
+	}


Why do we need to go through JSON here? I suppose it's a shortcut, we could also declare the object as-is, couldn't we? But I guess this matches what we do when the key is expected in object form (i.e. marshal it to *PrivateKey), so this is probably OK 😄

Yes it's more like a shortcut, and the following ast.InterfaceToValue() needs it in that form and I didn't step into very much for the InterfaceToValue 😄

srenatus · 2021-09-15T10:26:23Z

topdown/crypto_test.go

+	t.Run("TestParseRSAPrivateKey", func(t *testing.T) {
+		parsed, err := getRSAPrivateKeyFromString(rsaPrivateKey)
+		if err != nil {
+			t.Fatalf("failed to parse PEM cert chain: %v", err)


💭 Is this a PEM cert chain?

My bad 🥲

srenatus · 2021-09-15T10:26:37Z

topdown/crypto_test.go

+		}
+
+		if _, err := jwk.New(parsed); err != nil {
+			t.Error("RSA private key failed when it was expected to succeed")


[nit]

Suggested change

t.Error("RSA private key failed when it was expected to succeed")

t.Errorf("RSA private key failed when it was expected to succeed, got %v", err)

anderseknert

Looks great! Left a few comments. The one thing I think we might want to consider though is why this is needed in the first place. If this is only about token signing, would it not be better if those methods accepted a PEM encoded key directly? There seems to be an asymmetry in how we currently allow that for verification but not for signing. Or do we expect this to be useful outside of that particular use case?

docs/content/policy-reference.md

anderseknert · 2021-09-15T11:29:32Z

topdown/crypto.go

+		return nil, fmt.Errorf("PEM block type is '%s', expected %s", p.Type, blockTypeRSAPrivateKey)
+	}
+
+	return x509.ParsePKCS1PrivateKey(p.Bytes)


What about PKCS8? We've had issues with that in the past.

According to the go lib func ParsePKCS8PrivateKey(der []byte) (key interface{}, err error) is more for all asymmetric keys but not RSA only, and the func ParsePKCS1PrivateKey(der []byte) (*rsa.PrivateKey, error) is designed for RSA.

Since this feature is more dedicated for the RSA keys (also function names all prefixed with "RSA"), here the PKCS1 is more suitable, and I think for the PKCS8 we could work in another branch.

While you are correct that it's not RSA exclusive, RSA is still valid - and I think a built-in named parse_rsa_private_key needs to take this into account. I think the previously linked issues show that. We already have working code to deal with this, so I'd suggest we reuse that. It should not add much in terms of effort, or what do you think?

I see what you mean, and I was confused that if we also deal with PKCS8 here that I might have to remove the "RSA" prefix in the function name.

I will add the condition block at the end here which will be similar to what in the GetSigningKey

It will be something like this:

priv, err := x509.ParsePKCS1PrivateKey(block.Bytes) if err != nil { pkcs8priv, err2 := x509.ParsePKCS8PrivateKey(block.Bytes) if err2 != nil { return nil, fmt.Errorf("error parsing private key (%v), (%v)", err, err2) } return pkcs8priv, nil } return priv, nil

srenatus · 2021-09-15T12:45:01Z

internal/jwx/jwk/interface.go

@@ -48,6 +48,7 @@ type RSAPublicKey struct {
 // RSAPrivateKey is a type of JWK generated from RSA private keys
 type RSAPrivateKey struct {
 	*StandardHeaders
+	*jwa.AlgorithmParameters
 	key *rsa.PrivateKey


I think this may have broken a test over here:

opa/internal/jwx/jwk/rsa_test.go

Lines 44 to 46 in 4747e22

if !bytes.Equal(jsonBuf1, jsonBuf2) {

t.Fatal("JSON marshal buffers do not match")

}

👉 https://github.com/open-policy-agent/opa/runs/3601600664?check_suite_focus=true#step:3:207

Thank you catching it, and I have pushed the fix.

cris-he · 2021-09-15T14:33:44Z

Looks great! Left a few comments. The one thing I think we might want to consider though is why this is needed in the first place. If this is only about token signing, would it not be better if those methods accepted a PEM encoded key directly? There seems to be an asymmetry in how we currently allow that for verification but not for signing. Or do we expect this to be useful outside of that particular use case?

Thank you all for reviewing!

I think this feature would be only used for token signing as far as I can tell, it seems no one or no any other things that requires the RSA in the JWK format with the algo params.

I guess in general a "symmetric key" is good enough for most of the cases or companies, but mine here just needs a more strict control on this so we use RSA here.

The current parser func parse(jwkSrc string) (*Set, error) in the JWK looks don't have a "custom" marshaller and unmarshaller with a bunch of condition blocks that checks the key type, I think if we do the accepting key directly could be achieved. But I am not sure what was the original concern on the OPA side when designing the JWT encode sign function.

cris-he · 2021-09-15T16:06:54Z

Could anyone help me to understand the PR Check/ Check Generated is failing on "capabilities.json"?

Is it generated by a version update script?

srenatus · 2021-09-15T19:08:22Z

Is it generated by a version update script?

Sorry for the silence here. Yeah, it's make generate, but you've figured that out already 😄

anderseknert · 2021-09-15T19:21:24Z

I guess in general a "symmetric key" is good enough for most of the cases or companies, but mine here just needs a more strict control on this so we use RSA here.

Right, I wasn't talking about asymmetric keys here but the fact that our signing functions (which do not accept PEM encoded keys) are different from our verification functions (which do accept PEM encoded keys) - they are different from each other in that regard, i.e. asymmetric :) Given how rarely OPA currently issues tokens outside of test environments, I think we can live with that until this changes.. and this additional helper function is certainly helpful.

cris-he · 2021-09-15T19:33:43Z

I guess in general a "symmetric key" is good enough for most of the cases or companies, but mine here just needs a more strict control on this so we use RSA here.

Right, I wasn't talking about asymmetric keys here but the fact that our signing functions (which do not accept PEM encoded keys) are different from our verification functions (which do accept PEM encoded keys) - they are different from each other in that regard, i.e. asymmetric :) Given how rarely OPA currently issues tokens outside of test environments, I think we can live with that until this changes.. and this additional helper function is certainly helpful.

Thanks, and I have squashed all my commits since I made all updates regarding to each comment, if no more changes please let me know if this PR can be merged.

docs/content/policy-reference.md

srenatus · 2021-09-16T07:41:50Z

ℹ️ I've changed something with the required tests, this PR will go green when it's rebased.

cris-he · 2021-09-16T14:16:22Z

ℹ️ I've changed something with the required tests, this PR will go green when it's rebased.

Hi @srenatus, @anderseknert and @ashutosh-narkar, thank you very much for reviewing!

I have finished the rebasing and please let me know when it can go green and merged, thanks again!

anderseknert

LGTM

This will help users for JWT signing using RSA key, because currently OPA only accepts RSA key in the JWK format. Fixes: open-policy-agent#3765 Signed-off-by: cris-he <cruztiempo@hotmail.com>

cris-he · 2021-09-16T22:46:22Z

I think I just lost the "approval" after another rebase to make the branch up-to-date, can anyone help approve this again 🥲

srenatus · 2021-09-17T05:03:30Z

Thanks again! 👏

anderseknert · 2021-09-17T08:25:38Z

Thanks for contributing @cris-he! I'm currently writing a blog post on using OPA to connect to GCP resources. As they require a signed JWT but deliver a PEM in their credentials file, this will be a first good real world test, as well as a spotlight on the new function :)

anderseknert · 2021-09-17T19:06:21Z

Works like a charm, and here's some POC code: https://gist.github.com/anderseknert/0789dd67df717f8d7de86786d002e5b7
Your new function on line 13 🙂

…t#3783) This will help users for JWT signing using RSA key, because currently OPA only accepts RSA key in the JWK format. Fixes: open-policy-agent#3765 Signed-off-by: cris-he <cruztiempo@hotmail.com> Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>

ashutosh-narkar reviewed Sep 14, 2021

View reviewed changes

topdown/crypto.go Outdated Show resolved Hide resolved

srenatus reviewed Sep 15, 2021

View reviewed changes

anderseknert reviewed Sep 15, 2021

View reviewed changes

srenatus reviewed Sep 15, 2021

View reviewed changes

cris-he force-pushed the rsa-decoder branch from 3acd283 to 78fbfe6 Compare September 15, 2021 15:01

cris-he force-pushed the rsa-decoder branch 2 times, most recently from 48de29e to 9eba4de Compare September 15, 2021 19:30

anderseknert reviewed Sep 15, 2021

View reviewed changes

docs/content/policy-reference.md Outdated Show resolved Hide resolved

cris-he force-pushed the rsa-decoder branch from 1c18080 to 54b7dae Compare September 15, 2021 20:36

cris-he force-pushed the rsa-decoder branch from 54b7dae to 11f9fab Compare September 16, 2021 13:59

anderseknert previously approved these changes Sep 16, 2021

View reviewed changes

JWK: decode a RSA private key into JWK format.

6d80586

This will help users for JWT signing using RSA key, because currently OPA only accepts RSA key in the JWK format. Fixes: open-policy-agent#3765 Signed-off-by: cris-he <cruztiempo@hotmail.com>

cris-he dismissed anderseknert’s stale review via 6d80586 September 16, 2021 22:42

cris-he force-pushed the rsa-decoder branch from 11f9fab to 6d80586 Compare September 16, 2021 22:42

srenatus approved these changes Sep 17, 2021

View reviewed changes

srenatus merged commit 22d505f into open-policy-agent:main Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JWK: decode a RSA private key into JWK format. #3783

JWK: decode a RSA private key into JWK format. #3783

cris-he commented Sep 13, 2021

ashutosh-narkar left a comment

ashutosh-narkar Sep 14, 2021

cris-he Sep 14, 2021

cris-he commented Sep 14, 2021

srenatus left a comment

srenatus Sep 15, 2021

cris-he Sep 15, 2021

srenatus Sep 15, 2021 •

edited

Loading

cris-he Sep 15, 2021

srenatus Sep 15, 2021

cris-he Sep 15, 2021

srenatus Sep 15, 2021

cris-he Sep 15, 2021

srenatus Sep 15, 2021

cris-he Sep 15, 2021

anderseknert left a comment

anderseknert Sep 15, 2021

cris-he Sep 15, 2021

anderseknert Sep 15, 2021

cris-he Sep 15, 2021

srenatus Sep 15, 2021

cris-he Sep 15, 2021

cris-he commented Sep 15, 2021

cris-he commented Sep 15, 2021 •

edited

Loading

srenatus commented Sep 15, 2021

anderseknert commented Sep 15, 2021

cris-he commented Sep 15, 2021

srenatus commented Sep 16, 2021

cris-he commented Sep 16, 2021

anderseknert left a comment

cris-he commented Sep 16, 2021

srenatus commented Sep 17, 2021

anderseknert commented Sep 17, 2021

anderseknert commented Sep 17, 2021

	t.Error("RSA private key failed when it was expected to succeed")
	t.Errorf("RSA private key failed when it was expected to succeed, got %v", err)

	if !bytes.Equal(jsonBuf1, jsonBuf2) {
	t.Fatal("JSON marshal buffers do not match")
	}

JWK: decode a RSA private key into JWK format. #3783

JWK: decode a RSA private key into JWK format. #3783

Conversation

cris-he commented Sep 13, 2021

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cris-he commented Sep 14, 2021

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anderseknert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cris-he commented Sep 15, 2021

cris-he commented Sep 15, 2021 • edited Loading

srenatus commented Sep 15, 2021

anderseknert commented Sep 15, 2021

cris-he commented Sep 15, 2021

srenatus commented Sep 16, 2021

cris-he commented Sep 16, 2021

anderseknert left a comment

Choose a reason for hiding this comment

cris-he commented Sep 16, 2021

srenatus commented Sep 17, 2021

anderseknert commented Sep 17, 2021

anderseknert commented Sep 17, 2021

srenatus Sep 15, 2021 •

edited

Loading

cris-he commented Sep 15, 2021 •

edited

Loading