[JENKINS-37899] RemoteGitImpl: Take a snapshot of the credentials before passing them to the git client proxy

[cyrille-leclerc]

JENKINS-37899 RemoteGitImpl: Take a snapshot of the credentials before passing them to the git client proxy.

We need to snapshot of the credentials before passing them to the git client proxy so that the credentials are snapshotted before being serialized and transferred to the build agent through Jenkins remoting.

[cyrille-leclerc]

cc @jtnord @stephenc

[jtnord]

I've seen the patched code work, but am not 100% confident with the snapshot.

[jtnord]

So i have seen this work - but I'm at a bit of a loss as the snapshot provider would be the first that can handle StandardCredentials and the subclass may require a more specific type.
@stephenc is this correct use of this API?

[stephenc]

The supplied clazz is just used to maintain the type signature.

        Class bestType = null;
        CredentialsSnapshotTaker bestTaker = null;
        for (CredentialsSnapshotTaker taker : ExtensionList.lookup(CredentialsSnapshotTaker.class)) {
            if (clazz.isAssignableFrom(taker.type()) && taker.type().isInstance(credential)) {
                if (bestTaker == null || bestType.isAssignableFrom(taker.type())) {
                    bestTaker = taker;
                    bestType = taker.type();
                }
            }
        }
        if (bestTaker == null) {
            return credential;
        }
        return clazz.cast(bestTaker.snapshot(credential));

So the best fit (with the highest ordinal in case of equivalent fits) will win.

[stephenc]

🐝

[MarkEWaite]

@stephenc described once that there is a technique which could be used to create a test for this condition. Unfortunately, I don't recall the technique.

@stephenc, can you remind what the testing technique was that you suggested at the Jenkins World hackfest?

[jtnord]

🐝

[jtnord]

@MarkEWaite I recall he was referring to using the Jenkins Acceptance Test Harness with a docker fixture for your git server, possibly adding to the existing tests

[stephenc]

I have zero recollection other than us both complaining about stuff in the basement

[MarkEWaite]

I thought that you had described a technique available in some portion of JenkinsRule that would allow an agent (or a mock agent) to be launched and then for a job to be run on that agent. With that ability, a test might be possible which confirms that there was a problem prior to this change, and that the change has now fixed the problem.

[stephenc]

Ok I remember critiquing that tests using JenkinsRule.createOnlineSlave() might not give the correct answer as those tests still have access to the jenkins master's filesystem by virtue of running on the same machine and that might explain how you missed the issue.

But there do not seem to even be any such tests then that may explain how this issue was undetected. (esp as the three tests in git-plugin are not testing credentials)

[MarkEWaite]

You're correct that there are no tests which use JenkinsRule.createOnlineSlave(). There is the CredentialsTest that I created some time ago, and which I regularly use to identify working (and failing) authentication cases. The CredentialsTest does not use any remoting, and I was hoping to adapt CredentialsTest, or use createOnlineSlave to run useful remote tests.

[cyrille-leclerc]

Thanks @MarkEWaite