Restore CredentialsSnapshotTaker for special SSHUserPrivateKey impls

[jamesrobson-secondmind]

This restores the snapshot taker for ssh key creds that was removed in 18b3121. This is needed to fix an issue in hashicorp-vault plugin which currently prevents ssh keys being sent to agents which is being developed in jenkinsci/hashicorp-vault-plugin#218.

• 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

[jetersen]

Do we know the reason for the removal? @jtnord @jglick

[jtnord]

this was removed for security reasons (should be in the relevant security advisory).
sorry did not know about this at the time when I left the comment on the vault plugin.
https://www.jenkins.io/security/advisory/2018-06-25/#SECURITY-440

[jglick]

No, the snapshot taker is not a security risk. It was the non-direct-entry key sources which were a risk; and only they needed the snapshot taker in this plugin at least, so we deleted it as cleanup.

The question is jenkinsci/hashicorp-vault-plugin#218 (comment). From inspecting https://github.com/jenkinsci/credentials-plugin/blob/e05618c41e623cd974eef36b59e7c5bbda2a23c1/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java#L808-L821 it looks like it should work to define a snapshot taker for the interface type rather than the concrete impl type. (Trying to remember which way isAssignableFrom actually works gives me a headache.) Has this PR been tested with the Vault impl?

[jtnord]

@jglick are you completely sure
https://github.com/jenkinsci/ssh-credentials-plugin/blob/master/src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java#L372

IIUC they can still be used on the controller legitimately but not on agents, but you may not be able to create new types of these via the UI, in other words there may be some of these on the disk that have not been deleted and this would re-introduce the vulnerability whereby an agent can get the private keys of the controller.

Handled by the readResolve checking for super power so I am wrong here.

[jetersen]

Has this PR been tested with the Vault impl?

Yes, see user reporting success 👏 jenkinsci/hashicorp-vault-plugin#201 (comment)

[jetersen]

LGTM!

[jamesrobson-secondmind]

This might need more changes, for some reason doing a checkout inside a pipeline works, but not when you get the pipeline definition from SCM. That fails with

java.lang.IllegalStateException: Jenkins.instance is missing. Read the documentation of Jenkins.getInstanceOrNull to see what you are doing wrong.

I'm investigating this, but does anyone have any idea what the problem could be?

[jetersen]

That means that it gets executed on the agent, which ideally it should not.

[jamesrobson-secondmind]

Ignore my last comment, that job was using a username+password not the ssh key. So this PR should be good to go.

[jetersen]

These changes could be cleaned up

[jglick]

Looks right to me. Could have a unit (well, JenkinsRule) test proving that snapshot on a custom SSHUserPrivateKey impl with nonserializable fields produces a BasicSSHUserPrivateKey, but this would not be terribly interesting in isolation; the time would be better spent writing an actual integration test with Testcontainers in the Vault plugin proving that this fix permits remote checkout of a Git repository with SSH authentication from Vault.

Obsolete I think?

[jglick]

@jtnord do you plan to review this & jenkinsci/credentials-plugin#327 again?

[jetersen]

Regarding

the time would be better spent writing an actual integration test with Testcontainers in the Vault plugin proving that this fix permits remote checkout of a Git repository with SSH authentication from Vault.

The vault part would be easy but how would one go about testing remote checkout on an agent? Anything in jenkins-test-harness that would help?

[jglick]

how would one go about testing remote checkout on an agent

The agent part is easy; there are various examples of agents running via Testcontainers. I think you would need to run another container with an SSH-protected Git server. https://github.com/jenkinsci/acceptance-test-harness/blob/be0a8371600593fba1f728dd1de1a2994b763d9a/src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitContainer/Dockerfile shows this is feasible. Of course you also need to wire the agent container to be able to access the Git container as a logical hostname, which I hope is straightforward in Testcontainers.

[jetersen]

Could also use deploy keys to simply demonstrate that ssh auth works. No need to spin up a git server. 😅

[jglick]

use deploy keys to simply demonstrate that ssh auth works

You mean, against some github.com URL? This would make the test dependent on the continued existence and consistent configuration of some external server, so I would not recommend it. Better to have the test actually run the workflow that users presumably care about: retrieving an SSH private key from Vault, sending it to an agent in a snapshot form, and having the agent use this to authenticate to a Git server to clone a repository. (Bonus points for configuring Testcontainers to not allow the agent container to contact the Vault container, to prove that it is the controller which initially obtains the private key.)

[jglick]

@jetersen were you working on an integration test? Otherwise, has someone done a sanity check that the final version of this PR does work with the Vault plugin? (For username/password credentials, I just have one outstanding minor request: jenkinsci/credentials-plugin#327 (comment))

[jetersen]

@jetersen were you working on an integration test?

Nope.

Users have reported it working: jenkinsci/hashicorp-vault-plugin#201 (comment)

Although not with these minor changes. Although it should work.

[jglick]

Oh well. Merging and hoping for the best.