jenkinsci / oidc-provider-plugin

OpenID Connect Provider Plugin for Jenkins
https://plugins.jenkins.io/oidc-provider/
MIT License
22 stars 13 forks source link

Add explicit option to generate new keypair #30

Open phuongdnguyen opened 1 year ago

phuongdnguyen commented 1 year ago

Add option to skip rotate keypair, more on https://github.com/jenkinsci/oidc-provider-plugin/issues/31

Testing done

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

Not sure I follow what this is doing, but it seems to be allowing a private key to be specified in CasC, which is not secure.

The requirement as I understand it is not to provide an option per se; it is to avoid rotating the keypair (IdTokenCredentials.privateKey) if you happen to recreate an identical list of credentials via configuration-as-code during startup rather than simply restarting the controller and loading credentials.xml. There are probably various approaches to making that work, but it should be transparent, and certainly not involve keeping keys in CasC. (If you actually delete $JENKINS_HOME and recreate the controller from scratch using CasC, then all your keypairs are going to be rotated, but I suppose that scenario is relatively infrequent and it is tolerable to expect clients to retrieve the keypairs anew.)

phuongdnguyen commented 1 year ago

Sorry if the PR title is misleading as i've restructure my thoughts later. What this PR is doing is to provide a check box in credential update to let user explicitly choose if they want to generate a new private key. It is the case as you've mentioned above: avoid rotating the keypair (IdTokenCredentials.privateKey) if you happen to recreate an identical list of credentials via configuration-as-code during startup rather than simply restarting the controller and loading credentials.xml

image

jglick commented 1 year ago

I am sorry, but it is unclear here what the intended interaction with CasC would be, and there is no end-to-end test of a CasC-based scenario that would prove that the approach works. There are a number of technical problems with this PR (I would likely just rewrite it if I had any time to maintain this plugin—currently not really), but it does not make sense to go into these details if I do not even know what your intended design looks like.

I think what you are attempting to do is look up an existing credentials item with the same id and then copy its keypair, but as written this is not going to work. First of all, the context could be folder-scoped. Second, the id will be randomly generated unless you happen to define one explicitly in CasC YAML, which would not be apparent to users (and there is no check to prevent you from uselessly declining key rotation while using a random id). Even if this worked, I doubt the UX is ideal—we would probably prefer to always keep keypairs during reconfiguration, but offer an explicit button to rotate (ideally moving towards #3).