Make keypair format configurable #17

patrickpichler · 2022-09-17T08:20:15Z

This PR solves #4 by extending the credentials page with a combobox to select the desired
algorithm to create the key pair.

The base functionality is implemented and I now want to get some feedback from the
maintainers. Tests are currently missing and the existing tests still need some adaption.

It is now possible to choose between six different algorithms used for the keypair. Three
of those are RSA based and the other three EC. I found no easy way to recreate the
public key from the private EC key (I'm no expert on that matter, so if there is an
easy way I missed in my research, please let me know 🙂 ), hence
I introduced a new container object to store both private and public key as Jenkins secrets.
The previous private key secret is still around in order to ensure backwards compatibility
for java object serialization, but it is now marked transient.

As a base for creating key pairs for the different types, I leverage jjwt, since it already
provides a nice enum (SignatureAlgorithm) wrapping this functionality.

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

jglick

Looks about right from a quick glance. I am afraid I have not had the time to devote any effort to this plugin since its creation (and probably lack a good feel for how it is used in the field) so I would welcome new maintainers.

src/main/java/io/jenkins/plugins/oidc_provider/IdTokenCredentials.java

jglick · 2022-09-17T16:58:36Z

src/main/java/io/jenkins/plugins/oidc_provider/Keys.java

        return new JSONObject().
            accumulate("issuer", issuer).
            accumulate("jwks_uri", issuer + JWKS).
            accumulate("response_types_supported", new JSONArray().element("code")).
            accumulate("subject_types_supported", new JSONArray().element("public")).
-            accumulate("id_token_signing_alg_values_supported", new JSONArray().element("RS256")).
+            accumulate("id_token_signing_alg_values_supported", algValuesSupported).


I think this is only supposed to include the algorithm these credentials actually use. The spec is not terribly clear on what this is used for.

I am a bit confused by the documentation, since it mentions

The algorithm RS256 MUST be included
Does this mean a RS256 key must be present all the time?

Due to this, I went to listing all supported types by the plugin, but I can change it 🙂

src/main/java/io/jenkins/plugins/oidc_provider/Keys.java

patrickpichler · 2022-09-18T17:58:32Z

@jglick I've now also added some tests 🙂

patrickpichler · 2022-09-20T13:49:58Z

@jglick I think the PR is ready yet, could you have another look? 🙂
I would then proceed to make the git history nice and squash everything into a single commit 😅

jglick · 2022-09-24T14:18:25Z

src/main/java/io/jenkins/plugins/oidc_provider/IdTokenCredentials.java

+    @DataBoundSetter public void setAlgorithm(SupportedKeyAlgorithm algorithm) {
+        Objects.requireNonNull(algorithm);
+        this.algorithm = algorithm;
+    }
+
+    @NonNull public SupportedKeyAlgorithm getAlgorithm() {
+        return algorithm;
+    }


Propose doing #18 first, and then moving this option to IdTokenConfiguration, as I doubt there is a legitimate use case for a single controller having tokens with two issuers that use different algorithms; and a Jenkins admin would probably prefer to force use of a sufficiently secure algorithm.

The only reason I could think of to not make the algorithm global would be if certain
providers only support a subset of algorithms. Then it might make sense to keep it.

But then, I am by no means a expert in OIDC so maybe this is an unfounded fear 😄

I think everyone supports RS256 and IIRC it is mandated. Beyond that I am unsure.

What exactly do you mean by that? 🤔
That all keys must be signed by a RS256 key?

No, that all OIDC impls must support this algorithm at a minimum.

Right; #18 provides the global config class for another purpose, so if merged, that would be a place to move this option.

As said, I am not 100% sure if it makes sense to force the whole Jenkins instance to use a certain algorithm
for generating the key pair.

My fear is, as mentioned above, that certain OIDC connectors doesn't support elliptic curve key pairs. Instead
of now switching all keys to a non elliptic curve algorithm, it would make sense to me, to have a specialized key
pair for that provider which uses a supported key pair.

[It is possible] that certain OIDC connectors doesn't support elliptic curve key pairs

Right but in such a case why not stick with RS256, which all requesting parties should support? One possible reason is because you are a Jenkins admin who does not consider RS256 sufficiently secure and you want to force use of stronger cryptography. That would be undermined if the algorithm were overridable by developers in charge of some folders.

If there is no admin-vs.-dev distinction—one person or group has full config access to Jenkins—then it would make sense to switch to (say) ES512 to connect to one service that supports it, but stick with RS256 for another service which does not.

Not sure at this point what is best, just trying to think through the implications of letting the algorithm be configurable by people who are not Jenkins administrators. Of course all of the new options are stronger than the currently hard-coded option.

You are right, I guess that behavior will anyway be an edge case 🙂
I'll wait for your PR to be merged then and integrate my changes in the global configuration 🙂

wait for your PR to be merged

Merged (and released).

jglick · 2022-11-18T21:08:46Z

@patrickpichler do you still plan to work on this?

patrickpichler · 2022-11-18T21:27:20Z

Yes, I am still planning to implement this, but I need to first take a look at the changes in master.

patrickpichler · 2022-11-18T22:22:40Z

@jglick I've updated the branch. I am not 100% sure how I would go ahead and update all secrets
once the global plugin config is updated though 🤔
Do you have an idea?
https://github.com/jenkinsci/oidc-provider-plugin/pull/17/files#diff-8dfaabe0385e0b0a1de720d3227c8c8d0fdfd57d2f95426d2ea3eccc31984958R147

jglick · 2022-11-18T22:26:37Z

Just delete IdTokenCredentials.algorithm and retrieve the algorithm from global settings on demand. Then there is nothing to update.

patrickpichler · 2022-11-18T22:33:46Z

There is, as a update to the global algorithm should probably recreate the keypair used (if the algorithm changed). Otherwise the existing keypairs are out of sync with the global config .

jglick · 2022-11-21T12:57:41Z

Pending #3 you can simply resave existing credentials at any time.

patrickpichler · 2022-11-22T19:44:13Z

I've implemented the update version. Do you know of a better way of doing this? @jglick

src/main/java/io/jenkins/plugins/oidc_provider/config/IdTokenConfiguration.java

jglick reviewed Sep 17, 2022

View reviewed changes

jglick added the enhancement New feature or request label Sep 19, 2022

jglick mentioned this pull request Sep 24, 2022

Allow claims to be configured #18

Merged

9 tasks

jglick reviewed Sep 24, 2022

View reviewed changes

patrickpichler force-pushed the make-keypair-algo-configurable branch from 6c0d849 to b2e0f59 Compare November 18, 2022 22:21

patrickpichler force-pushed the make-keypair-algo-configurable branch from b2e0f59 to 018c16b Compare November 18, 2022 22:23

Patrick Pichler added 2 commits November 22, 2022 20:43

WIP

b9c4fc4

Update algorithm of credentials on settings update

91fe3dc

patrickpichler force-pushed the make-keypair-algo-configurable branch from d63ab1e to 91fe3dc Compare November 22, 2022 19:43

jglick reviewed Nov 22, 2022

View reviewed changes

src/main/java/io/jenkins/plugins/oidc_provider/config/IdTokenConfiguration.java Outdated Show resolved Hide resolved

Remove secret rotation

29dd1d4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make keypair format configurable #17

Make keypair format configurable #17

patrickpichler commented Sep 17, 2022 •

edited

Loading

jglick left a comment

jglick Sep 17, 2022

patrickpichler Sep 18, 2022

patrickpichler commented Sep 18, 2022

patrickpichler commented Sep 20, 2022

jglick Sep 24, 2022

patrickpichler Sep 24, 2022

jglick Sep 25, 2022

patrickpichler Sep 27, 2022

jglick Sep 27, 2022

jglick Sep 28, 2022

patrickpichler Sep 28, 2022 •

edited

Loading

jglick Sep 28, 2022

patrickpichler Sep 28, 2022

jglick Nov 1, 2022

jglick commented Nov 18, 2022

patrickpichler commented Nov 18, 2022

patrickpichler commented Nov 18, 2022

jglick commented Nov 18, 2022

patrickpichler commented Nov 18, 2022

jglick commented Nov 21, 2022

patrickpichler commented Nov 22, 2022

Make keypair format configurable #17

Are you sure you want to change the base?

Make keypair format configurable #17

Conversation

patrickpichler commented Sep 17, 2022 • edited Loading

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickpichler commented Sep 18, 2022

patrickpichler commented Sep 20, 2022

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

patrickpichler Sep 28, 2022 • 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

jglick commented Nov 18, 2022

patrickpichler commented Nov 18, 2022

patrickpichler commented Nov 18, 2022

jglick commented Nov 18, 2022

patrickpichler commented Nov 18, 2022

jglick commented Nov 21, 2022

patrickpichler commented Nov 22, 2022

patrickpichler commented Sep 17, 2022 •

edited

Loading

patrickpichler Sep 28, 2022 •

edited

Loading