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

hashi_vault - refactor tests with mount_point capabilities. fix jwt mount_point options #31

Merged
merged 4 commits into from
Dec 21, 2020

Conversation

briantist
Copy link
Collaborator

SUMMARY

Fixes #29

  • Refactors tests to include testing for mount_point in a few ways:
    • First, each auth method is added to the test vault server twice, once with the default mount path, and once with an alternate path
    • The tests are set up now to be run through twice. once with each path
    • When testing the default path, the mount_point parameter is specifically left out of the calls to the lookup; this will catch a situation where the default path that's set via the vault CLI (when we set up the method) and the default path used by hvac (when we use it in the plugin), differ. It's an unlikely event, and one that isn't quite our place to fix but something we should be aware of.
    • More importantly, in the alt mount cycle, we do use the mount_point parameter. This ensures that the parameter is being sent into hvac when supplied. This catches the situation in Parameter 'mount_point' does not work with JWT auth #29 where we thought we were passing the parameter in but had the wrong name (in this case due to JWT doing something different JWT auth uses a non-standard parameter name for mount_point hvac/hvac#655)
    • This leaves one area that wouldn't be caught by the above two, which is the one described in lookup/hashi_vault.py: IAM auth does not support the mount_point parameter #7 : in that case, the mount_point parameter in the plugin was completely ignored. This results in the request being sent to vault with no mount_point, so using the default path. Since we just duplicate the policy in our test environment though, that would result in "alternate" mount point tests all being successful in this scenario, because under the hood they are using the default auth method mount which has all the permissions needed.
    • To catch that situation, I'm now creating two policies, with a new secret, secret5. The main test policy is denied access to that secret while the alternate policy has all the same access as the main one except that it can also read secret5. A new test was added to each auth method trying to access secret5 and expecting failure on the default mount while expecting success otherwise.
    • lookup/hashi_vault.py: IAM auth does not support the mount_point parameter #7 is a little bit special since we don't test the AWS auth method at all, but I'll introduce a new PR after this one that attempts to do some basic tests on the auth method (without actually interacting with AWS) to see what we can put in place, including trying to catch something like that.
  • As for the actual fix for the JWT method, that was of course the easiest part. Although I've run through the tests locally while developing this, I'm putting up the PR first without the JWT fix commit, expecting the new tests to catch the JWT error. I'll push that commit afterwards which should cause it to pass.
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

hashi_vault.py

ADDITIONAL INFORMATION

@briantist briantist added bug Something isn't working tests Adds or modifies tests labels Dec 19, 2020
@briantist briantist added this to the v0.2.0 milestone Dec 19, 2020
@briantist briantist added this to In progress in CI and Testing Improvements Dec 19, 2020
@briantist
Copy link
Collaborator Author

briantist commented Dec 20, 2020

briantist added a commit to briantist/community.hashi_vault that referenced this pull request Dec 20, 2020
@codecov
Copy link

codecov bot commented Dec 20, 2020

Codecov Report

Merging #31 (3fb7704) into main (a40f315) will increase coverage by 0.30%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   63.67%   63.98%   +0.30%     
==========================================
  Files           1        1              
  Lines         234      236       +2     
  Branches       41       42       +1     
==========================================
+ Hits          149      151       +2     
  Misses         71       71              
  Partials       14       14              
Impacted Files Coverage Δ
...ommunity/hashi_vault/plugins/lookup/hashi_vault.py 63.98% <0.00%> (+0.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a40f315...3fb7704. Read the comment docs.

@briantist
Copy link
Collaborator Author

@erikgb might you have a chance to review and/or test this?

Co-authored-by: Felix Fontein <felix@fontein.de>
@briantist briantist merged commit f993133 into ansible-collections:main Dec 21, 2020
@briantist briantist self-assigned this Dec 21, 2020
@briantist briantist moved this from In progress to Done in CI and Testing Improvements Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tests Adds or modifies tests
Development

Successfully merging this pull request may close these issues.

Parameter 'mount_point' does not work with JWT auth
2 participants