Add support for HashiCorp Vault JWT auth #1213

erikgb · 2020-11-01T10:58:30Z

This PR is based on the work by @mbrancato in #154. The original pull request needs rebase, and there has been no feedback from @mbrancato since the PR was submitted in April 2020.

SUMMARY

This adds generic JWT/OIDC authentication support for the HashiCorp Vault lookup plugin. The JWT and OIDC auth only differ in the default path their methods will use. I.e., v1/auth/jwt versus v1/auth/oidc.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

hashi_vault

ADDITIONAL INFORMATION

The generic JWT auth API is used by the:

JWT auth - https://www.vaultproject.io/api-docs/auth/jwt#jwt-login
GCP auth - https://www.vaultproject.io/api-docs/auth/gcp#login
Kubernetes auth - https://www.vaultproject.io/api-docs/auth/kubernetes#login
even Azure and others...

ansibullbot · 2020-11-01T11:03:49Z

cc @briantist
click here for bot help

plugins/lookup/hashi_vault.py

briantist

Thanks for submitting this @erikgb !
My comments are mostly the same as on the original PR, in that we shouldn't rebrand a specific auth method as something more generic, based on the current implementation. But since then we have a generic JWT/OIDC method to use in HVAC. See my inline comments and the original PR.

plugins/lookup/hashi_vault.py

erikgb · 2020-11-01T16:06:53Z

@felixfontein @briantist Thank you for your comments and suggestions. My first goal was to rebase the original commits, so I know I am not done! 😄 I will look through your feedback and modify accordingly. And then ping you for another review.

briantist · 2020-11-01T16:08:30Z

@felixfontein @briantist Thank you for your comments and suggestions. My first goal was to rebase the original commits, so I know I am not done! 😄 I will look through your feedback and modify accordingly. And then ping you for another review.

Sounds great! Thanks again for picking this up @erikgb

erikgb · 2020-11-01T18:03:53Z

@briantist @felixfontein I think I am ready for another review! Please note that I haven't programmed a lot of Python, so please excuse me if doing stupid mistakes! 😉 Trying to learn something new every day!

One thing I am wondering about: The hvac documentation states that the jwt auth method must be "enabled": https://hvac.readthedocs.io/en/stable/usage/auth_methods/jwt-oidc.html#enabling. Is that something we need to take care of here? If so, how? I notice that the ldap auth method also must be "enabled" (https://hvac.readthedocs.io/en/stable/usage/auth_methods/ldap.html#enabling-the-ldap-auth-method), but I could not find anything in the code here....

felixfontein

This needs a changelog fragment. Everything else I can't really judge.

erikgb · 2020-11-01T22:10:11Z

@felixfontein Thanks, I've added a changelog fragment. I would also like to add some tests, but I am not sure how to set it up? I can see that there are a few tests for this lookup plugin, but features seems untested....

plugins/lookup/hashi_vault.py

briantist · 2020-11-01T23:38:52Z

@felixfontein Thanks, I've added a changelog fragment. I would also like to add some tests, but I am not sure how to set it up? I can see that there are a few tests for this lookup plugin, but features seems untested....

Thank you very much for considering tests!
There are a few cases that have missing tests, some of those cases were before I started working on the plugin, but one big instance is from me: the AWS IAM auth method. It didn't seem feasible to test that (in CI) since it needs some things set up in AWS that I don't know how I'd manipulate in Red Hat's environment :-/

It may be that this is the case for JWT also, but you may be able to get some inspiration from the PR that added JWT to HVAC, since it seems some tests were implemented there: https://github.com/hvac/hvac/pull/613/files

One thing I am wondering about: The hvac documentation states that the jwt auth method must be "enabled": https://hvac.readthedocs.io/en/stable/usage/auth_methods/jwt-oidc.html#enabling. Is that something we need to take care of here? If so, how? I notice that the ldap auth method also must be "enabled" (https://hvac.readthedocs.io/en/stable/usage/auth_methods/ldap.html#enabling-the-ldap-auth-method), but I could not find anything in the code here....

As far as I can tell, that's enabling the authentication method in the vault server, so not something we'd do in this plugin (as it's read-only and doesn't modify the state of Vault).

changelogs/fragments/1213-hashi_vault-jwt-auth-support.yaml

erikgb · 2020-11-02T07:27:25Z

As far as I can tell, that's enabling the authentication method in the vault server, so not something we'd do in this plugin (as it's read-only and doesn't modify the state of Vault).

Argh, of course. 😕 I forgot for a moment that hvac is a complete HashiCorp Vault API....

briantist · 2020-11-02T14:43:00Z

As far as I can tell, that's enabling the authentication method in the vault server, so not something we'd do in this plugin (as it's read-only and doesn't modify the state of Vault).

Argh, of course. 😕 I forgot for a moment that hvac is a complete HashiCorp Vault API....

You and me both, when you linked that I was really intrigued because I didn't remember having to enable any methods, took a little while to figure it out haha 😅

plugins/lookup/hashi_vault.py

erikgb · 2020-11-03T17:28:14Z

@briantist We really need some tests on this new feature! Any chance that you, or some of the others watching this PR can help me get started? 😄

briantist · 2020-11-03T20:27:20Z

@briantist We really need some tests on this new feature! Any chance that you, or some of the others watching this PR can help me get started? 😄

I don't use JWT auth myself, so I have very little familiarity with it. But I can help point you in the right direction on how the existing tests work, if you want to take a look at those and see if you have any questions.

It also seems like the person who implemented the JWT auth method in HVAC had some similar concerns about how to write tests for it and there may be some good stuff there as far as how we might set up something to test it within CI: https://github.com/hvac/hvac/pull/613/files

Tests are implemented as ansible roles, and you can see the tests for this plugin here: https://github.com/ansible-collections/community.general/tree/main/tests/integration/targets/lookup_hashi_vault

plugins/lookup/hashi_vault.py

briantist · 2020-11-14T20:46:44Z

tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/jwt_test.yml

+      register: test_wrong_cred
+      ignore_errors: true
+
+# FIXME: Somehow this test is failing. The role is getting access to secret3 - even if the policy explicitly denies it.


I'm going to try to pull this down and see if I can figure anything out with the tests locally.

And what a job! Great work @briantist! I just wished you did it sooner! 😉 Now ALL tests are green. In addition to following up on the registered hvac issue, I think we should improve our test setup (in another PR). We have to make sure no additional working hours are wasted on a "self-shot" test setup. I understand why Vault is booted in dev-mode, but I think we should have a look at the -dev-no-store-token option when starting the Vault server in dev-mode. And include the returned root-token in the vault_cmd variable. At least I think that could work....

We're planning to move this plugin to its own collection very soon, which will be a good place to improve testing. Especially since then the plugin won't be limited by the community.general test setup, but you can change the testing environment in whatever way you please.

Sorry took a while before I could find the time. Running these tests locally is kind of a pain because it's a custom/runme.sh type of test apparently. Just another thing we can hopefully improve by moving the plugin. Good idea on the -dev-no-store-token option, thanks for that! I hope you'll consider more contributions in the future :)

briantist · 2020-11-14T20:49:23Z

Thanks! But we probably have to decide what to do with OIDC. Take it out, or accept to include a feature without tests? Having an integration test for that seems like a lot of work. Ref. hvac/hvac#613 (comment)

I don't use JWT/OIDC so I'm a little unfamiliar. If the methods are implemented the same in HVAC except the difference being the mount/path, what makes OIDC more difficult to test? Apologies if this is a basic question.

Overall this is looking great @erikgb , I want to thank you once again for the effort here especially with regard to testing.

erikgb · 2020-11-14T21:30:18Z

I don't use JWT/OIDC so I'm a little unfamiliar. If the methods are implemented the same in HVAC except the difference being the mount/path, what makes OIDC more difficult to test? Apologies if this is a basic question.

Even if the methods are the same in hvac (and Vault), OIDC requires a server to be set up. So it is very hard to integration test - as noted in hvac/hvac#613 (comment). You need an OIDC provider....

briantist · 2020-11-14T22:26:12Z

I don't use JWT/OIDC so I'm a little unfamiliar. If the methods are implemented the same in HVAC except the difference being the mount/path, what makes OIDC more difficult to test? Apologies if this is a basic question.

Even if the methods are the same in hvac (and Vault), OIDC requires a server to be set up. So it is very hard to integration test - as noted in hvac/hvac#613 (comment). You need an OIDC provider....

Thank you, I see the issue now looking over the other PR.

I think I'm on the side of keeping OIDC and acknowledging that we aren't testing it (similar to AWS IAM auth). I'm going to start keeping track of things like that that don't have coverage to document why, or what the lift is, etc. But I think we're better off including it. Thank you!

briantist

I think I figured out the problem with the test. It's a good thing we didn't merge without it, it was literally the only test catching a critical error!

The problem comes from this login method not behaving like the others in HVAC. The others automatically set the token property of the client object when doing a login; this one doesn't. I submitted an issue about it: hvac/hvac#644

The reason this was so difficult to realize is that in CI we run vault server in dev mode. This really simplifies setting it up, avoiding the need for initializing and unsealing, and automatically authenticating all local requests with the root token.

When we test auth methods we're usually using the token retrieved by that login.

But because this auth method doesn't do that, all the requests to vault ended up using the original token the HVAC client inherited, which was the root token, and that's why the request to read the secret worked,

In a realworld scenario, it likely would have failed on reading the secret even when then auth worked; or worse it may have inherited some other token.

In #23 I fixed a bug where this happened on some failed auth, and it read your local client token, but it didn't fix this scenario because the token didn't come from manual action.

I need to look into how this token gets inherited because as far as I can tell it's not in an env var or file. The fix might actually be to force a .logout() on our HVAC client object before we do anything, so that it doesn't pull in something unexpected ever. It would have caught this sooner, as all the other reads would have failed too.

But anyway, that's another PR.

plugins/lookup/hashi_vault.py

briantist · 2020-11-15T02:02:18Z

I think I'm on the side of keeping OIDC and acknowledging that we aren't testing it (similar to AWS IAM auth). I'm going to start keeping track of things like that that don't have coverage to document why, or what the lift is, etc. But I think we're better off including it. Thank you!

Looking at the code more closely, and the examples in HVAC, it doesn't look like our OIDC method would work, as it seems th correct method is oidc_authorization_url_request() rather than jwt_login() method. Ami I reading that right?

https://hvac.readthedocs.io/en/stable/usage/auth_methods/jwt-oidc.html#oidc-authorization-url-request

With the uncertainty around that and the fact that we can't test it, I think I'm more comfortable reversing course and removing it from this PR.

Does that make sense to you @erikgb ? If you are able to test the plugin locally (outside of CI) with OIDC, let me know, that would give me more confidence in including it.

If not, no worries at all; let's remove it from this and it can be implemented another time (sorry, I know it was my suggestion to include it; I think I was mistaken about how similar they are).

erikgb · 2020-11-15T09:15:33Z

With the uncertainty around that and the fact that we can't test it, I think I'm more comfortable reversing course and removing it from this PR.

Does that make sense to you @erikgb ? If you are able to test the plugin locally (outside of CI) with OIDC, let me know, that would give me more confidence in including it.

I do agree, and I will remove the OIDC auth from this PR now. As I am not a administrator on our Vault installation, nor having access to a running OIDC provider, I do not have a simple way of verifying that it works.... And ideally this should be possible to set up in CI - it is just that I do not have to do it now. 😄

Co-authored-by: Brian Scholer <[email protected]>

erikgb · 2020-11-15T11:41:16Z

@felixfontein I see that a single test seems "flaky". How can we trigger a new CI Shippable build?

felixfontein · 2020-11-15T11:52:46Z

You can add a comment with /rebuild_failed. I'll trigger tests, that's a bit quicker :)

briantist

Looking good, thanks for sticking with this through the ups and downs.

Also it got lost in the main change but I want to thank you for the docstring / description updates too. Cheers!

* Add support for Hashicorp Vault JWT auth * Add support for HashiCorp Vault JWT auth (continued) Co-authored-by: Brian Scholer <[email protected]> Co-authored-by: Mike Brancato <[email protected]> Co-authored-by: Brian Scholer <[email protected]> (cherry picked from commit 64c6f20)

felixfontein · 2020-11-16T06:50:12Z

@mbrancato @erikgb thanks a lot for working on this!
@briantist thanks a lot for reviewing and testing and everything!
@Andersson007 also thanks for reviewing!

* Add support for Hashicorp Vault JWT auth * Add support for HashiCorp Vault JWT auth (continued) Co-authored-by: Brian Scholer <[email protected]> Co-authored-by: Mike Brancato <[email protected]> Co-authored-by: Brian Scholer <[email protected]> (cherry picked from commit 64c6f20) Co-authored-by: Erik Godding Boye <[email protected]>

erikgb · 2020-11-16T08:03:29Z

@felixfontein Thanks! Maybe a noob question: When will this feature be available in an Ansible installation?

felixfontein · 2020-11-16T08:07:27Z

@erikgb it will be included in community.general 1.3.0, to be released by the end of this month (probably mid next week or so), and that will get included in the next Ansible 2.10 release. These happen ~every three weeks, so one should be released next week as well. My aim would be that c.g 1.3.0 is included in it. So end of next week is a good estimate ;)

erikgb mentioned this pull request Nov 1, 2020

Add support for HashiCorp Vault JWT auth #154

Closed

ansibullbot added WIP Work in progress affects_2.10 feature This issue/PR relates to a feature request lookup lookup plugin needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels Nov 1, 2020

felixfontein reviewed Nov 1, 2020

View reviewed changes

briantist suggested changes Nov 1, 2020

View reviewed changes

plugins/lookup/hashi_vault.py Outdated Show resolved Hide resolved

plugins/lookup/hashi_vault.py Outdated Show resolved Hide resolved

plugins/lookup/hashi_vault.py Outdated Show resolved Hide resolved

ansibullbot removed the needs_triage label Nov 1, 2020

erikgb changed the title ~~WIP: Add support for HashiCorp Vault JWT auth~~ Add support for HashiCorp Vault JWT auth Nov 1, 2020

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed WIP Work in progress labels Nov 1, 2020

felixfontein reviewed Nov 1, 2020

View reviewed changes

briantist reviewed Nov 1, 2020

View reviewed changes

plugins/lookup/hashi_vault.py Outdated Show resolved Hide resolved

felixfontein reviewed Nov 2, 2020

View reviewed changes

changelogs/fragments/1213-hashi_vault-jwt-auth-support.yaml Outdated Show resolved Hide resolved

Andersson007 reviewed Nov 3, 2020

View reviewed changes

plugins/lookup/hashi_vault.py Outdated Show resolved Hide resolved

erikgb changed the title ~~Add support for HashiCorp Vault JWT auth~~ WIP: Add support for HashiCorp Vault JWT/OIDC auth Nov 3, 2020

ansibullbot added the WIP Work in progress label Nov 3, 2020

erikgb force-pushed the hashi_vault_add_jwt branch 2 times, most recently from 245fbe9 to eac72df Compare November 14, 2020 16:43

briantist suggested changes Nov 14, 2020

View reviewed changes

erikgb force-pushed the hashi_vault_add_jwt branch from eac72df to 279a58e Compare November 14, 2020 22:30

briantist suggested changes Nov 15, 2020

View reviewed changes

plugins/lookup/hashi_vault.py Outdated Show resolved Hide resolved

plugins/lookup/hashi_vault.py Outdated Show resolved Hide resolved

erikgb changed the title ~~Add support for HashiCorp Vault JWT/OIDC auth~~ Add support for HashiCorp Vault JWT auth Nov 15, 2020

Add support for HashiCorp Vault JWT auth (continued)

9147003

Co-authored-by: Brian Scholer <[email protected]>

erikgb force-pushed the hashi_vault_add_jwt branch from 7a260b5 to 9147003 Compare November 15, 2020 11:21

briantist approved these changes Nov 15, 2020

View reviewed changes

ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 15, 2020

felixfontein approved these changes Nov 16, 2020

View reviewed changes

felixfontein added the backport-1 label Nov 16, 2020

felixfontein merged commit 64c6f20 into ansible-collections:main Nov 16, 2020

patchback bot mentioned this pull request Nov 16, 2020

[PR #1213/64c6f20b backport][stable-1] Add support for HashiCorp Vault JWT auth #1309

Merged

briantist mentioned this pull request Dec 2, 2020

hashi_vault - token used for auth may inadvertently be inferred ansible-collections/community.hashi_vault#13

Closed

briantist mentioned this pull request Feb 24, 2021

hashi_vault - environment variable guidelines ansible-collections/community.hashi_vault#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for HashiCorp Vault JWT auth #1213

Add support for HashiCorp Vault JWT auth #1213

erikgb commented Nov 1, 2020 •

edited

Loading

ansibullbot commented Nov 1, 2020

briantist left a comment

erikgb commented Nov 1, 2020

briantist commented Nov 1, 2020

erikgb commented Nov 1, 2020

felixfontein left a comment

erikgb commented Nov 1, 2020

briantist commented Nov 1, 2020

erikgb commented Nov 2, 2020

briantist commented Nov 2, 2020

erikgb commented Nov 3, 2020

briantist commented Nov 3, 2020

briantist Nov 14, 2020

erikgb Nov 15, 2020

felixfontein Nov 15, 2020

briantist Nov 15, 2020

briantist commented Nov 14, 2020

erikgb commented Nov 14, 2020 •

edited

Loading

briantist commented Nov 14, 2020

briantist left a comment

briantist commented Nov 15, 2020 •

edited

Loading

erikgb commented Nov 15, 2020

erikgb commented Nov 15, 2020

felixfontein commented Nov 15, 2020

briantist left a comment

felixfontein commented Nov 16, 2020

erikgb commented Nov 16, 2020

felixfontein commented Nov 16, 2020

Add support for HashiCorp Vault JWT auth #1213

Add support for HashiCorp Vault JWT auth #1213

Conversation

erikgb commented Nov 1, 2020 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Nov 1, 2020

briantist left a comment

Choose a reason for hiding this comment

erikgb commented Nov 1, 2020

briantist commented Nov 1, 2020

erikgb commented Nov 1, 2020

felixfontein left a comment

Choose a reason for hiding this comment

erikgb commented Nov 1, 2020

briantist commented Nov 1, 2020

erikgb commented Nov 2, 2020

briantist commented Nov 2, 2020

erikgb commented Nov 3, 2020

briantist commented Nov 3, 2020

briantist Nov 14, 2020

Choose a reason for hiding this comment

erikgb Nov 15, 2020

Choose a reason for hiding this comment

felixfontein Nov 15, 2020

Choose a reason for hiding this comment

briantist Nov 15, 2020

Choose a reason for hiding this comment

briantist commented Nov 14, 2020

erikgb commented Nov 14, 2020 • edited Loading

briantist commented Nov 14, 2020

briantist left a comment

Choose a reason for hiding this comment

briantist commented Nov 15, 2020 • edited Loading

erikgb commented Nov 15, 2020

erikgb commented Nov 15, 2020

felixfontein commented Nov 15, 2020

briantist left a comment

Choose a reason for hiding this comment

felixfontein commented Nov 16, 2020

erikgb commented Nov 16, 2020

felixfontein commented Nov 16, 2020

erikgb commented Nov 1, 2020 •

edited

Loading

erikgb commented Nov 14, 2020 •

edited

Loading

briantist commented Nov 15, 2020 •

edited

Loading