[JENKINS-71788] Fix timing of SecretPatterns.getAggregateSecretPattern

[jglick]

Fixes #311, supersedes #309, triggered by jenkinsci/credentials-binding-plugin#260. Implementation is simpler than the WeakHashMap trick in jenkinsci/credentials-binding-plugin#28. Not tested other than whatever CI covers.

[icep87]

@jetersen Can you please approve this.

[icep87]

@jglick There seems to be errors when running the test on a higher version of Jenkins, I've updated the pom.xml with version 2.414. And I'm getting errors during tests:

Also: org.jenkinsci.plugins.workflow.actions.ErrorAction$ErrorId: 7816f80f-dfba-4cec-b784-ff6863c7269c java.lang.NullPointerException at com.cloudbees.plugins.credentials.CredentialsProvider.lambda$contextualize$1(CredentialsProvider.java:975) at java.logging/java.util.logging.Logger.log(Logger.java:1050) at java.logging/java.util.logging.Logger.warning(Logger.java:1893) at com.cloudbees.plugins.credentials.CredentialsProvider.contextualize(CredentialsProvider.java:975) at com.cloudbees.plugins.credentials.CredentialsProvider.findCredentialById(CredentialsProvider.java:921) at com.cloudbees.plugins.credentials.CredentialsProvider.findCredentialById(CredentialsProvider.java:856) at org.jenkinsci.plugins.credentialsbinding.MultiBinding.getCredentials(MultiBinding.java:195) at org.jenkinsci.plugins.credentialsbinding.impl.SSHUserPrivateKeyBinding.bind(SSHUserPrivateKeyBinding.java:100) at org.jenkinsci.plugins.credentialsbinding.impl.BindingStep$Execution2.doStart(BindingStep.java:132) at org.jenkinsci.plugins.workflow.steps.GeneralNonBlockingStepExecution.lambda$run$0(GeneralNonBlockingStepExecution.java:77) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:829) Finished: FAILURE

[jglick]

OK, I can take a look soon; which test was that?

hashicorp-vault-plugin/Jenkinsfile

Lines 2 to 3 in 0a1c04c

	[ platform: 'linux', jdk: '8' ],
	[ platform: 'linux', jdk: '11', jenkins: '2.332.2' ],

is quite old, and the plugin generally ought to be pulled up to a more current dependency set (Java 11, core perhaps 2.387.x).

[icep87]

It several of the tests, here are few of those that are failing for me:

com.datapipe.jenkins.vault.it.VaultGCRLoginIT.shouldRetrieveCorrectCredentialsFromVault
com.datapipe.jenkins.vault.it.VaultSSHUserPrivateKeyIT.shouldRetrieveCorrectCredentialsFromVault
com.datapipe.jenkins.vault.it.VaultStringCredentialIT.shouldRetrieveCorrectCredentialsFromVault
com.datapipe.jenkins.vault.it.VaultUsernamePasswordCredentialIT.shouldRetrieveCorrectCredentialsFromVault
com.datapipe.jenkins.vault.log.MaskingConsoleLogFilterSpec.shouldCorrectlyMask
com.datapipe.jenkins.vault.log.MaskingConsoleLogFilterSpec.shouldCorrectlyMaskOverlappingSecrets
com.datapipe.jenkins.vault.log.MaskingConsoleLogFilterSpec.shouldFilterNullSecrets

I've changed to following in the pom.xml:

  <properties>
    <changelist>9999-SNAPSHOT</changelist>
    <jenkins.version>2.414</jenkins.version>
    <hpi.compatibleSinceVersion>2.0.0</hpi.compatibleSinceVersion>
    <gitHubRepo>jenkinsci/${project.artifactId}</gitHubRepo>
  </properties>

    <dependencies>
      <dependency>
        <groupId>io.jenkins.tools.bom</groupId>
        <artifactId>bom-2.414.x</artifactId>
        <version>2373.v18527128f2a_c</version>
        <scope>import</scope>
        <type>pom</type>
      </dependency>
    </dependencies>

Maybe we should at least update it to the version when this change was implemented (Jenkins 2.403)
jenkinsci/credentials-binding-plugin@4ea6669

[jglick]

That is not related to this PR. I would advise avoiding Mockito generally, but if you must use it,

diff --git src/test/java/com/datapipe/jenkins/vault/it/VaultStringCredentialIT.java src/test/java/com/datapipe/jenkins/vault/it/VaultStringCredentialIT.java
index b3515b0..61c164b 100644
--- src/test/java/com/datapipe/jenkins/vault/it/VaultStringCredentialIT.java
+++ src/test/java/com/datapipe/jenkins/vault/it/VaultStringCredentialIT.java
@@ -17,6 +17,7 @@ import org.jvnet.hudson.test.JenkinsRule;
 import static com.datapipe.jenkins.vault.it.VaultConfigurationIT.getShellString;
 import static com.datapipe.jenkins.vault.it.VaultConfigurationIT.getVariable;
 import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -34,6 +35,7 @@ public class VaultStringCredentialIT {
         VaultStringCredential up = mock(VaultStringCredential.class);
         when(up.getId()).thenReturn(credentialsId);
         when(up.getSecret()).thenReturn(Secret.fromString(secret));
+        when(up.forRun(any())).thenReturn(up);
         CredentialsProvider.lookupStores(jenkins.getInstance()).iterator().next()
             .addCredentials(Domain.global(), up);
         WorkflowJob p = jenkins.createProject(WorkflowJob.class, jobId);

(Apparently it does not handle interface default methods correctly, or perhaps this plugin is just using an outdated version of Mockito.)

[icep87]

@jglick In that case perfect :D Can we get this merged?

[jglick]

I think @jetersen is the maintainer?

[icep87]

@jetersen any update on approving this?

[icep87]

@jetersen Can you merge this after approval?

[jglick]

BTW https://plugins.jenkins.io/hashicorp-vault-plugin/ still warns https://www.jenkins.io/security/advisory/2023-05-16/#SECURITY-3077 which I think is obsolete? Can we get rid of this warning?

[daniel-beck]

@jglick Thanks, we didn't get notified as described in https://www.jenkins.io/security/plugins/#followup, I'll take care of this in a moment.