[feat][authentication] Add JWKS support for AuthenticationProviderToken by nodece · Pull Request #18336 · apache/pulsar

nodece · 2022-11-04T08:19:39Z

Motivation

Add JWKS support for AuthenticationProviderToken.

Modifications

Add tokenKeySet config to provide the JWKS support

Verifying this change

Added RSA test
Added EC test

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: nodece#11

nodece · 2022-11-04T09:17:54Z

/pulsarbot rerun-failure-checks

codecov-commenter · 2022-11-22T05:22:56Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.48%. Comparing base (68ca60c) to head (7ed2770).
⚠️ Report is 3187 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18336      +/-   ##
============================================
- Coverage     50.05%   46.48%   -3.58%     
+ Complexity    11024    10441     -583     
============================================
  Files           703      703              
  Lines         68814    68816       +2     
  Branches       7378     7377       -1     
============================================
- Hits          34446    31986    -2460     
- Misses        30621    33219    +2598     
+ Partials       3747     3611     -136

Flag	Coverage Δ
unittests	`46.48% <ø> (-3.58%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 117 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

rdhabalia

this PR is adding new feature and it should go through PIP.

codelipenghui · 2022-12-07T07:51:58Z

this PR is adding new feature and it should go through PIP.

+1

@nodece Could you please start a proposal for this change?

nodece · 2022-12-07T08:11:10Z

this PR is adding new feature and it should go through PIP.

Thanks.

Let me make a PIP.

tuteng · 2022-12-07T14:56:57Z

+                if (bytes == null || bytes.length == 0) {
+                    throw new IOException("invalid JWKs data");
+                }
+                json = new String(AuthTokenUtils.readKeyFromUrl(data), StandardCharsets.UTF_8);


Why call this function AuthTokenUtils.readKeyFromUrl(data) again?

Good catch. Here should be bytes.

tuteng · 2022-12-07T15:01:29Z

            return AuthTokenUtils.decodePublicKey(validationKey, publicKeyAlg);
        } else {
-            throw new IOException("No secret key was provided for token authentication");
+            return null;


I don't think it's the third case, it seems like it should be considered adding it to the if branch of the readKeyFromUrl, which is essentially where the public key is read (file, string or an http host)

See lines 158-162.

This method just gets the public or secret key. When returns null, we read the JWKS data.

tuteng · 2022-12-07T15:03:39Z

        }
    }
+
+    private static final class TokenSigningKeyResolver implements SigningKeyResolver {


I think the TokenSigningKeyResolver should be called in the readKeyFromUrl function

See line 491.

data is a file path or data-url.

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

github-actions · 2023-01-23T02:00:49Z

The pr had no activity for 30 days, mark with Stale label.

nodece · 2023-02-14T03:04:47Z

Ping @michaeljmarshall, could you track this PR?

michaeljmarshall · 2022-12-23T20:28:07Z

+                String kid = node.get("kid").textValue();
+                KeyFactory kf;
+                // Reference from:
+                // https://github.com/auth0/jwks-rsa-java/blob/0.21.2/src/main/java/com/auth0/jwk/Jwk.java#L176


Is there a reason we cannot use this library directly instead of copying the code here?

We just use the Jwk class, and don't want to introduce the other dependency.

The license might require that we change this PR: https://github.com/auth0/jwks-rsa-java/blob/0.21.2/LICENSE.

@dave2wave - are you able to provide any guidance on the licensing aspect here? Thanks!

michaeljmarshall

#8152 asks for support to rotate public keys and to load new public keys "on the fly". This PR appears to add support for loading multiple public keys on start up, but it does not provide support for rotation or dynamic loading. Is that correct? If so, I don't think we should say this PR fixes #8152.

nodece · 2023-02-15T05:25:21Z

#8152 asks for support to rotate public keys and to load new public keys "on the fly". This PR appears to add support for loading multiple public keys on start up, but it does not provide support for rotation or dynamic loading. Is that correct? If so, I don't think we should say this PR fixes #8152.

You are right, I think we can listener to the file changes.

michaeljmarshall

Requesting changes while we determine the licensing portion of this PR.

michaeljmarshall · 2023-02-15T19:41:20Z

+                String kid = node.get("kid").textValue();
+                KeyFactory kf;
+                // Reference from:
+                // https://github.com/auth0/jwks-rsa-java/blob/0.21.2/src/main/java/com/auth0/jwk/Jwk.java#L176


The license might require that we change this PR: https://github.com/auth0/jwks-rsa-java/blob/0.21.2/LICENSE.

@dave2wave - are you able to provide any guidance on the licensing aspect here? Thanks!

github-actions · 2023-03-18T01:59:06Z

The pr had no activity for 30 days, mark with Stale label.

nodece · 2023-07-07T15:55:11Z

Closing this PR with #19849.

nodece · 2024-03-07T06:42:12Z

Use #22215 instead of this PR.

nodece added the area/authn label Nov 4, 2022

nodece added this to the 2.12.0 milestone Nov 4, 2022

nodece requested review from RobertIndie, Technoboy-, codelipenghui, mattisonchao, merlimat and michaeljmarshall November 4, 2022 08:24

github-actions Bot added the doc-required Your PR changes impact docs and you will update later. label Nov 4, 2022

codelipenghui assigned nodece Nov 11, 2022

Demogorgon314 approved these changes Nov 22, 2022

View reviewed changes

Demogorgon314 added the ready-to-test label Nov 22, 2022

Demogorgon314 closed this Nov 22, 2022

Demogorgon314 reopened this Nov 22, 2022

rdhabalia requested changes Dec 2, 2022

View reviewed changes

nodece mentioned this pull request Dec 7, 2022

PIP-226: Add JWKS support for AuthenticationProviderToken #18798

Open

sijie mentioned this pull request Dec 7, 2022

ISSUE-18798: PIP-226: Add JWKS support for AuthenticationProviderToken streamnative/pulsar-archived#5220

Open

tuteng reviewed Dec 7, 2022

View reviewed changes

[feat][authentication] Add JWKS support for AuthenticationProviderToken

7ed2770

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

nodece force-pushed the authentication-provider-jwks branch from 9dfe47c to 7ed2770 Compare December 8, 2022 03:06

github-actions Bot added the Stale label Jan 23, 2023

github-actions Bot removed the Stale label Feb 15, 2023

michaeljmarshall reviewed Feb 15, 2023

View reviewed changes

michaeljmarshall requested changes Feb 15, 2023

View reviewed changes

github-actions Bot added the Stale label Mar 18, 2023

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

nodece closed this Jul 7, 2023

damienburke mentioned this pull request Mar 6, 2024

Pulsar Authentication should support rotation of Validation keys e.g. Public keys used in JWT validation #8152

Open

nodece mentioned this pull request Mar 7, 2024

[improve][broker] PIP-226: Add JWKS support for AuthenticationProviderToken #22215

Open

4 tasks

Uh oh!

Conversation

nodece commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

Uh oh!

nodece commented Nov 4, 2022

Uh oh!

codecov-commenter commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rdhabalia left a comment

Choose a reason for hiding this comment

Uh oh!

codelipenghui commented Dec 7, 2022

Uh oh!

nodece commented Dec 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuteng Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jan 23, 2023

Uh oh!

nodece commented Feb 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

nodece commented Feb 15, 2023

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 18, 2023

Uh oh!

nodece commented Jul 7, 2023

Uh oh!

nodece commented Mar 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nodece commented Nov 4, 2022 •

edited

Loading

codecov-commenter commented Nov 22, 2022 •

edited

Loading

tuteng Dec 7, 2022 •

edited

Loading