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

Support --valid_pgpkeys option in Git module () #55396

Merged
merged 2 commits into from
Jun 5, 2019

Conversation

jelly
Copy link
Contributor

@jelly jelly commented Apr 16, 2019

Make Git module support --valid-pgpkeys option, which allows
configuring a list of valid PGP fingerprints which are compared with the
used PGP fingerprint if verify_commit is true. This requires
verify_commit to be set to 'yes'.

Signed-off-by: Jelle van der Waa jelle@vdwaa.nl

SUMMARY

Add an option to specify allows PGP fingerprints from which signed commits are excepted this adds an extra verification requirement when verify_commit is set. Basically this prevents a trusted repository with a malicious signed commit to be seen as a valid commit. This is comparable with Arch Linux's pacman's PKGBUILD which allows the same sort of syntax to specify valid PGP keys.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Git module

ADDITIONAL INFORMATION

Adds a new option to the Git module called valid_pgpkeys with an array of valid PGP keys to be used to verify the signed commit.

- name: test my new module
  hosts: localhost
  tasks:
  - name: 
    git:
      repo: 'https://github.com/archlinux/archweb.git'
      dest: /tmp/archweb
      verify_commit: yes
      version: release_2019-04-15
      valid_pgpkeys:
        - 'E499C79F53C96A54E572FEE1C06086337C50773F'

On error:

fatal: [localhost]: FAILED! => {"changed": false, "msg": "Public key \"E499C79F53C96A54E572FEE1C06086337C50773E\" missing from the whitelist", "rc": 0, ...

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. source_control Source-control category support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Apr 16, 2019
@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 Apr 16, 2019
lib/ansible/modules/source_control/git.py Outdated Show resolved Hide resolved
lib/ansible/modules/source_control/git.py Show resolved Hide resolved
lib/ansible/modules/source_control/git.py Outdated Show resolved Hide resolved
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Apr 17, 2019
@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Apr 17, 2019
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 17, 2019
@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 Apr 25, 2019
@jelly
Copy link
Contributor Author

jelly commented May 6, 2019

Any update?

@mkrizek mkrizek requested a review from webknjaz June 3, 2019 13:26
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

It's a nice patch, yet it requires a few bits of polishing.
There's a few style improvements I've suggested and we also 100% need to improve the consistency with the argument naming.

lib/ansible/modules/source_control/git.py Outdated Show resolved Hide resolved
lib/ansible/modules/source_control/git.py Outdated Show resolved Hide resolved
lib/ansible/modules/source_control/git.py Outdated Show resolved Hide resolved
lib/ansible/modules/source_control/git.py Outdated Show resolved Hide resolved
lib/ansible/modules/source_control/git.py Outdated Show resolved Hide resolved
@ansibot ansibot 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 Jun 5, 2019
@webknjaz
Copy link
Member

webknjaz commented Jun 5, 2019

@webknjaz webknjaz dismissed their stale review June 5, 2019 12:59

This has been addressed

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Waiting for the changelog fragment #55396 (comment) and docs review

@jelly
Copy link
Contributor Author

jelly commented Jun 5, 2019

@jelly thanks! Could you please also add a changelog fragment to this folder: https://github.com/ansible/ansible/tree/devel/changelogs/fragments

It's explained @ https://docs.ansible.com/ansible/devel/community/development_process.html#creating-a-changelog-fragment

Ok, added one, hope I did it correctly!

@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Jun 5, 2019
@ansibot

This comment has been minimized.

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jun 5, 2019
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jun 5, 2019
Make Git module support `--valid-pgpkeys` option, which allows
configuring a list of valid PGP fingerprints which are compared with the
used PGP fingerprint if verify_commit is true. This requires
verify_commit to be set to 'yes'.

Signed-off-by: Jelle van der Waa <jelle@vdwaa.nl>
@webknjaz
Copy link
Member

webknjaz commented Jun 5, 2019

I've polished the message a bit

@webknjaz webknjaz changed the title Support --valid_pgpkeys option in Git module Support --valid_pgpkeys option in Git module () Jun 5, 2019
@webknjaz webknjaz merged commit 7a3914b into ansible:devel Jun 5, 2019
@webknjaz
Copy link
Member

webknjaz commented Jun 5, 2019

Thanks @jelly for the patch and @acozine for the docs review!

@jelly jelly deleted the valid_pgpkeys branch June 5, 2019 19:27
@ansible ansible locked and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. source_control Source-control category support:community This issue/PR relates to code supported by the Ansible community. 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

6 participants