Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make pass renderer configurable & other fixes #62120

Merged
merged 9 commits into from
Sep 27, 2022
Merged

Make pass renderer configurable & other fixes #62120

merged 9 commits into from
Sep 27, 2022

Conversation

dmach
Copy link
Contributor

@dmach dmach commented May 30, 2022 •
edited
Loading

What does this PR do?

The pass renderer becomes configurable.
Also several issues in the code have been fixed.

Previous Behavior

No changes to the existing behavior, new features must be explicitly enabled.

New Behavior

Config option pass_variable_prefix allows to distinguish variables that contain paths to pass secrets.
Config option pass_strict_fetch allows to error out when a secret cannot be fetched from pass.
Config option pass_dir allows setting the PASSWORD_STORE_DIR env for pass.
Config option pass_gnupghome allows setting the $GNUPGHOME env for pass.
Pass executable path from _get_path_exec() is used when calling the program.
The $HOME env is no longer modified globally.
Only trailing newlines are stripped from the fetched secret.
Pass process arguments are handled in a secure way.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@dmach dmach mentioned this pull request May 30, 2022
Closed
3 tasks
@dmach dmach force-pushed the make-pass-renderer-configurable branch from 7fa9464 to a569f0b Compare May 30, 2022 16:38
@dmach dmach marked this pull request as ready for review May 30, 2022 16:38
@dmach dmach requested a review from a team as a code owner May 30, 2022 16:38
@dmach dmach requested review from krionbsd and removed request for a team May 30, 2022 16:38
@dmach dmach mentioned this pull request Jun 1, 2022
Merged
meaksh
meaksh reviewed Jun 3, 2022
View reviewed changes
Copy link
Contributor

@meaksh meaksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! The code looks good to me 👍

It would be definetely great that you could also provide some tests here.

@dmach dmach force-pushed the make-pass-renderer-configurable branch 3 times, most recently from 981fb5a to 9415498 Compare July 22, 2022 13:43
meaksh
meaksh previously approved these changes Jul 25, 2022
View reviewed changes
Copy link
Contributor

@meaksh meaksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for the new enhancements and unit tests! 👍

LGTM

(BTW: the failing test does look related to the changes in this PR)

@dmach
Copy link
Contributor Author

dmach commented Jul 25, 2022

(BTW: the failing test does look related to the changes in this PR)

You mean this one? https://jenkins.saltproject.io/job/pr-macosx-catalina-x86_64-py3-pytest/job/PR-62120/6/
That failed in the tests.unit.utils.test_verify.TestVerify.test_max_open_files test - which is most likely unrelated.

@meaksh
Copy link
Contributor

meaksh commented Jul 26, 2022

@dmach sorry I meant it does not look related 😅 . Thanks!

@dmach
Copy link
Contributor Author

dmach commented Aug 23, 2022

@meaksh @krionbsd Is there something I can do to move this forward?

@meaksh
Copy link
Contributor

meaksh commented Aug 23, 2022

@Ch3LL any chance to have a review of this one? Thanks in advance!

Ch3LL
Ch3LL suggested changes Aug 29, 2022
View reviewed changes
salt/renderers/pass.py Show resolved Hide resolved
salt/renderers/pass.py Outdated Show resolved Hide resolved
salt/renderers/pass.py Show resolved Hide resolved
@Ch3LL Ch3LL added the Sulfur v3006.0 release code name and version label Aug 31, 2022
@dmach dmach force-pushed the make-pass-renderer-configurable branch from 05c1cd8 to f1ad67c Compare September 7, 2022 13:33
Ch3LL
Ch3LL previously approved these changes Sep 7, 2022
View reviewed changes
@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 7, 2022 •
edited
Loading

lint and pre-commit are failing. Once you get that fixed up we can get this one merged in.

@dmach dmach force-pushed the make-pass-renderer-configurable branch from f1ad67c to cd24609 Compare September 8, 2022 07:52
@dmach
Copy link
Contributor Author

dmach commented Sep 8, 2022

lint and pre-commit are failing. Once you get that fixed up we can get this one merged in.

Fixed (I hope). Other 2 tests are failing now, but I don't think they're related.

Ch3LL
Ch3LL previously approved these changes Sep 21, 2022
View reviewed changes
@dmach dmach requested review from meaksh and removed request for krionbsd September 22, 2022 14:10
@dmach
pass: Use a secure way of handling pass arguments
d44f8b2 
The original code would fail on pass paths with spaces,
because they would be split into multiple arguments.
@dmach
pass: Strip only trailing newline characters from the secret
b453a0b
dmach and others added 5 commits September 22, 2022 16:15
@dmach
pass: Do not modify $HOME env globally
d60b22a 
Just set $HOME for calling the pass binary
to avoid affecting anything outside the pass renderer.
@dmach
pass: Use pass executable path from _get_pass_exec()
9eed6da
@darix @dmach
Make the pass renderer more configurable
cbb1b68 
1. Allow us to make the pass renderer fail during pillar rendering
   when a secret corresponding with a pass path cannot be fetched.
   For this we add a master config variable pass_strict_fetch.

2. Allow to have prefix for variables that should be processed
   with the pass renderer.
   For this we add a master config variable pass_variable_prefix.

3. Allow us to configure pass' GNUPGHOME and PASSWORD_STORE_DIR
   environmental variables.
   For this we add master config variables pass_gnupghome and pass_dir.
@dmach
Add tests for the pass renderer
b116c6e
@dmach
pass: Handle FileNotFoundError when pass binary is not available
605e55c
@dmach dmach force-pushed the make-pass-renderer-configurable branch from cad2f29 to 605e55c Compare September 22, 2022 14:16
@Ch3LL
Copy link
Contributor

Ch3LL commented Sep 22, 2022

For some reason I cannot update the branch. Can you rebase and push? We shouldn't be seeing those test issues so lets re-base and start them again. thanks

@dmach
Copy link
Contributor Author

dmach commented Sep 22, 2022

For some reason I cannot update the branch. Can you rebase and push? We shouldn't be seeing those test issues so lets re-base and start them again. thanks

Done via GitHub webui.
I was also quite surprised seeing the test issues. Looks like a caching problem to me - jenkins did not know about the commit I just pushed.

@Ch3LL Ch3LL merged commit 98d1cdb into saltstack:master Sep 27, 2022
@welcome
Copy link

welcome bot commented Sep 27, 2022

Congratulations on your first PR being merged! 🎉

@lkubb lkubb mentioned this pull request May 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ch3LL Ch3LL Ch3LL approved these changes

@meaksh meaksh Awaiting requested review from meaksh

Assignees
No one assigned
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmach @meaksh @Ch3LL @darix