Add AWS KMS support for OAuth2 Client Credentials JWT authentication #5942

prasanthu · 2023-05-23T10:21:05Z

This implementaion adds new configuration properties to "oauth2" aws_kms: AWS KMS key details
aws_signing: Infomation for signing AWS requestion, similar to s3_signing

References:

Sample config file: https://gist.github.com/prasanthu/16e5e668ea864487d44dd7e95ff0d34b

Please refer to this Github issue for more details: #5892

Why the changes in this PR are needed?

What are the changes in this PR?

Notes to assist PR review:

Further comments:

ashutosh-narkar · 2023-05-23T20:51:57Z

@prasanthu thanks for working on this. Can you please add some tests and also instructions on how users can configure this. The later would need changes in this file.

netlify · 2023-05-24T14:26:31Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`6dc46be`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/64a30a6346d80600084d7e13
😎 Deploy Preview	https://deploy-preview-5942--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ashutosh-narkar

@prasanthu thanks for the contribution. The changes look ok to me. Few comments in-line. Have you done some end-to-end tests with these changes? It would be interesting to know the results of some manual testing.

ashutosh-narkar · 2023-05-30T22:53:36Z

docs/content/configuration.md

+| `services[_].credentials.oauth2.aws_kms_key.algorithm`               | `string`   | Yes      | Specifies the signing algorithm `(ECDSA_SHA_256, ECDSA_SHA_384 or ECDSA_SHA_512)`.           |
+| `services[_].credentials.oauth2.aws_signing.`                        | `{}` | No       | AWS credentials for signing requests. Required if `aws_kms_key` is provided.                 |
+| `services[_].credentials.oauth2.aws_signing.service`                 | `string`   | Yes      | The AWS service to sign requests with. Only `kms` is currently supported.                    |
+| `services[_].credentials.oauth2.aws_signing.environment_credentials` | `{}`       | Yes      | See description for `services[_].credentials.s3_signing.environment_credential`.             |


Only the Static Environment Credentials method is supported here? What about other methods like Named Profile Credentials, EC2 Metadata Credentials etc?

It supports all off them, I have updated the docs

ashutosh-narkar · 2023-05-30T23:11:19Z

plugins/rest/auth.go

+	Algorithm string `json:"algorithm"`
+}
+
+func converSignatureToBase64(alg string, der []byte) (string, error) {


Nit: converSignatureToBase64 -> convertSignatureToBase64

ashutosh-narkar · 2023-05-30T23:24:10Z

plugins/rest/auth.go

+		return nil, nil, fmt.Errorf("failed to unmarshall the signature from DER format %v", err)
+
+	}
+	// The format of our DER string is 0x02 + rlen + r + 0x02 + slen + s


Is this based on some standard? If yes, please include the references.

Some of the references are mentioned in the commit
References:

https://github.com/go-jose/go-jose/blob/v3/asymmetric.go#L501

https://github.com/codelittinc/gobitauth/blob/master/sign.go#L101

Added some references

ashutosh-narkar · 2023-05-30T23:25:23Z

plugins/rest/auth.go

+	}
+	// We serialize the outputs (r and s) into big-endian byte arrays and pad
+	// them with zeros on the left to make sure the sizes work out. Both arrays
+	// must be keyBytes long, and the output must be 2*keyBytes long.


Same as above. If this is based on some standard please, include the references.

Should I add this in the code
https://github.com/go-jose/go-jose/blob/v3/asymmetric.go#L501

prasanthu · 2023-06-02T11:50:03Z

@prasanthu thanks for the contribution. The changes look ok to me. Few comments in-line. Have you done some end-to-end tests with these changes? It would be interesting to know the results of some manual testing.

I have tested this in our AWS environment (using environment_credential). I am not sure how I can share the logs.

ashutosh-narkar · 2023-06-07T17:22:10Z

docs/content/configuration.md

+| `services[_].credentials.oauth2.aws_kms_key`                         | `{}`       | No       | AWS KMS details for signing client assertions. Required only for signing with AWS KMS.                                                                                                                                           |
+| `services[_].credentials.oauth2.aws_kms_key.name`                    | `string`   | Yes      | To specify a KMS key, use its key ID, key ARN, alias name, or alias ARN.                                                                                                                                                         |
+| `services[_].credentials.oauth2.aws_kms_key.algorithm`               | `string`   | Yes      | Specifies the signing algorithm `(ECDSA_SHA_256, ECDSA_SHA_384 or ECDSA_SHA_512)`.                                                                                                                                               |


We can remove the aws_kms_key row. In the description for aws_kms_key.name and aws_kms_key.algorithm change the Required field to No. Also can you update the descriptions to explicitly mention that these fields are required only when using AWS KMS.

Please check the changes.

ashutosh-narkar · 2023-06-07T17:25:27Z

docs/content/configuration.md

+| `services[_].credentials.oauth2.aws_signing.`                        | `{}` | No       | AWS credentials for signing requests. Required if `aws_kms_key` is provided. Several methods of obtaining the necessary credentials are available; exactly one must be specified to use the AWS KMS signing. |
+| `services[_].credentials.oauth2.aws_signing.service`                 | `string`   | Yes      | The AWS service to sign requests with. Only `kms` is currently supported.                                                                                                                                                        |
+| `services[_].credentials.oauth2.aws_signing.environment_credentials` | `{}`       | Yes      | See description for `services[_].credentials.s3_signing.environment_credential`.                                                                                                                                                 |
+| `services[_].credentials.oauth2.aws_signing.profile_credentials` | `{}`       | No       | See description for `services[_].credentials.s3_signing.profile_credentials`.                                                                                                                                                    |
+| `services[_].credentials.oauth2.aws_signing.metadata_credentials` | `{}`       | No       | See description for `services[_].credentials.s3_signing.metadata_credentials`.                                                                                                                                                    |
+| `services[_].credentials.oauth2.aws_signing.web_identity_credentials` | `{}`       | No       | See description for `services[_].credentials.s3_signing.web_identity_credentials`.                                                                                                                                                    |


You can probably replace this by adding a note in that section about how the credentials should be provided if AWS KMS signing is used. For ex,

{{< info >}}
ADD RELEVANT INFO
{{< /info >}}

Please check the changes.

ashutosh-narkar · 2023-06-07T18:02:03Z

@prasanthu thanks for the contribution. The changes look ok to me. Few comments in-line. Have you done some end-to-end tests with these changes? It would be interesting to know the results of some manual testing.

I have tested this in our AWS environment (using environment_credential). I am not sure how I can share the logs.

You can share the OPA logs if any that exercise the workflow.

prasanthu · 2023-06-15T14:33:14Z

Not sure why "PR Check / Go compat build/test (ubuntu-22.04, 1.19) (pull_request)" is failing

ashutosh-narkar · 2023-06-15T17:29:14Z

Not sure why "PR Check / Go compat build/test (ubuntu-22.04, 1.19) (pull_request)" is failing

We're seeing some test flakiness lately which we're looking into. Not related to the changes in this PR.

prasanthu · 2023-06-15T19:45:00Z

@prasanthu thanks for the contribution. The changes look ok to me. Few comments in-line. Have you done some end-to-end tests with these changes? It would be interesting to know the results of some manual testing.

I have tested this in our AWS environment (using environment_credential). I am not sure how I can share the logs.

You can share the OPA logs if any that exercise the workflow.

Attaching logs showing successful discovery, status & bundles calls using this mechanism
sit.log

prasanthu · 2023-06-15T20:08:33Z

Not sure why "PR Check / Go compat build/test (ubuntu-22.04, 1.19) (pull_request)" is failing

We're seeing some test flakiness lately which we're looking into. Not related to the changes in this PR.

Now its "PR Check / Go Race Detector (pull_request)". Both failures look similar

ashutosh-narkar

@prasanthu this is getting close. Thanks for addressing the comments. I had one more inline.

ashutosh-narkar · 2023-06-20T17:49:49Z

docs/content/configuration.md

+| `services[_].credentials.oauth2.aws_kms_key.name`                   | `string`   | No       | To specify a KMS key, use its key ID, key ARN, alias name, or alias ARN. Required only for signing with AWS KMS.                                                                                            |
+| `services[_].credentials.oauth2.aws_kms_key.algorithm`              | `string`   | No       | Specifies the signing algorithm used by the key `aws_kms_key.name` `(ECDSA_SHA_256, ECDSA_SHA_384 or ECDSA_SHA_512)`. Required only for signing with AWS KMS.                                               |


What do you think about replacing aws_kms_key.name to aws_kms.key and aws_kms_key.algorithm to aws_kms.algorithm? Seems more future-proof if we need to add more fields for aws_kms.

ashutosh-narkar · 2023-06-26T23:22:07Z

docs/content/configuration.md

+| `services[_].credentials.oauth2.token_url`         | `string`   | Yes      | URL pointing to the token endpoint at the OAuth2 authorization server.                                                                                                                          |
+| `services[_].credentials.oauth2.grant_type`        | `string`   | No       | Defaults to `client_credentials`.                                                                                                                                                               |
+| `services[_].credentials.oauth2.client_id`         | `string`   | No       | The client ID to use for authentication.                                                                                                                                                        |
+| `services[_].credentials.oauth2.signing_key`       | `string`   | No       | Reference to private key used for signing the JWT. Required if `aws_kms` is not provided                                                                                                        |


Required if aws_kms is not provided this means if someone does not specify aws_kms, then this field has to be set? If that's the case then this would be backward incompatible. Or is this the current behavior already?

It was a "Required" field before, with this feature you can either pass a private key or a KMS key.

Ok. Thanks for the clarification.

ashutosh-narkar · 2023-06-26T23:36:46Z

plugins/rest/rest_test.go

@@ -543,6 +543,55 @@ func TestNew(t *testing.T) {
 				}
 			}`, grantTypeClientCredentials),
 		},
+		{


@prasanthu we should have a test that exercises the workflow. There are examples of this in plugins/rest/rest_test.go. WDYT?

I had a look at the TestOauth2ClientCredentialsJwtAuthentication. The current setup won't allow to mock the client_assertion creation.

Refer these methods:
https://github.com/open-policy-agent/opa/blob/main/plugins/rest/rest_test.go#L1632
https://github.com/open-policy-agent/opa/blob/main/plugins/rest/rest_test.go#L2002

Any pointers on how to mock the generation of client_assertion

Can you help me understand where you would like to have the ability to mock the generation of client_assertion? We should be able to customize the test to accommodate this I would imagine.

When we use KMS to sign the assertion, the SignDigest() gets invoked. It will send a signed AWS request to the KMS endpoint (based on the environment) for signing.
https://github.com/prasanthu/opa/blob/feature/kms-using-v4signed-request/internal/providers/aws/kms.go#L67
For mocking this part, we have to do something similar to the tests here: https://github.com/prasanthu/opa/blob/feature/kms-using-v4signed-request/internal/providers/aws/kms_test.go#L44

I am not sure how this can be done from a test like [TestOauth2ClientCredentialsJwtAuthentication](https://github.com/open-policy-agent/opa/blob/main/plugins/rest/rest_test.go#L1602)

Ok. Unless I'm missing something the test you mentioned creates a test server which SignDigest calls and then the test verifies the signature. We should be able to add a new similar test, correct?

I have added a test which implements your suggestion. Can you check TestOauth2ClientCredentialsGrantTypeWithKms

prasanthu · 2023-07-01T07:29:50Z

plugins/rest/rest_test.go

@@ -2344,3 +2401,117 @@ func (m *myPluginMock) NewClient(c Config) (*http.Client, error) {
 func (*myPluginMock) Prepare(*http.Request) error {
 	return nil
 }
+
+func TestOauth2ClientCredentialsGrantTypeWithKms(t *testing.T) {


New test involving AWS KMS additions

ashutosh-narkar

Thanks for the contribution @prasanthu!

This implementaion adds new configuration properties to "oauth2" aws_kms: AWS KMS key details aws_signing: Infomation for signing AWS requestion, similar to s3_signing References: 1) https://github.com/go-jose/go-jose/blob/v3/asymmetric.go#L501 2) https://github.com/codelittinc/gobitauth/blob/master/sign.go#L101 Signed-off-by: Prasanth Ullattil <prasanth.ullattil@dnb.no>

prasanthu force-pushed the feature/kms-using-v4signed-request branch 4 times, most recently from 5c819ab to ab51d82 Compare May 23, 2023 10:49

prasanthu force-pushed the feature/kms-using-v4signed-request branch from ab51d82 to 8379036 Compare May 24, 2023 14:25

ashutosh-narkar requested a review from anderseknert May 24, 2023 16:28

ashutosh-narkar reviewed May 30, 2023

View reviewed changes

ashutosh-narkar added the waiting-for-response label Jun 1, 2023

prasanthu force-pushed the feature/kms-using-v4signed-request branch 3 times, most recently from 92fba57 to 66ba0cb Compare June 2, 2023 11:46

ashutosh-narkar marked this pull request as ready for review June 7, 2023 16:38

ashutosh-narkar reviewed Jun 7, 2023

View reviewed changes

prasanthu force-pushed the feature/kms-using-v4signed-request branch 4 times, most recently from 1e8cf68 to 40ded4a Compare June 15, 2023 14:02

ashutosh-narkar removed the waiting-for-response label Jun 15, 2023

prasanthu force-pushed the feature/kms-using-v4signed-request branch from 40ded4a to 6b24a2c Compare June 15, 2023 19:52

ashutosh-narkar reviewed Jun 20, 2023

View reviewed changes

ashutosh-narkar added the waiting-for-response label Jun 20, 2023

prasanthu force-pushed the feature/kms-using-v4signed-request branch from 6b24a2c to 89c6ba9 Compare June 23, 2023 11:44

prasanthu force-pushed the feature/kms-using-v4signed-request branch from 89c6ba9 to 589dd6a Compare June 23, 2023 19:34

ashutosh-narkar reviewed Jun 26, 2023

View reviewed changes

prasanthu force-pushed the feature/kms-using-v4signed-request branch 6 times, most recently from 3bcbd1f to f0d54a8 Compare July 1, 2023 07:26

prasanthu commented Jul 1, 2023

View reviewed changes

ashutosh-narkar approved these changes Jul 3, 2023

View reviewed changes

ashutosh-narkar force-pushed the feature/kms-using-v4signed-request branch from f0d54a8 to 6dc46be Compare July 3, 2023 17:50

ashutosh-narkar merged commit db2f8ae into open-policy-agent:main Jul 3, 2023
25 checks passed

ashutosh-narkar mentioned this pull request Jul 3, 2023

AWS KMS support for OAuth2 Client Credentials JWT authentication #5892

Closed

		\| `services[_].credentials.oauth2.aws_kms_key.name` \| `string` \| No \| To specify a KMS key, use its key ID, key ARN, alias name, or alias ARN. Required only for signing with AWS KMS. \|
		\| `services[_].credentials.oauth2.aws_kms_key.algorithm` \| `string` \| No \| Specifies the signing algorithm used by the key `aws_kms_key.name` `(ECDSA_SHA_256, ECDSA_SHA_384 or ECDSA_SHA_512)`. Required only for signing with AWS KMS. \|

Add AWS KMS support for OAuth2 Client Credentials JWT authentication #5942

Add AWS KMS support for OAuth2 Client Credentials JWT authentication #5942

Conversation

prasanthu commented May 23, 2023 • edited

Why the changes in this PR are needed?

What are the changes in this PR?

Notes to assist PR review:

Further comments:

ashutosh-narkar commented May 23, 2023

netlify bot commented May 24, 2023 • edited

✅ Deploy Preview for openpolicyagent ready!

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

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

prasanthu commented Jun 2, 2023 • edited

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

ashutosh-narkar commented Jun 7, 2023

prasanthu commented Jun 15, 2023

ashutosh-narkar commented Jun 15, 2023

prasanthu commented Jun 15, 2023

prasanthu commented Jun 15, 2023

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

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

prasanthu Jun 29, 2023 • edited

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

ashutosh-narkar left a comment

Choose a reason for hiding this comment

prasanthu commented May 23, 2023 •

edited

netlify bot commented May 24, 2023 •

edited

prasanthu commented Jun 2, 2023 •

edited

prasanthu Jun 29, 2023 •

edited