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.

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

Custom salt for ansible-vault encrypt (Fixes #35480) #71424

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

stawii
Copy link
Contributor

@stawii stawii commented Aug 24, 2020

SUMMARY

Please read #35480 for details.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

Ansible-Vault

ADDITIONAL INFORMATION

The encrypt() method was not deterministic due to random salt each time. This is simplest solution to fix this - let user define it's own salt in environment variable. It is user role to make sure salt is "good enough". Using dedicated class method gives possibilities for future extensions, ie. generating salt from plain text value.
By default, it acts as it was before.

@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Aug 24, 2020
lib/ansible/parsing/vault/__init__.py Outdated Show resolved Hide resolved
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Aug 25, 2020
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Aug 25, 2020
@stawii stawii requested review from bcoca and Akasurde Aug 26, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. support:community This issue/PR relates to code supported by the Ansible community. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Aug 26, 2020
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Sep 8, 2020
@stawii
Copy link
Contributor Author

stawii commented Sep 14, 2020

Please review changes.

@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. has_issue and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Dec 3, 2020
@mattclay
Copy link
Member

mattclay commented Dec 4, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 4, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. pre_azp This PR was last tested before migration to Azure Pipelines. core_review In order to be merged, this PR must follow the core review workflow. labels Dec 4, 2020
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 16, 2020
@ansibot ansibot removed the support:community This issue/PR relates to code supported by the Ansible community. label Mar 6, 2021
@aslafy-z
Copy link

aslafy-z commented Mar 9, 2021

Any updates on this topic? Any chance this gets backported to 2.9/2.10 then? Thanks!

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jul 4, 2021
@s-hertel
Copy link
Contributor

s-hertel commented Nov 12, 2021

@stawii Sorry this slipped through the cracks. There was a commit merged recently 363c1a3 which added vault filters that accept a user-provided salt. Do you think the global toggle is necessary to resolve #35480?

@stawii
Copy link
Contributor Author

stawii commented Nov 24, 2021

@s-hertel thanks for your patience. This PR seems to lag for everyone ;)

Anyway, I believe it is still needed. The merge you mentioned adds support for vault filters, and only there you can set the salt.
That means it doesn't work with ansible-vault command-line tool, which was covered by my solution. Of course, you can write playbook or just call Ansible to wrap the new filters, set custom static salt, read and write files and whatnot to emulate, but it's less convenient than using native command. Well.. in some cases it actually MIGHT be better, but in general it's workaround.
At this point my PR seems to have conflicts. If my arguments and proposed solution is still valid for you, please let me know and I'll update it to resolve conflicts. Otherwise please close it.

@bcoca
Copy link
Member

bcoca commented Jul 13, 2022

sorry for the delay, yes, it is ok to go forward with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.11 bug This issue/PR relates to a bug. has_issue needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants