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 - environment variable guidelines #10

Open
briantist opened this issue Nov 11, 2020 · 8 comments
Open

hashi_vault - environment variable guidelines #10

briantist opened this issue Nov 11, 2020 · 8 comments

Comments

@briantist
Copy link
Collaborator

briantist commented Nov 11, 2020

SUMMARY

The hashi_vault lookup takes several parameters via environment variables, and recently, some via INI (ansible.cfg) as well.

Most of the original vars were intended to re-use vars that are commonly used in the vault CLI., like VAULT_ADDR and VAULT_TOKEN.

Along the way some newer ones have been added that aren't related to the vault CLI but are strictly for the plugin, but also adopted the VAULT_ prefix, and some others used ANSIBLE_VAULT_ as their prefix.

An additional consideration is that in Ansible, environment variables have higher precedence (are preferred) over INI values. We can think of this in terms of specificity; the INI value is defined "ansible-wide" (for your specific implementation), while the env var is defined per invocation so it should be considered more specific, especially given that the env vars defined for plugin parameters are specific to that Ansible plugin (usually denoted with the ANSIBLE_ prefix) and so have no use outside of that context.

The precedence issue becomes less clear when we take values that have meaning outside of ansible, like the VAULT_ vars used in the vault CLI.

I believe we should consider then that an env var with meaning outside of Ansible has a lower precedence (is less preferred) than an INI value defined in ansible.cfg, as in the context of Ansible, an INI value is a more specific value.

Additional background on the ANSIBLE_ prefix:

PROPOSAL

This proposal aims to provide guidelines on how we name environment variables, and how we handle their precedence.

Ansible-only vars

This applies to a parameter to the plugin that will not inherit any existing vault CLI env var.

  1. The variable should use the prefix ANSIBLE_HASHI_VAULT_. This name is on the long side, however it keeps to the pattern of starting with ANSIBLE_. Also considered (and even used before this proposal) was ANSIBLE_VAULT_ however this may cause confusion with Ansible Vault (unrelated to this plugin).
  2. This variable should follow existing Ansible precedence rules, and be specified in the env: key in the plugin's argspec, so that the standard plugin loader can take care of it.

Even if there is no specific existing VAULT_ env var, we don't know whether one will be introduced in the future, and so don't want to use these names.

Variables used outside of Ansible

This applies to env vars that have a meaning outside of Ansible, but whose value we want to use. Examples include the vault CLI env vars, but also the $HOME variable as used for a default for the token_path parameter.

  1. As far as naming, we of course have no choice but to use the name as it exists.
  2. We should NOT specify this value in the argspec. Instead, we must write code that retrieves this value in the case that the parameter was not already set via any other means. This enforces lowest precedence.
  3. Consider adding an additional ANSIBLE_HASHI_VAULT_ var (following guidelines above to add it to the argspec) so that the value can be overridden for specific Ansible invocations, overriding both the non-Ansible var and INI value.
RELATED ISSUES & PRs

See this project: https://github.com/ansible-collections/community.hashi_vault/projects/1

Not all will be listed here, but want to point some specific examples.

COMPONENT NAME

hashi_vault.py

@ansibullbot

This comment has been minimized.

@ansibullbot

This comment has been minimized.

@dmsimard dmsimard transferred this issue from ansible-collections/community.general Dec 1, 2020
@briantist briantist changed the title hashi_vault - environment variable guideline proposal hashi_vault - environment variable guidelines Dec 2, 2020
briantist added a commit to briantist/community.hashi_vault that referenced this issue Dec 21, 2020
- Adds `ANSIBLE_HASHI_VAULT_ADDR` env
- Moves `VAULT_ADDR` env to `LOW_PRECEDENCE_ENV_VAR_OPTIONS`
- Resolves ansible-collections#10
@pilou-
Copy link

pilou- commented Feb 24, 2021

Should not the support of environment variables starting with ANSIBLE_HASHI_VAULT_ be removed ?

  • VAULT_ADDR and VAULT_TOKEN are taken in account by hvac library, the lookup doesn't need to handle them too
  • ansible precedence rules between plugin parameters and environment variables don't concern environment variables not handled by ansible
  • hvac python module already defines a default value for the vault address parameter, ansible should not use another value and doesn't need to use a default value at all.
  • in order to define address and token parameters through environment variables, users can use another environment variable of their choice: "{{ lookup('community.hashi_vault.hashi_vault', 'secret=secret/whatever', url=lookup('env', 'MY_PREFIX_VAULT_ADDR')) }}"

@briantist
Copy link
Collaborator Author

Hi @pilou- , thanks for commenting.

I think the issue with relying on underlying defaults, especially for options we require, is that it makes for an unclear or inconsistent user experience on the Ansible side.

As it relates to VAULT_TOKEN, this has already happened: #13

I found in using the plugin that when my auth method failed, the failure didn't stop it from operating because I had a token in a place that hvac could read it from; really not a good experience, and not intuitive at all. In another case, a new auth method was added and passed all the tests, meanwhile it didn't work at all: the token in the test suite was being inferred by hvac and caused the tests to pass (we updated the tests too, but again this was unintuitive). And so for example we now forcefully logout to prevent this behavior.

In my opinion, the defaults in hvac are a worse case of this than the Vault CLI, because as a library, end users aren't really aware of it.

And so even though we are duplicating some functionality by processing it within Ansible, the purpose of this is to provide a consistent experience, especially within a specific release.

This isn't always cut and dry: for example when the proxies option was added recently, I became aware of the ability to set proxies via env vars that would be used not even by hvac but by requests (as used by hvac). It doesn't always make sense for us to overwrite/block/intercept those, but in some cases, it does. For sure it can be case by case.

Your last example, is always an option for anyone regardless of what we do or don't process as far as env vars go.
As it pertains to lookup plugins though, it can be quite inconvenient, because when you have many lookups, all using the same params for things like url, auth_method, etc., you must repeat it on every lookup. Mitigated somewhat by assigning to an ansiblr var first, and doing url=myurl but still, you must have even that small snippet on every call.

Env vars solve some of that. I'll be looking to add vars: as another possible way to achieve that, by setting ansible vars instead of env vars, which offer even more flexibility for setting these in a single place, and to override them in specific contexts.

Unlike modules (tasks), we don't have as many ways to set these things for lookups.


Those things aside, I would like to know more about your position. Does the presence of these env vars cause issues for your use cases?

@pilou-
Copy link

pilou- commented Feb 27, 2021

About #13 the unix user used by the lookup tests should not be the user executing the vault service.

Those things aside, I would like to know more about your position.

The ANSIBLE_HASHI_VAULT_ prefixed variables seems a burden to maintain. Other lookup plugins (aws, k8s) don't use such variables.

Does the presence of these env vars cause issues for your use cases?

No.

@jghal
Copy link

jghal commented May 11, 2021

We're using AWX, and trying to find the best path forward to provide the AppRole credentials to this lookup plugin in an AWX context. While the vault CLI doesn't appear to have env variables for AppRole authentication, this plugin does. So we were looking at custom Credential Type with injectors. However AWX does not permit env variable injectors with the ANSIBLE_ prefix.

image

We would prefer using env variables over extra_vars as we then don't need to provide the authentication parameters in calls to the lookup plugin.

    {{ lookup('community.hashi_vault.hashi_vault', \
      'secret=secret/data/foo:my_secret', \
      auth_method='approle', \
      role_id=ict_vault_role_id, \
      secret_id=ict_vault_role_secret_id) \
    }}"

vs

{{ lookup('community.hashi_vault.hashi_vault', 'secret=secret/data/foo:my_secret' }}"

@grnrk
Copy link

grnrk commented May 11, 2021

@jghal Take a look at #49

@jghal
Copy link

jghal commented May 11, 2021

@h3m5k perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants