Resolve credential leak via ps while jenkins-cli is used during puppe…#946
Resolve credential leak via ps while jenkins-cli is used during puppe…#946avbm wants to merge 2 commits intovoxpupuli:masterfrom
Conversation
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
Looking at this, there should be a wrapper define for the CLI to keep the logic central. Not strictly needed for this PR, but it'd be a nice improvement.
I've updated #937 to update this module to our current standard. I'm hoping to get it merged so we at least have working unit tests again. Ideally this should be rebased when that's merged.
manifests/cli.pp
Outdated
|
|
||
| if $::jenkins::cli_env_var { | ||
| $cmd_environment = [ | ||
| "JENKINS_USER_ID=${::jenkins::cli_username}", |
There was a problem hiding this comment.
Please use consistent indenting and remove the leading colons from variables. This is a repeated thing in the PR so I'll only comment it once :)
| "JENKINS_USER_ID=${::jenkins::cli_username}", | |
| "JENKINS_USER_ID=${jenkins::cli_username}", |
There was a problem hiding this comment.
fixed the indents using puppet-lint
manifests/init.pp
Outdated
| Boolean $purge_plugins = $jenkins::params::purge_plugins, | ||
| ) inherits jenkins::params { | ||
|
|
||
| validate_string($version) |
There was a problem hiding this comment.
We use data types and since you're not changing anything, I think it should already be covered. Please have a look at the parameters to verify.
There was a problem hiding this comment.
You are correct. I removed all the validate statements since using types makes them moot
manifests/init.pp
Outdated
| $_cli_auth_arg = "-auth '${cli_username}:${cli_password}'" | ||
| # username and password passed as environment variables to prevent showing in ps output | ||
| # so setting cli_auth_arg to empty string | ||
| $_cli_auth_arg = "" |
There was a problem hiding this comment.
| $_cli_auth_arg = "" | |
| $_cli_auth_arg = '' |
avbm
force-pushed
the
upstream_master_cli
branch
2 times, most recently
from
83a8f76 to
f38eb0b
Compare
|
I made updates based on your feedback @ekohl. Let me know if it looks good now. Also I have a question about commit/merge practices. I usually like to rebase and squash updates on feature/bugfix branches. ex. my current fixes were squashed into my previous commit and pushed to my fork. I was wondering if there was a best pracitces/recommend process around this for this repo/org. (ie should we squash in fixes or keep them as separate commits) |
avbm
force-pushed
the
upstream_master_cli
branch
3 times, most recently
from
1557d86 to
a51b5fa
Compare
|
Rebased on current master HEAD |
avbm
force-pushed
the
upstream_master_cli
branch
from
a51b5fa to
17de537
Compare
|
@ekohl or @bastelfreak : are any other chages/updates required in this PR? |
avbm
force-pushed
the
upstream_master_cli
branch
from
51af577 to
17de537
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
It would be nice to have some tests around this. Looks like right now there's not a single test around passing in a password. Other than that it looks correct.
avbm
force-pushed
the
upstream_master_cli
branch
7 times, most recently
from
88f5c74 to
26897d9
Compare
|
@ekohl , I added a couple of unit tests to ensure the auth args are added to enviornment when defined and they are absent when not defined. |
|
Hey @ekohl , just wanted to check if there were any updates required for this PR could be merged? |
avbm
force-pushed
the
upstream_master_cli
branch
from
9f11ff6 to
07035e4
Compare
|
Dear @avbm, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs? |
|
Dear @avbm, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
2 participants
…t runs
Pull Request (PR) description
Passes credentials to jenkins-cli as environement variables by default so passwords aren't visible in the output of ps during puppet agent runs.
This Pull Request (PR) fixes the following issues
Not sure if this has an associated issue