credentials-plugin icon indicating copy to clipboard operation
credentials-plugin copied to clipboard

[JENKINS-70101] Revive ability to snapshot() the CertificateCredentials so they can be used on remote agents

Open jimklimov opened this issue 2 years ago • 13 comments

As detailed in https://issues.jenkins.io/browse/JENKINS-70101 I've hit a problem using certificate-based user authentication in http-request-plugin (noticed as part of my work on https://issues.jenkins.io/browse/JENKINS-70000 in https://github.com/jenkinsci/http-request-plugin/pull/120 but also reproduced with latest 1.16 release of that plugin).

After a week of reproductions and tracing across different plugins, the root of the problem crystallized as the use of SecretBytes in UploadedKeyStoreSource and effective lack of special snapshot() support. As a consequence, serialized copies of the key store used the same SecretBytes as on the Jenkins Controller, but being on a different JVM in the remote agent case, they used a different instance of the static KEY field with its own secret field. Although the SecretBytes are diligently copied (took a lot of effort to confirm that as seen in sibling branch https://github.com/jimklimov/credentials-plugin/tree/JENKINS-70101-trace), they are unreadable on the remote agent.

During investigation I found that credentials-plugin did have an implementation for the Certificate Credentials Snapshot Taker, but it was removed by https://github.com/jenkinsci/credentials-plugin/commit/40d0b5cc53c265b601ffaa4469310fad390a80fb as part of deprecation of FileOnMasterKeyStoreSource subclass. Also an earlier history of the plugin involved the Secret class which effectively stores a base64 string and only encodes/decodes it on demand -- this was superseded by SecretBytes but some handling for conversion from older versions remained. This code proved to be a good starting point for fixing the problem, although not without some further work:

  • originally it consulted isSnapshotSource (forced true for UploadedKeyStoreSource subclass) and just returned the original credential if so;
  • even with that check disabled, the snapshot used SecretBytes for the copied key store and suffered the same fate - not usable on agent.

This PR adds tests (failing at first to confirm the problem), makes use of older codebases and new verified fixes:

  • UploadedKeyStoreSource.isSnapshotSource() depends on Channel.current() == null (so for remoting-related snapshots it is false and shortcuts to return this are not taken)
  • CertificateCredentialsSnapshotTaker was revived as a separate source file and standalone class, similar to existing UsernamePasswordCredentialsSnapshotTaker
  • UploadedKeyStoreSource was modified to handle again a Secret uploadedKeyStore field (and provide data from it if SecretBytes uploadedKeyStoreBytes are currently null). As part of that, transient and final modifiers on these fields had to go.
  • CertificateCredentialsSnapshotTaker was modified to create the new UploadedKeyStoreSource instance with Secret version rather than SecretBytes (as CredentialsSnapshotTaker docs stipulate, "all the details are captured within the credential")

Thanks to @mawinter69 and @slide for bright ideas and pointers, and general sympathy as I woed on the chat :)

Separately note that this PR adds tests using a separate JVM for the build agent to reproduce the problem. This code may be worth exporting into some JenkinsAgentRule if someone is up for it.

  • [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
  • [x] Link to relevant pull requests, esp. upstream and downstream changes
  • [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

NOTE: I'll post commits in several phases, so CI has its chance to fail with the tests and show the original problem in logs.

jimklimov avatar Nov 21 '22 13:11 jimklimov