Extend JWT Signing Algorithms to Support RSA and EC #2805

MaciejMierzwa · 2023-05-30T14:02:23Z

Description

Previously, the application was limited to HMAC-based algorithms (HS256, HS384, HS512). With this update, RSA and Elliptic Curve algorithms are now supported as well.

Supported Algorithms:

HMAC: HS256, HS384, HS512
RSA: RS256, RS384, RS512
Elliptic Curve: ES256, ES384, ES512

The new feature enables users to configure the signing method by specifying JSON Web Keys (JWK) in the configuration, modeled after their JSON structure, or by directly using private RSA/EC keys. This aligns with the algorithms specified in RFC 7518 Section 3.1.

Issues Resolved

#2680

Testing

Unit tests prepared with focus on EC and SHA JWT creation

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2023-05-30T14:35:16Z

Codecov Report

Merging #2805 (98e2ecc) into main (1034cef) will increase coverage by 0.12%.
The diff coverage is 93.68%.

@@             Coverage Diff              @@
##               main    #2805      +/-   ##
============================================
+ Coverage     63.24%   63.36%   +0.12%     
- Complexity     3452     3474      +22     
============================================
  Files           263      264       +1     
  Lines         20053    20130      +77     
  Branches       3348     3352       +4     
============================================
+ Hits          12683    12756      +73     
- Misses         5741     5746       +5     
+ Partials       1629     1628       -1

Files Changed	Coverage Δ
...org/opensearch/security/authtoken/jwt/JwkUtil.java	`93.61% <93.61%> (ø)`
...g/opensearch/security/authtoken/jwt/JwtVendor.java	`83.67% <100.00%> (+7.91%)`	⬆️

... and 5 files with indirect coverage changes

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

cwperks · 2023-05-30T15:11:54Z

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

@@ -123,6 +127,38 @@ static JsonWebKey createJwkFromSettings(Settings settings) throws Exception {
        }
    }

+    private static JsonWebKey getSigningJwk(String algorithm, Settings settings) throws Exception {


Hey @MaciejMierzwa, I also wanted to add a link to how this is configured for the SAML backend. The SAML backend is very generic and let's any configured properties be added to the JWK. Idk if that area should be updated to match the logic here to have a pre-defined list of properties or if it makes sense to match the logic there to read in any configured settings: https://github.com/opensearch-project/security/blob/main/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java#L265-L297

I think it's already added by Ryan in his original implementation. It should allow to load non-pre-defined config. It would just adhere to library standards, which just implement naming from the documentation. If no config with our keys is present, then we could load config looking like this:

{ "kty":"RSA", "n":"ofgWCuLjybRlzo0tZWJjNiuSfb4p4fAkd_wWJcyQoTbji9k0l8W26mPddx HmfHQp-Vaw-4qPCJrcS2mJPMEzP1Pt0Bm4d4QlL-yRT-SFd2lZS-pCgNMs D1W_YpRPEwOWvG6b32690r2jZ47soMZo9wGzjb_7OMg0LOL-bSf63kpaSH SXndS5z5rexMdbBYUsLA9e-KXBdQOS-UTo7WTBEMa2R2CapHg665xsmtdV MTBQY4uDZlxvb3qCo5ZwKh9kG4LT6_I5IhlJH7aGhyxXFvUK-DWNmoudF8 NAco9_h9iaGNj8q2ethFkMLs91kzk2PAcDTW9gb54h4FRWyuXpoQ", "e":"AQAB", "d":"Eq5xpGnNCivDflJsRQBXHx1hdR1k6Ulwe2JZD50LpXyWPEAeP88vLNO97I jlA7_GQ5sLKMgvfTeXZx9SE-7YwVol2NXOoAJe46sui395IW_GO-pWJ1O0 BkTGoVEn2bKVRUCgu-GjBVaYLU6f3l9kJfFNS3E0QbVdxzubSu3Mkqzjkn 439X0M_V51gfpRLI9JYanrC4D4qAdGcopV_0ZHHzQlBjudU2QvXt4ehNYT CBr6XCLQUShb1juUO1ZdiYoFaFQT5Tw8bGUl_x_jTj3ccPDVZFD9pIuhLh BOneufuBiB4cS98l2SR_RQyGWSeWjnczT0QU91p1DhOVRuOopznQ", "p":"4BzEEOtIpmVdVEZNCqS7baC4crd0pqnRH_5IB3jw3bcxGn6QLvnEtfdUdi YrqBdss1l58BQ3KhooKeQTa9AB0Hw_Py5PJdTJNPY8cQn7ouZ2KKDcmnPG BY5t7yLc1QlQ5xHdwW1VhvKn-nXqhJTBgIPgtldC-KDV5z-y2XDwGUc", "q":"uQPEfgmVtjL0Uyyx88GZFF1fOunH3-7cepKmtH4pxhtCoHqpWmT8YAmZxa ewHgHAjLYsp1ZSe7zFYHj7C6ul7TjeLQeZD_YwD66t62wDmpe_HlB-TnBA -njbglfIsRLtXlnDzQkv5dTltRJ11BKBBypeeF6689rjcJIDEz9RWdc", "dp":"BwKfV3Akq5_MFZDFZCnW-wzl-CCo83WoZvnLQwCTeDv8uzluRSnm71I3Q CLdhrqE2e9YkxvuxdBfpT_PI7Yz-FOKnu1R6HsJeDCjn12Sk3vmAktV2zb 34MCdy7cpdTh_YVr7tss2u6vneTwrA86rZtu5Mbr1C1XsmvkxHQAdYo0", "dq":"h_96-mK1R_7glhsum81dZxjTnYynPbZpHziZjeeHcXYsXaaMwkOlODsWa 7I9xXDoRwbKgB719rrmI2oKr6N3Do9U0ajaHF-NKJnwgjMd2w9cjz3_-ky NlxAr2v4IKhGNpmM5iIgOS1VZnOZ68m6_pbLBSp3nssTdlqvd0tIiTHU", "qi":"IYd7DHOhrWvxkwPQsRM2tOgrjbcrfvtQJipd-DlcxyVuuM9sQLdgjVk2o y26F0EmpScGLq2MowX7fhd_QJQ3ydy5cY7YIBi87w93IKLEdfnbJtoOPLU W0ITrJReOgo1cq9SbsxYawBgfp_gh6A5603k2-ZQwVK0JKSHuLFkuQ3U" }

for RSA which is also ok since it's well documented

Hey @MaciejMierzwa , I'd like to revisit this thread. In the SAML Authentication Response handler it creates a JWT and will use the exchange_key from the saml authenticator settings in the authc: section of the security config. Example:

authc: saml_auth_domain: http_enabled: true transport_enabled: false order: 1 http_authenticator: type: saml challenge: true config: idp: metadata_file: metadata.xml entity_id: http://idp.example.com/ sp: entity_id: https://opensearch-dashboards.example.com kibana_url: https://opensearch-dashboards.example.com:5601/ roles_key: Role exchange_key: 'peuvgOLrjzuhXf ...' authentication_backend: type: noop

Inside of the logic where it generates the token there is an else block that takes any settings under key and sets them as properties for the JWK. It looks like the logic is kept very generic to provide as much flexibility as possible to the cluster admin configuring the SAML authenticator. Do you think that logic is too generic and should be better validated like you are doing in this PR or should this On Behalf Of authenticator have the same flexibility as its done in the SAML authenticator?

Hi @MaciejMierzwa, did you get a chance to see this comment above? I know you shared some code earlier that looked like it had some updates. Did you get a chance to push those changes?

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java

stephen-crawford

Overall looks really close. I just wanted to check with you about what you thought for documentation and also see whether you can add checks for all the algorithms. I know it may seem like overkill, but in theory we should test all the algorithms we offer.

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

cwperks

Thank you @MaciejMierzwa ! This is quite thorough and the tests cover the different configuration options. I left a few comments around the ease of configuration and let me know what you think.

build.gradle

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

stephen-crawford · 2023-06-22T20:01:42Z

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

+        Settings jwkSettings = settings.getAsSettings("jwt").getAsSettings("key");
+
+        if (algorithm != null) {
+            return getSigningJwk(algorithm.toUpperCase(), settings);


Suggested change

return getSigningJwk(algorithm.toUpperCase(), settings);

return getSigningJwk(algorithm.toUpperCase(Locale.ROOT), settings);

stephen-crawford

HI @MaciejMierzwa, overall looks very good. Just a few minor comments. Let's try to work together to drive this to close.

stephen-crawford · 2023-06-22T20:04:57Z

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

            jwk.setPublicKeyUse(PublicKeyUse.SIGN);
+            jwk.setKeyType(KeyType.OCTET);
+            String signingKey = Optional.ofNullable(settings.get("signing_key")).orElseThrow(() -> new IllegalArgumentException("Settings for signing key is missing. Please specify at least the option signing_key with a shared secret."));


We will want to document this; we can add a table or something with the supported keys and explain secret creation.

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java

src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java

stephen-crawford

This looks good to me after the CI issues are resolved. Excellent job with the tests!

DarshitChanpura · 2023-08-17T20:36:36Z

@MaciejMierzwa LGTM. Once you resolve merge conflicts, this should be good to go.

peternied · 2023-08-24T13:26:09Z

I've re-written the description of this change and the title of the pull request to be more accessible to the community

peternied

There is another pull request that is merging feature/extension to main [1], that leaves this change in a strange place.

@RyanL1997 since you are handling this branch what next steps do you recommend?

[1] OnBehalfOf authenticator and token generator #3179

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

RyanL1997 · 2023-08-24T16:39:09Z

There is another pull request that is merging feature/extension to main [1], that leaves this change in a strange place.
@RyanL1997 since you are handling this branch what next steps do you recommend?

Hi @MaciejMierzwa and @peternied, I'm pushing the merge into main for now. And that PR (#3179 ) is very close for merging, so should we await for the merging of that PR and change point this PR to main?

RyanL1997 · 2023-08-31T00:20:09Z

Hi @MaciejMierzwa, just a friendly reminder that we can reference this PR directly to main branch according to the merge of #3179. The feature branch will be cleaned up and deleted soon.

The base branch was changed.

MaciejMierzwa · 2023-08-31T13:52:39Z

Hi @RyanL1997 since this branch was based on branch feature/extensions there are a lot of commit's that didn't made yet to main. What do you think about dropping most of the commits beside mine and letting commits from feature/extensions be pulled in later?
I'm not sure how much of my changes were based on commits existing exclusively existing in feature/extensions. It might be impossible to get rid of all of them.

Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

# Conflicts: # build.gradle

Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

peternied · 2023-09-07T14:48:00Z

@MaciejMierzwa I think we should put this PR on hold until after the JWT library swap [1] has been addressed. Maybe this would be a good one to sync up offline

[1] Switch JWT library implementations #3267

davidlago · 2023-09-07T15:22:58Z

Closing PR for now as we are leaving it in the back burner for now.

MaciejMierzwa requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford and reta as code owners May 30, 2023 14:02

willyborankin requested changes May 30, 2023

View reviewed changes

cwperks reviewed May 30, 2023

View reviewed changes

DarshitChanpura reviewed May 30, 2023

View reviewed changes

DarshitChanpura reviewed May 31, 2023

View reviewed changes

src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java Outdated Show resolved Hide resolved

stephen-crawford reviewed Jun 1, 2023

View reviewed changes

peternied reviewed Jun 6, 2023

View reviewed changes

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java Outdated Show resolved Hide resolved

cwperks reviewed Jun 7, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java Outdated Show resolved Hide resolved

peternied reviewed Jun 7, 2023

View reviewed changes

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java Outdated Show resolved Hide resolved

MaciejMierzwa force-pushed the JWK_support_for_RSA_EC branch 3 times, most recently from a520210 to 1a8de74 Compare June 13, 2023 14:13

stephen-crawford reviewed Jun 22, 2023

View reviewed changes

MaciejMierzwa force-pushed the JWK_support_for_RSA_EC branch 2 times, most recently from e7a801a to cf8d2c9 Compare June 23, 2023 14:39

MaciejMierzwa force-pushed the JWK_support_for_RSA_EC branch from f7cf41a to d6685df Compare July 4, 2023 07:38

MaciejMierzwa requested a review from willyborankin July 4, 2023 08:50

cwperks mentioned this pull request Jul 5, 2023

[Feature/Extension] Add oboauthcbackend registry and set up e2e endpoint testing flow #2857

Merged

3 tasks

stephen-crawford previously approved these changes Jul 27, 2023

View reviewed changes

peternied changed the title ~~Jwk support for rsa ec~~ Extend JWT Signing Algorithms to Support RSA and EC Aug 24, 2023

peternied requested changes Aug 24, 2023

View reviewed changes

peternied reviewed Aug 24, 2023

View reviewed changes

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java Outdated Show resolved Hide resolved

cwperks mentioned this pull request Aug 24, 2023

Ensure that PRs that were merged into feature/extension after the merge to main was opened get added to main after the merge is completed #3235

Closed

6 tasks

MaciejMierzwa changed the base branch from feature/extensions to main August 31, 2023 08:15

Merge branch 'main' into JWK_support_for_RSA_EC

9779a87

Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

MaciejMierzwa force-pushed the JWK_support_for_RSA_EC branch from 6c07d83 to d2f3003 Compare September 5, 2023 13:04

MaciejMierzwa added 2 commits September 6, 2023 16:38

tests

fdbb53d

Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

SpotlessApply

f5c92b1

Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

MaciejMierzwa force-pushed the JWK_support_for_RSA_EC branch from d2f3003 to f5c92b1 Compare September 6, 2023 14:46

MaciejMierzwa added 2 commits September 7, 2023 11:31

Merge branch 'main' into JWK_support_for_RSA_EC

47e4d1d

# Conflicts: # build.gradle

Extract JwkUtil

98e2ecc

Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

davidlago closed this Sep 7, 2023

davidlago mentioned this pull request Sep 7, 2023

Configuration of JWK for JWT Creation for extensions requests #2680

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend JWT Signing Algorithms to Support RSA and EC #2805

Extend JWT Signing Algorithms to Support RSA and EC #2805

MaciejMierzwa commented May 30, 2023 •

edited by peternied

Loading

codecov bot commented May 30, 2023 •

edited

Loading

cwperks May 30, 2023 •

edited

Loading

MaciejMierzwa May 30, 2023 •

edited by DarshitChanpura

Loading

cwperks Jul 7, 2023

cwperks Jul 27, 2023

stephen-crawford left a comment

cwperks left a comment

stephen-crawford Jun 22, 2023

stephen-crawford left a comment

stephen-crawford Jun 22, 2023

stephen-crawford left a comment

DarshitChanpura commented Aug 17, 2023

peternied commented Aug 24, 2023

peternied left a comment

RyanL1997 commented Aug 24, 2023 •

edited

Loading

RyanL1997 commented Aug 31, 2023

MaciejMierzwa commented Aug 31, 2023

peternied commented Sep 7, 2023

davidlago commented Sep 7, 2023

	return getSigningJwk(algorithm.toUpperCase(), settings);
	return getSigningJwk(algorithm.toUpperCase(Locale.ROOT), settings);

Extend JWT Signing Algorithms to Support RSA and EC #2805

Extend JWT Signing Algorithms to Support RSA and EC #2805

Conversation

MaciejMierzwa commented May 30, 2023 • edited by peternied Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented May 30, 2023 • edited Loading

Codecov Report

cwperks May 30, 2023 • edited Loading

Choose a reason for hiding this comment

MaciejMierzwa May 30, 2023 • edited by DarshitChanpura Loading

Choose a reason for hiding this comment

cwperks Jul 7, 2023

Choose a reason for hiding this comment

cwperks Jul 27, 2023

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

cwperks left a comment

Choose a reason for hiding this comment

stephen-crawford Jun 22, 2023

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

stephen-crawford Jun 22, 2023

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Aug 17, 2023

peternied commented Aug 24, 2023

peternied left a comment

Choose a reason for hiding this comment

RyanL1997 commented Aug 24, 2023 • edited Loading

RyanL1997 commented Aug 31, 2023

MaciejMierzwa commented Aug 31, 2023

peternied commented Sep 7, 2023

davidlago commented Sep 7, 2023

MaciejMierzwa commented May 30, 2023 •

edited by peternied

Loading

codecov bot commented May 30, 2023 •

edited

Loading

cwperks May 30, 2023 •

edited

Loading

MaciejMierzwa May 30, 2023 •

edited by DarshitChanpura

Loading

RyanL1997 commented Aug 24, 2023 •

edited

Loading