jenkinsci / hashicorp-vault-plugin

Jenkins plugin to populate environment variables from secrets stored in HashiCorp's Vault.
https://plugins.jenkins.io/hashicorp-vault-plugin/
MIT License
217 stars 143 forks source link

fix: proper way of loading pattern secrets [SECURITY-3077] #309

Closed icep87 closed 1 year ago

icep87 commented 1 year ago

A fix for SECURITY-3077

Testing done

### Submitter checklist
- [ ] 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
jglick commented 1 year ago

Fixes #311. See https://issues.jenkins.io/browse/JENKINS-71788

jglick commented 1 year ago

@jetersen do you maintain this plugin? (@dineshba & @ptierno are also listed)

dineshba commented 1 year ago

@jglick Sorry, it's been a while I worked on this repo. So, hard for me to review this PR.

jetersen commented 1 year ago

@icep87 do you mind fixing the import order:

14:13:52  [ERROR] src/main/java/com/datapipe/jenkins/vault/log/MaskingConsoleLogFilter.java:[11,1] (imports) ImportOrder: Wrong order for 'java.util.regex.Pattern' import.
jetersen commented 1 year ago

Any reason this is a draft?

timja commented 1 year ago

Any reason this is a draft?

Probably because the tests fail:

[ERROR] com.datapipe.jenkins.vault.it.VaultConfigurationIT.shouldDealWithTokenBasedCredential  Time elapsed: 0.598 s  <<< FAILURE!
java.lang.AssertionError:

Expected: a string containing "echo ****"
     but: was "Legacy code started this job.  No cause information is available
Running as SYSTEM
Building in workspace /Users/timja/code/jenkins/hashicorp-vault-plugin/target/tmp/j h16743163402282501985/workspace/test
Retrieving secret: secret/path1
[test] $ /bin/sh -xe /Users/timja/code/jenkins/hashicorp-vault-plugin/target/tmp/jenkins6308559927767116713.sh
+ echo some-secret
some-secret
Finished: SUCCESS
"
icep87 commented 1 year ago

Yes, the tests fail with "Legacy code started this job. No cause information is available" I haven't gone around to looking into it.

I think I will be able to have a look at it today.

icep87 commented 1 year ago

Here are some findings on why the test fails:

- The following test should only actually test if the connection to Vault is working properly and not if variables are masked (as the variables in those tests are not coming from vault): shouldUseGlobalConfiguration(), shouldUseJobConfiguration(), shouldDealWithTokenBasedCredential()

~~- As far as I can see the test that are failing are not providing any values to mask, instead it's just printing a variable out: VaultConfigurationIT.java#L164 so when the initial class MaskingConsoleLogFilter.java#L25 is called, valuesToMask is empty. A variable just printed out in shell and not coming from vault should not be processed as secret.~~

~~- Should only variables passed in VaultBindingStep be masked VaultBindingStep.java#L126 ~~

If you agree with above, I will modify the test to include the variables from Vault instead of just shell printout.

Ive realized that those test are for when secrets are loaded from global configuration instead of actual step in the pipeline. I will continue on making it works.

bluesliverx commented 1 year ago

FWIW, we're using this commit in our local fork, and we no longer have errors with the latest credentials plugin.

jglick commented 1 year ago

Try #314.

icep87 commented 1 year ago

@jetersen I will close this in favour or https://github.com/jenkinsci/hashicorp-vault-plugin/pull/314