Automatically mask credentials from build log #2
Conversation
|
I am going to review this. And I agree on mocking the vault calls. I'll look into implementing that in the very near future. |
|
I took the time and ran the tests locally. I fixed them, they now assert to find echo **** in the log and assert to not find the secret value in the log. From my point of view this feature is now ready to merge. Better tests including a mocked vault would belong to a different PR in my opinion. |
|
@tobilarscheid I am getting the following error when using the plugin in a pipeline script: |
|
@ptierno I assume the problem is in the anonymous ConsoleLogFilter I defined, I will update it to be serializable. (Also, it might be cleaner to put that into a separate class anyways.) |
|
@ptierno I updated it. Can you please run it in your Jenkins instance to see if it works? |
|
@tobilarscheid I am still getting the same exception when using in a pipeline script |
|
Hi @ptierno , let me try to understand the issue. I just built a Version of the plugin like so: I set up a fresh Jenkins install (Version 2.19.2) with the latest version of the workflow / pipeline plugin. I uploaded the freshly built I then ran this pipeline: node{
wrap([$class: 'VaultBuildWrapper', vaultSecrets: [
[$class: 'VaultSecret', path: "secret/my", secretValues: [
[$class: 'VaultSecretValue', envVar: 'password', vaultKey: "password"]]]]]) {
echo env.password
}
}The pipeline successfully finishes and prints what I would expect it to print: What can we do to sort out why it doesn't work for you? |
|
@tobilarscheid i was using mvn hpi:run to run jenkins. let me check on another jenkins instance. |
|
For me, it even works in the jenkins started from |
|
@tobilarscheid strange. let me look into it a bit more. i want to have this merged and a release pushed asap. ill keep you updated |
|
Thank you. If I can be helpful in any way just let me know. We should definitely make sure every thing works fine before we release it. |
| * Created by tobiaslarscheid on 14.11.16. | ||
| */ | ||
| public class MaskingConsoleLogFilter extends ConsoleLogFilter implements Serializable{ | ||
| final Run<?, ?> build; |
There was a problem hiding this comment.
com.hudson.Run is not serializable. This is what is causing my errors. Change to String charsetName
There was a problem hiding this comment.
be sure to update all refs
| @@ -167,6 +164,11 @@ public void setUp(Context context, Run<?, ?> build, FilePath workspace, | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public ConsoleLogFilter createLoggerDecorator(@Nonnull final Run<?, ?> build) { | |||
| return new MaskingConsoleLogFilter(build, valuesToMask); | |||
There was a problem hiding this comment.
first aregument should be buld.getCharset().name()
| */ | ||
| public class MaskingConsoleLogFilter extends ConsoleLogFilter implements Serializable{ | ||
| final Run<?, ?> build; | ||
| private List<String> valuesToMask; |
There was a problem hiding this comment.
Be sure to add a private static final long serialVersionUID (ie 1L)
|
@tobilarscheid see my inline comments in the code. once you make those changes we should be good to merge. |
|
oh, and if you have the time please cleanup unused imports ;) |
|
Hi Peter, thanks for your valuable feedback! I incorporated all of it into the code. |
|
@tobilarscheid Thanks! im going to merge this now and push release either sometime today or early tomorrow. Thanks again! |
The vault plugin should mask any secrets obtained through it from the build login see JIRA Issue. This is already done by the regular jenkins credentials binding plugin - implementation is therefore heavily inspired by how it is done there.
Important: I was not able to run the tests, as it requires a properly initialized local Vault Instance. I would propose to rather mock the real Vault Instance for the Unit Tests to make them not only faster but also easier to run for contributors. Also, my change is actually breaking the test as the test asserts echo'ed output in the log.