Open horazont opened 2 years ago
Merging #287 (432b1f1) into main (0bac5df) will decrease coverage by
0.44%
. The diff coverage is94.40%
.:exclamation: Current head 432b1f1 differs from pull request most recent head 5bdbd9b. Consider uploading reports for the commit 5bdbd9b to get more accurate results
@@ Coverage Diff @@
## main #287 +/- ##
==========================================
- Coverage 98.66% 98.22% -0.45%
==========================================
Files 73 68 -5
Lines 3679 3382 -297
Branches 313 291 -22
==========================================
- Hits 3630 3322 -308
- Misses 40 51 +11
Partials 9 9
Flag | Coverage Δ | |
---|---|---|
env_docker-default | 98.22% <94.40%> (-0.45%) |
:arrow_down: |
integration | ? |
|
sanity | ? |
|
target_ansible-doc | ? |
|
target_auth_approle | ? |
|
target_auth_aws_iam | ? |
|
target_auth_azure | ? |
|
target_auth_cert | ? |
|
target_auth_jwt | ? |
|
target_auth_ldap | ? |
|
target_auth_none | ? |
|
target_auth_token | ? |
|
target_auth_userpass | ? |
|
target_connection_options | ? |
|
target_controller | 82.31% <84.41%> (-0.95%) |
:arrow_down: |
target_filter_vault_login_token | ? |
|
target_import | ? |
|
target_lookup_hashi_vault | ? |
|
target_lookup_vault_ansible_settings | ? |
|
target_lookup_vault_kv1_get | ? |
|
target_lookup_vault_kv2_get | ? |
|
target_lookup_vault_login | ? |
|
target_lookup_vault_read | ? |
|
target_lookup_vault_token_create | ? |
|
target_lookup_vault_write | ? |
|
target_module_utils | 96.24% <47.61%> (-0.78%) |
:arrow_down: |
target_module_vault_kv1_get | ? |
|
target_module_vault_kv2_get | ? |
|
target_module_vault_login | ? |
|
target_module_vault_pki_generate_certificate | ? |
|
target_module_vault_read | ? |
|
target_module_vault_token_create | ? |
|
target_module_vault_write | ? |
|
target_modules | 79.31% <84.05%> (+0.69%) |
:arrow_up: |
units | 95.26% <87.20%> (-0.98%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
plugins/module_utils/_auth_method_approle.py | 85.71% <50.00%> (-3.76%) |
:arrow_down: |
plugins/module_utils/_auth_method_aws_iam.py | 92.59% <50.00%> (-1.64%) |
:arrow_down: |
plugins/module_utils/_auth_method_jwt.py | 92.00% <50.00%> (-3.66%) |
:arrow_down: |
plugins/module_utils/_auth_method_ldap.py | 85.71% <50.00%> (-3.76%) |
:arrow_down: |
plugins/module_utils/_auth_method_none.py | 93.33% <50.00%> (-6.67%) |
:arrow_down: |
plugins/module_utils/_auth_method_userpass.py | 86.95% <50.00%> (-3.52%) |
:arrow_down: |
plugins/module_utils/_hashi_vault_common.py | 99.13% <50.00%> (-0.87%) |
:arrow_down: |
plugins/lookup/vault_kv1_get.py | 100.00% <100.00%> (ø) |
|
plugins/lookup/vault_kv2_get.py | 100.00% <100.00%> (ø) |
|
plugins/lookup/vault_read.py | 100.00% <100.00%> (ø) |
|
... and 29 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Are there any instructions available on how to run the unittest? Invoking pytest yields
$ pytest tests
=============================================== test session starts ================================================
platform linux -- Python 3.10.4, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
rootdir: /home/jssfr/Builds/ansible_collections/community/hashi_vault
collected 0 items / 1 error
====================================================== ERRORS ======================================================
__________________________________________ ERROR collecting test session ___________________________________________
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1050: in _gcd_import
???
<frozen importlib._bootstrap>:1027: in _find_and_load
???
<frozen importlib._bootstrap>:1006: in _find_and_load_unlocked
???
<frozen importlib._bootstrap>:688: in _load_unlocked
???
/usr/lib/python3/dist-packages/_pytest/assertion/rewrite.py:170: in exec_module
exec(co, module.__dict__)
/home/jssfr/Builds/community.hashi_vault/tests/unit/conftest.py:14: in <module>
???
E ImportError: attempted relative import with no known parent package
============================================= short test summary info ==============================================
ERROR - ImportError: attempted relative import with no known parent package
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================= 1 error in 0.43s =================================================
ansible-test units
failed with:
$ ansible-test units
WARNING: Skipping unit tests on Python 3.6 because it could not be found.
WARNING: Skipping unit tests on Python 3.7 because it could not be found.
WARNING: Skipping unit tests on Python 3.8 because it could not be found.
Unit test modules with Python 3.9
ERROR: usage: __main__.py [options] [file_or_dir] [file_or_dir] [...]
__main__.py: error: unrecognized arguments: --forked -n tests/unit/plugins/modules/test_vault_kv1_get.py tests/unit/plugins/modules/test_vault_kv2_get.py tests/unit/plugins/modules/test_vault_login.py tests/unit/plugins/modules/test_vault_pki_generate_certificate.py tests/unit/plugins/modules/test_vault_read.py tests/unit/plugins/modules/test_vault_token_create.py tests/unit/plugins/modules/test_vault_write.py
inifile: /home/jssfr/Builds/ansible_collections/community/hashi_vault/.direnv/python-3.10.4/lib/python3.10/site-packages/ansible_test/_data/pytest/config/default.ini
rootdir: /home/jssfr/Builds/ansible_collections/community/hashi_vault
FATAL: Command "pytest --forked -r a -n auto --color yes -p no:cacheprovider -c /home/jssfr/Builds/ansible_collections/community/hashi_vault/.direnv/python-3.10.4/lib/python3.10/site-packages/ansible_test/_data/pytest/config/default.ini --junit-xml /home/jssfr/Builds/ansible_collections/community/hashi_vault/tests/output/junit/python3.9-modules-units.xml --strict-markers --rootdir /home/jssfr/Builds/ansible_collections/community/hashi_vault tests/unit/plugins/modules/test_vault_kv1_get.py tests/unit/plugins/modules/test_vault_kv2_get.py tests/unit/plugins/modules/test_vault_login.py tests/unit/plugins/modules/test_vault_pki_generate_certificate.py tests/unit/plugins/modules/test_vault_read.py tests/unit/plugins/modules/test_vault_token_create.py tests/unit/plugins/modules/test_vault_write.py" returned exit status 4.
That makes it a bit hard to write the tests to fix the coverage…
Hello again @horazont !
Are there any instructions available on how to run the unittest? Invoking pytest yields
I thought I had something in the contributor guide but it looks like I left out unit tests 😕 I've added an issue for that so I don't lose track of it https://github.com/ansible-collections/community.hashi_vault/issues/288
But luckily running units are a lot easier than running integration. Although the units are written in pytest, we run them through ansible-test
, so you can run:
ansible-test units --docker default
And that's basically it.
That makes it a bit hard to write the tests to fix the coverage…
The coverage also includes integration tests, so that's part of why coverage is lacking. Will want to see some integration tests too.
I would prefer you opening a discussion (or possibly issue) before a PR for large sweeping changes like this, so we can discuss the proposed change, and how it might be implemented, before putting in the work.
That said, I think the overall idea is good, I will need some time to think over the implementation.
Something I will probably want to see is a new option for controlling this behavior, and will probably default it to being off. The reason is that this is an additional request to Vault, and I would like that to be controllable, and to default to doing fewer calls such as in https://github.com/ansible-collections/community.hashi_vault/issues/248 . Especially since many folks will have low TTLs on their tokens and may want to allow them to expire on their own.
I'm trying to think if there's a possibility that a token would not have the ability to revoke itself, but probably they all have that ability? If you have confirmation let me know.
Thanks for all the guidance.
I would prefer you opening a discussion (or possibly issue) before a PR for large sweeping changes like this, so we can discuss the proposed change, and how it might be implemented, before putting in the work.
I understand that generally. In this case, before filing an issue, I wanted to evaluate the feasibility first on my own and ended up writing all strictly necessary code, so I thought I might as well put it up for discussion right away. I don't mind if we are going to discard all of this. If you prefer, we can move the discussion into an issue.
Something I will probably want to see is a new option for controlling this behavior, and will probably default it to being off.
Hmmmm... I'm not so happy with the default being "off", but I won't die on that particular hill. If it can be controlled via an environment variable so we can easily default it to "on" for all of our things, that's good enough for me.
The reason is that this is an additional request to Vault, and I would like that to be controllable, and to default to doing fewer calls such as in https://github.com/ansible-collections/community.hashi_vault/issues/248 . Especially since many folks will have low TTLs on their tokens and may want to allow them to expire on their own.
Fair enough (unfortunately, it's not possible to limit the number of uses for a token during login, that would be really neat; then we could just limit it to 1 for vault_read/write or N for the lookups and be done with it, without explicit revocation).
I'm trying to think if there's a possibility that a token would not have the ability to revoke itself, but probably they all have that ability? If you have confirmation let me know.
If a token is issued without the default
policy, or if a user modifies the default
policy to exclude permissions for the /auth/token/revoke-self
endpoint, a token will be unable to revoke itself.
Regarding running any tests, ansible-test units --docker default
doesn't seem to work either:
ansible-test units --docker default
I understand that generally. In this case, before filing an issue, I wanted to evaluate the feasibility first on my own and ended up writing all strictly necessary code, so I thought I might as well put it up for discussion right away. I don't mind if we are going to discard all of this. If you prefer, we can move the discussion into an issue.
Ok that's fine, the code is written and PR is up, so we can keep discussing here.
Something I will probably want to see is a new option for controlling this behavior, and will probably default it to being off.
Hmmmm... I'm not so happy with the default being "off", but I won't die on that particular hill. If it can be controlled via an environment variable so we can easily default it to "on" for all of our things, that's good enough for me.
Absolutely, this is the kind of option I would like to be settable by environment variable, INI (ansible.cfg
), and ansible variable, to ensure that it can be set "globally" easily. The situation is a little muddied for modules, but that's a more general Ansible issue, and it's why I created the vault_ansible_settings
lookup.
The reason is that this is an additional request to Vault, and I would like that to be controllable, and to default to doing fewer calls such as in #248 . Especially since many folks will have low TTLs on their tokens and may want to allow them to expire on their own.
Fair enough (unfortunately, it's not possible to limit the number of uses for a token during login, that would be really neat; then we could just limit it to 1 for vault_read/write or N for the lookups and be done with it, without explicit revocation).
I suppose this could be done intentionally by the end user. For example by creating the right policies and additional roles or configurations on the login types to be used. Like creating an approle, or an AWS IAM auth role, specifically for use with Ansible that limits token uses. I guess to your point, the number of uses will vary by plugin/module, so that's not so easy or feasible.
I'm trying to think if there's a possibility that a token would not have the ability to revoke itself, but probably they all have that ability? If you have confirmation let me know.
If a token is issued without the
default
policy, or if a user modifies thedefault
policy to exclude permissions for the/auth/token/revoke-self
endpoint, a token will be unable to revoke itself.
Perfect, thank you for confirming; had a feeling the default policy would play a part there. Something we will need to point out in notes (and another reason for the behavior to be controllable).
Regarding running any tests,
ansible-test units --docker default
doesn't seem to work either:
ansible-test units --docker default
WARNING: Skipping unit tests on Python 2.7 because it is not supported by this collection. Supported Python versions are: 3.6, 3.7, 3.8, 3.9, 3.10 WARNING: Skipping unit tests on Python 3.5 because it is not supported by this collection. Supported Python versions are: 3.6, 3.7, 3.8, 3.9, 3.10 Starting new "ansible-test-controller-UT3RNgvF" container. Adding "ansible-test-controller-UT3RNgvF" to container database. Error response from daemon: Container 01371249165d384185f66abc50556fd3aab74e97375dea5b694664bb97a8eaf5 is not running ERROR: Host <ansible_test._internal.host_profiles.DockerProfile object at 0x7f454b6a2f20> job failed: Command "docker exec -i ansible-test-controller-UT3RNgvF /bin/sh" returned exit status 1. ...
I believe this happens due to issues with systemd
. I am running on Windows with WSL, which is a real linux kernel and distro but without systemd so I have issues running containers that require it. More generally speaking, this means that the container that was started by the ansible-test
exited unexpectedly, so you could take a look and try to find out why, but in my experience there was no useful output.
Something in the upstream containers used changed in ansible-core
version 2.12
, so ever since then, I use 2.11
to run the tests locally on my machine. Not the best answer, but might be worth a try for you. You can set up a virtualenv with ansible-core>=2.11,<2.12
and then run the command there.
~If you want to try to report this upstream you can~ I read today in IRC they might be working on this, which would be great news! If they post something publicly I'll link it here.
Alternatively, ansible-test units --venv
can be used to try to have it manage its own venv for running the units. This doesn't use docker at all.
Thank you for your work on this @horazont !
Update: None of that works (--venv
fails to find PyYAML
) and it makes no sense, as I'm on normal Debian testing. I'm trying to get help for that in #ansible
on IRC...
Update: None of that works (
--venv
fails to findPyYAML
) and it makes no sense, as I'm on normal Debian testing. I'm trying to get help for that in#ansible
on IRC...
I have responded on IRC as well, but I should have mentioned to add --requirements
:
ansible-test units --venv --requirements
I believe that will install the necessary requirements in the venv.
It may still be better to create your own venv with ansible-core>=2.11,<2.12
and use --docker default
from there, because the docker container will do the unit tests against all versions of python, and is easier to run.
Now that I got the tests to work (by downgrading to ansible-core 2.11 as you suggested, the docker method then works), I pushed a commit with a proof-of-concept for:
I'd appreciate any feedback. In particular, I'm unsure about the placement of the integration tests. I put them into the auth method tests for now, because there I have access to already prepared credentials I can just readily use.
In addition, I'm only testing the vault_read
plugin+lookup, because I don't know how to test this with write and approle for instance (I don't know of a way to leak the token out of write).
From that perspective, it seems to be sensible to put them there; if you think they should be in the plugin/lookup tests and/or I should find ways to integration-test the other modules too, let me know.
Regarding unit testing, I'll add tests for the other modules in the meantime.
Regarding the option: I hope this is the correct way of handling that. Ansible option processing is still a bit opaque to me.
Thank you for contribution!✨
The docs for this PR have been published here: https://ansible-collections.github.io/community.hashi_vault/pr/287
You can compare to the docs for the main
branch here:
https://ansible-collections.github.io/community.hashi_vault/branch/main
The docsite for this PR is also available for download as an artifact from this run: https://github.com/ansible-collections/community.hashi_vault/actions/runs/3152453425
File changes:
M
collections/community/hashi_vault/hashi_vault_lookup.htmlM
collections/community/hashi_vault/vault_kv1_get_lookup.htmlM
collections/community/hashi_vault/vault_kv1_get_module.htmlM
collections/community/hashi_vault/vault_kv2_get_lookup.htmlM
collections/community/hashi_vault/vault_kv2_get_module.htmlM
collections/community/hashi_vault/vault_login_lookup.htmlM
collections/community/hashi_vault/vault_login_module.htmlM
collections/community/hashi_vault/vault_pki_generate_certificate_module.htmlM
collections/community/hashi_vault/vault_read_lookup.htmlM
collections/community/hashi_vault/vault_read_module.htmlM
collections/community/hashi_vault/vault_token_create_lookup.htmlM
collections/community/hashi_vault/vault_token_create_module.htmlM
collections/community/hashi_vault/vault_write_lookup.htmlM
collections/community/hashi_vault/vault_write_module.html
When true
, tokens created implicitly by auth methods will be revoked when the operation they are used for is completed.
For example, calling community.hashi_vault.vault_read with userpass
auth will perform a userpass
login to retrieve a token, perform a read with that token, then attempt to revoke the token so it can no longer be used.
Revocation is considered best-effort. Errors on revocaton will not result in execution failure. A warning will be emitted on revocation failure but in some circumstances, like non-revocation failures, the warning may not be displayed.
+Choices:
+false ← (default)
true
Configuration:
+INI entry:
+[hashi_vault_collection]
+revoke_ephemeral_token = false
+
Environment variable: ANSIBLE_HASHI_VAULT_REVOKE_EPHEMERAL_TOKEN
Variable: ansible_hashi_vault_revoke_ephemeral_token
Vault path to the secret being requested in the format path[:field]
.
timeout
integer
added in community.hashi_vault 1.3.0
@@ -677,7 +703,7 @@ seetoken_validate
boolean
added in community.hashi_vault 0.2.0
@@ -772,7 +798,7 @@ seeWhen true
, tokens created implicitly by auth methods will be revoked when the operation they are used for is completed.
For example, calling community.hashi_vault.vault_read with userpass
auth will perform a userpass
login to retrieve a token, perform a read with that token, then attempt to revoke the token so it can no longer be used.
Revocation is considered best-effort. Errors on revocaton will not result in execution failure. A warning will be emitted on revocation failure but in some circumstances, like non-revocation failures, the warning may not be displayed.
+Choices:
+false ← (default)
true
Configuration:
+INI entry:
+[hashi_vault_collection]
+revoke_ephemeral_token = false
+
Environment variable: ANSIBLE_HASHI_VAULT_REVOKE_EPHEMERAL_TOKEN
Variable: ansible_hashi_vault_revoke_ephemeral_token
timeout
integer
added in community.hashi_vault 1.3.0
@@ -684,7 +710,7 @@ seetoken_validate
boolean
added in community.hashi_vault 0.2.0
@@ -779,7 +805,7 @@ seeWhen true
, tokens created implicitly by auth methods will be revoked when the operation they are used for is completed.
For example, calling community.hashi_vault.vault_read with userpass
auth will perform a userpass
login to retrieve a token, perform a read with that token, then attempt to revoke the token so it can no longer be used.
Revocation is considered best-effort. Errors on revocaton will not result in execution failure. A warning will be emitted on revocation failure but in some circumstances, like non-revocation failures, the warning may not be displayed.
+Choices:
+false ← (default)
true
Secret ID to be used for Vault AppRole authentication.
timeout
integer
added in community.hashi_vault 1.3.0
@@ -418,7 +433,7 @@ see hvac library’s default is used.If no token is specified, will try to read the token_file from this path.
token_validate
boolean
added in community.hashi_vault 0.2.0
@@ -458,7 +473,7 @@ seeAuthentication user name.
When true
, tokens created implicitly by auth methods will be revoked when the operation they are used for is completed.
For example, calling community.hashi_vault.vault_read with userpass
auth will perform a userpass
login to retrieve a token, perform a read with that token, then attempt to revoke the token so it can no longer be used.
Revocation is considered best-effort. Errors on revocaton will not result in execution failure. A warning will be emitted on revocation failure but in some circumstances, like non-revocation failures, the warning may not be displayed.
+Choices:
+false ← (default)
true
Configuration:
+INI entry:
+[hashi_vault_collection]
+revoke_ephemeral_token = false
+
Environment variable: ANSIBLE_HASHI_VAULT_REVOKE_EPHEMERAL_TOKEN
Variable: ansible_hashi_vault_revoke_ephemeral_token
timeout
integer
added in community.hashi_vault 1.3.0
@@ -685,7 +711,7 @@ seetoken_validate
boolean
added in community.hashi_vault 0.2.0
@@ -780,7 +806,7 @@ seeWhen true
, tokens created implicitly by auth methods will be revoked when the operation they are used for is completed.
For example, calling community.hashi_vault.vault_read with userpass
auth will perform a userpass
login to retrieve a token, perform a read with that token, then attempt to revoke the token so it can no longer be used.
Revocation is considered best-effort. Errors on revocaton will not result in execution failure. A warning will be emitted on revocation failure but in some circumstances, like non-revocation failures, the warning may not be displayed.
+Choices:
+false ← (default)
true
Secret ID to be used for Vault AppRole authentication.
timeout
integer
added in community.hashi_vault 1.3.0
@@ -419,7 +434,7 @@ see hvac library’s default is used.If no token is specified, will try to read the token_file from this path.
token_validate
boolean
added in community.hashi_vault 0.2.0
@@ -459,7 +474,7 @@ seeAuthentication user name.
When true
, tokens created implicitly by auth methods will be revoked when the operation they are used for is completed.
For example, calling community.hashi_vault.vault_read with userpass
auth will perform a userpass
login to retrieve a token, perform a read with that token, then attempt to revoke the token so it can no longer be used.
Revocation is considered best-effort. Errors on revocaton will not result in execution failure. A warning will be emitted on revocation failure but in some circumstances, like non-revocation failures, the warning may not be displayed.
+Choices:
+false ← (default)
true
Configuration:
+INI entry:
+[hashi_vault_collection]
+revoke_ephemeral_token = false
+
Environment variable: ANSIBLE_HASHI_VAULT_REVOKE_EPHEMERAL_TOKEN
Variable: ansible_hashi_vault_revoke_ephemeral_token
timeout
integer
added in community.hashi_vault 1.3.0
@@ -672,7 +698,7 @@ seeThanks for your patience on this! I've spent some time looking it over today. I want to reiterate that I like the idea a lot, to have a way to proactively revoke the tokens.
Something is nagging me about the extra try/finally blocks we have to add everywhere, I am wondering if we can find a way to make this a little easier and less prone to forgetting it in new plugins. I'm also thinking about whether the revocation/logout functionality should belong to the authenticator class or elsehwere.
I've got a few ideas that I'm going to play with a little bit.
Now that I got the tests to work (by downgrading to ansible-core 2.11 as you suggested, the docker method then works), I pushed a commit with a proof-of-concept for:
- Integration testing
- Unit testing
- And the requested option
I'd appreciate any feedback. In particular, I'm unsure about the placement of the integration tests. I put them into the auth method tests for now, because there I have access to already prepared credentials I can just readily use.
The testing will ultimately depend on how we implement the functionality in the end, so let's see how much testing we do in the auth methods. The way it's currently implemented, the auth method is only responsible for returning whether that method should have its ephemeral token revoked.
In addition, I'm only testing the
vault_read
plugin+lookup, because I don't know how to test this with write and approle for instance (I don't know of a way to leak the token out of write).
The way I test some of these internals in integration tests is with special plugins and modules written for the tests instead of using the collection, that's where vault_ci_*
, vault_test_auth
, and vault_test_connection
come from (the setup_vault_test_plugins
integration target). It's a little early to tell if we should modify those, but those give us a way to get at some things we can't in the "productions" plugins, when it comes to the shared components and auth methods.
From that perspective, it seems to be sensible to put them there; if you think they should be in the plugin/lookup tests and/or I should find ways to integration-test the other modules too, let me know.
Right this is the tricky part. We really do need to test that all plugins and modules implement this functionality properly, because it's really easy to miss: everything "works" correctly if it's left out, and it's hard to see that the resulting token was not revoked. For this, I think it will fall largely on the unit tests to ensure that when they are called with the right options they call the right method(s), it's an indirect way to ensure that.
Regarding unit testing, I'll add tests for the other modules in the meantime.
You might want to wait a bit before adding more in the unit tests, let's see what we can come up with in tweaking the implementation a little, and then it should be a little clearer how to address the testing.
Regarding the option: I hope this is the correct way of handling that. Ansible option processing is still a bit opaque to me.
It's close, there's a few small things that need to be changed (default needs to be set in the docstring, plugins use that as code), needs version_added, we'll fix that up later as needed.
In the meantime, please rebase, I've fixed the issue that was causing devel
tests to fail in #291 and there seems too be some conflicts.
Hi @horazont , I apologize for further delays. I've had some things take up a lot of personal time recently. I'm trying to get caught up on pending work in the collection. I've just released version 3.2.0 which contains a fix for the sanity errors, and contains a new auth method, so I will ask you to rebase once again, and then I can go back to working through some of the ideas I was working on.
Thank you for your patience 🙏
No need to apologize, thanks for the reply. Did the rebase, worked without conflicts \o/.
@horazont could you ensure you've checked the box on this PR to allow maintainers up push to your branch/repository? I've tried but I got permission denied.
$ git push cloudandheat HEAD:feature/ephemeral-token-revocation
ERROR: Permission to cloudandheat/community.hashi_vault.git denied to briantist.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Please also rebase once more 🙏
Since I was not able to push to your branch, I pushed to a new one of my own (starting from yours, so it contains your changes too).
Here is my branch, compared to yours, so showing the things I changed: https://github.com/cloudandheat/community.hashi_vault/compare/feature/ephemeral-token-revocation...briantist:community.hashi_vault:pr/horazont/287
Here is my branch, compared to the collection main
so you can see all the changes together:
https://github.com/ansible-collections/community.hashi_vault/compare/main...briantist:community.hashi_vault:pr/horazont/287
Here's a summary of what I've done:
should_revoke_token
method, instead focusing on implementing a logout
method in the auth method classeslogout
method is present in the base class, with the default behavior of deciding whether to revoke based on the revoke_ephemeral_token
optiontoken
and none
auth methods both override logout
and do what they need to.HashiVaultAuthContext
class is introduced, providing a context manager approach around the client.get_context
method to return an instance of the above class.authenticate
methods are updated to return a HashiVaultAuthContext
class now instead of the raw response (the raw response is available via the raw
member of the context class).auth = module.auth.authenticate(...)
, which can now be used like so: with auth:
to have the logout
function called at the end of the context block.vault_token_create
shows a more advanced / fine-grained way of using it, where more control over when and if a logout occurs is desired; we can still revoke ephemeral tokens in this case if the requested new token is an orphan.I still consider this WIP; I have not touched the tests at all, so they are pretty broken now, but I wanted to stop here and see how you felt about this approach first. I may also consider some changes to omitted vs None
functionality in logout
's revoke_token
parameter.
Please let me know what you think!
Since I was not able to push to your branch, I pushed to a new one of my own (starting from yours, so it contains your changes too).
Thanks, I couldn't find the button :/
I've read through the diffs and I must say this is looking pretty good to me. Clever use of the context manager and moving the default into the base class also helps readability a lot.
To me, that looks great.
How would you like to proceed? I'm going to be on vacation for 14d starting on September 1st, and I probably won't get around to work on this before. I can pick it up at the end of September though.
Since I was not able to push to your branch, I pushed to a new one of my own (starting from yours, so it contains your changes too).
Thanks, I couldn't find the button :/
I think it's usually a checkbox at the bottom of the right sidebar, below or near the Subscribe/Unsubscribe button? Were you able to find it? If you could check that off, it would enable me to push changes, so they can run in CI and update the PR. If you already did it, thanks (I can only see the setting on my own PRs unfortunately, so I can't tell if you did it until I try to actually push)
I've read through the diffs and I must say this is looking pretty good to me. Clever use of the context manager and moving the default into the base class also helps readability a lot.
To me, that looks great.
Thank you!
How would you like to proceed? I'm going to be on vacation for 14d starting on September 1st, and I probably won't get around to work on this before. I can pick it up at the end of September though.
I see, that's no problem at all. If I can push to the branch, I may continue some work on it if/when I have time, or at least keep it rebased and up-to-date with any other changes, but we can definitely pick it up in earnest when you get back.
I hope you enjoy your time off :)
Ah, I figured it out. The source repository is owned by my employers org, and apparently that cross-repo pushing only works for personal repositories (emphasis mine):
People with push access to the upstream repository of a fork owned by a personal account can commit to the forked branches.
I suppose for the time of my vacation, it makes most sense that if you want to do anything, you do so in your branch and let me know here, then I'll force-pull from your branch once I pick this up after vacation?
The lack of token revocation (when we use JWT) is resulting in complaints from the ops team that runs Vault, when we utilize this module. Is there any guess when this may be resolved?
The lack of token revocation (when we use JWT) is resulting in complaints from the ops team that runs Vault, when we utilize this module. Is there any guess when this may be resolved?
Hi @brettjacobson , thanks for showing your support for this new feature! The PR author is on vacation at the moment but we've made some really good progress on it. In the end it depends on the time we both have to put toward it, but I'd like to see it out within about a month if everything works out.
In the meantime, there are two main possibilities for how to mitigate this in your environment:
vault_login
to perform your JWT login, and then make the rest of your calls from Ansible with the resulting token, using token
authThanks again, feel free to subscribe to this PR to get updates.
o/
Fear not, for I have returned!
@briantist Did you do anything in the past weeks I need to pull in from somewhere or can I go ahead as soon as it fits me to work on this?
o/
Fear not, for I have returned!
Welcome back!
@briantist Did you do anything in the past weeks I need to pull in from somewhere or can I go ahead as soon as it fits me to work on this?
I did not, I've unfortunately had an avalanche of things taking up my time 😭
The stuff I put up in my branch last we talked is how I left it. It would be great if we could get it into a place where I can push up changes as well, that be best for collaborating I think. Otherwise, go ahead! You'll likely need a rebase (and another after #308 ).
Hi again! :-)
I force-pulled from your branch, rebased, and then went and tried to fix all the unit tests. Do you think this is a valid approach at testing (see test_vault_write_logout
and the diff in 6bba91ee5a445a4fc9928c18ab26637cf26b0053 in general)?
If so, I'd go ahead and add unit tests for the other modules and lookups, and then follow up with the integration tests.
I accidentally the other unit tests, so I think this is now done except for linting and integration tests?
Integration tests are failing on orphan token stuff, and there was a PR for that while you were gone so I'm wondering if you didn't rebase against/merge from main? The PR was https://github.com/ansible-collections/community.hashi_vault/issues/210 where that stuff was changed.
That probably means that I messed up the merge. I'll get right to it.
@horazont it seems there are still issues here, are you looking to get these resolved soon? Please also note that #304 has just been merged which adds a new module, so revocation changes may need to be added to that as well.
@horazont Please also note that I will be releasing a new major version (4.0.0) of the collection soon with breaking changes. If you'd like this feature to be included in version 3.x.x we'll have to wrap it up within the next week or so... the absolute last day for me to release 4.0.0 is Nov 6, and if I merge any of the breaking changes before then, it will be too late for this to make it in 3.x.x.
If getting it added to 3.x.x is not important for you, then don't worry about the deadline.
@briantist Yeah, let's not worry about 4.0.0.
I'm still looking into the integration tests and will look into getting the lint right after that. Unfortunately, my ansible-test dev environment broke again a week or two back and I haven't gotten around to fixing it :/ (but will do so, eventually).
Does this mean we can expect this feature in a few weeks, in a 4.x?
@brettjacobson
Does this mean we can expect this feature in a few weeks, in a 4.x?
It means it will be in a 4.x.0 version (and not a 3.x.0 version), but timing still depends on availability for both of us. This is a busy time of year for a lot of people, myself included, but I look forward to getting it across the line.
Were you able to look into any of the workarounds I mentioned previously?
@briantist
Were you able to look into any of the workarounds I mentioned previously?
Can you link the workarounds? But I am pretty sure they don't work for us. Convincing 100s of internal customers to do workarounds isn't very viable for us.
@brettjacobson
Were you able to look into any of the workarounds I mentioned previously?
Can you link the workarounds? But I am pretty sure they don't work for us. Convincing 100s of internal customers to do workarounds isn't very viable for us.
https://github.com/ansible-collections/community.hashi_vault/pull/287#issuecomment-1246113038
@brettjacobson
Were you able to look into any of the workarounds I mentioned previously?
Can you link the workarounds? But I am pretty sure they don't work for us. Convincing 100s of internal customers to do workarounds isn't very viable for us.
Thanks but nah, our main problem lately is rate limiting, because so many repeate requests keep getting sent to our Vault server. Hopefully this PR gets shipped soon!
Are there any near term plans to finish this PR? Seems like there hasn't been any active development on it since end of September :(
Are there any near term plans to finish this PR? Seems like there hasn't been any active development on it since end of September :(
I certainly would like to have this feature as well.
@horazont how are you feeling about picking this up again? If we could get this on a branch where I have push access it would be helpful for collaboration.
Unfortunately, my ansible-test dev environment broke again a week or two back and I haven't gotten around to fixing it :/ (but will do so, eventually).
Some good news perhaps, is that a lot of improvements to ansible-test
's docker support have been merged, and so if you were unable to use it before with newer versions of ansible-core, this may be fixed for you.
Hey @horazont I wanted to check in once more and see if you're interested in working on this again. If not that's ok too, in that case I might take the current commits and put them on a new branch that I could commit to, to see if I could get it across the line, when I have some time.
Hi @briantist
As much as I hate leaving unfinished work, I don't see myself having enough time to finish this off.
I'm also ok if you drop the changes, I don't want to force them onto your future maintenance budget.
Thanks for checking in!
@horazont no problem, thanks for following up! I don't want to lose this work because I think it's an important feature, and clearly there are others interested in it. I'll look to put the work so far on my own branch and continue the work in a new PR, once I have some time available.
@briantist Do you foresee this work being resumed any time soon?
Do you foresee this work being resumed any time soon?
@brettjacobson Realistically, probably not for a few months at least going by what's currently on my plate. For this collection specifically my focus over the next month with be on releasing v5 ahead of the May 15 deadline for major version changes to be included in Ansible 8: https://docs.ansible.com/ansible/devel/roadmap/COLLECTIONS_8.html
I know you've been interested in this feature for a long time, so I'm sure it's disappointing to hear.
If you're at all interested in taking it up, I do try to prioritize other contributors' work, but I admit this change is bigger than most and going off the original branch will have a lot of conflicts to resolve, so I understand if you're reluctant to do so.
@briantist We've finally been able to implement the workaround per the docs of using a single login command using the JWT, then the lookup, then setting fact, to reduce the number of Vault lookups. Its just really painful to explain to our 100s of dev teams why they need to do the workaround and understand whats going on it. We had mostly been looking forward to the internal caching that would have taken place so that the number of vault logins was reduced to 1 just using the simple lookup.
We've finally been able to implement the workaround per the docs of using a single login command using the JWT, then the lookup, then setting fact, to reduce the number of Vault lookups. Its just really painful to explain to our 100s of dev teams why they need to do the workaround and understand whats going on it.
@brettjacobson glad to hear you've been able to work around it, I definitely agree that a workaround is not ideal, and that this is better handled within the plugins and modules.
We had mostly been looking forward to the internal caching that would have taken place so that the number of vault logins was reduced to 1 just using the simple lookup.
Just to clarify, the work here is to do automatic token revocation in the modules, but would not do any caching of the token or re-use it between plugin/module calls.
SUMMARY
For the modules and lookups which are not meant specifically about creating tokens for later use (e.g.
vault_read
,vault_write
, ..), this change adds revocation of the tokens generated by non-Token and non-None authentication methods.There is no reason to keep those tokens valid. Valid tokens consume space in Vault and may be a security risk if leaked somewhere. As tokens generally expire and it would require extra work (e.g.
vault_read
against the lookup/self endpoint) to obtain the token generated by this plugin, I don't consider this as a security issue as-is.When implementing this, I went for a helper function on the Authenticator, to keep the code in the modules more short and concise. The authentication modules themselves provide the information about the revokability of the token to the Authenticator, so that we can distinguish the Token method (where the token has been supplied externally and should not be revoked) and the other methods cleanly.
ISSUE TYPE
COMPONENT NAME
Authentication
ADDITIONAL INFORMATION
Before this change, if you ran
vault_read
againstauth/token/lookup/self
and used the token in the result, it would still be valid.After this change, the token is revoked before
vault_read
returns, preventing accidental later use.The
token_create
andlogin
modules/lookups are not affected, as those are clearly intended to obtain a token for later use.