maxhoesel-ansible / ansible-collection-smallstep

Unofficial Ansible Collection for Smallstep CLI and the step-ca server
https://github.com/smallstep
GNU General Public License v3.0
72 stars 22 forks source link

support alternative location for STEP configuration, effectively the specification of STEPPATH #83

Closed eengstrom closed 3 years ago

eengstrom commented 3 years ago

By default, STEP wants to look in $HOME/.step for configuration files, but they also allow the specification of the STEPPATH environment variable. It would be nice to be able to specify that in an Ansible variable and therefore be able to move the directory out of a user's home directory, into a more typical system directory - e.g. /etc/step

It looks like there are a number of locations where the assumption about /root/.step or $HOME/.step is hard coded, and perhaps a few other where it's implicit, though I don't think it will be hard to fully exercise the code and fix this.

I'm curious your thoughts before I proceed, however.

maxhoesel commented 3 years ago

I agree that we should support $STEPPATH as a configuration source where it makes sense. As far as I'm aware, the modules should already be doing this - we just call step-cli, which automatically reads configuration from $STEPPATH, then $HOME/.step. Any module that needs to read/write in $STEPPATH also accounts for both a set and unset variable.

As for the roles, the only hard-coded assumption I can find is in step_bootstrap_host: https://github.com/maxhoesel/ansible-collection-smallstep/blob/32035634eae76e7e5e482f5bacc0865f4c7cae5c/roles/step_bootstrap_host/tasks/main.yml#L23 That file should definitely respect $STEPPATH.

It's possible that I'm missing something, but since most of the actual step-related calls are handled by the modules, I don't think there's anything else that needs to be done. I could be wrong about this though - I have never tested with a non-standard $STEPPATH.

Updating the docs to mention $STEPPATH support would probably be a good idea too.

eengstrom commented 3 years ago

I'll likely work on this sometime in the next couple of weeks, depending on other needs. If you get to it before me, thanks.

maxhoesel commented 3 years ago

Partially addressed in https://github.com/maxhoesel/ansible-collection-smallstep/pull/87, but i'll reopen this issue in case anything else pops up

eengstrom commented 3 years ago

I tested your patch from #87, and unfortunately it would not work for me. I think it's related to the use of lookup(env, ..., which does not work to find remote environment variables. I have a fix in #89.

I'm not sure it's complete, since in particular I'm not sure this is the cleanest approach. Reiterating the commit/MR message:


This amends the checks and installation of STEP CA Root certificate to use STEPPATH if set. If not set, will mimic the step-cli behavior and default to $HOME/.step.

Note that the previous implementation utilized the ansible lookup(env, 'STEPPATH') filter, but that will not work for environment variables set via the environment: block, per the documentation:

https://docs.ansible.com/ansible/latest/user_guide/playbooks_environment.html

The environment: keyword does not affect Ansible itself, Ansible configuration settings, the environment for other users, or the execution of other plugins like lookups and filters.

What this does allow is setting of the environment variable at a play or task level; e.g.:

- hosts: step_client
  become: yes
  environment:
    STEPPATH: "{{ my_steppath }}"
  tasks:
   [...]

I thought about adding that as an example to the documentation, but then I also thought that perhaps a specific variable definition for STEPPATH (e.g. step_cli_steppath, mirroring step_ca_path) would be a better solution.

BTW: I have tested and am actively using the example above currently.

eengstrom commented 3 years ago

@maxhoesel - I appreciate that you took my patch in #89 so quickly, and it did seem to work in my limited testing, but I've now found a place where that won't work - namely, since the facts held in ansbile_env are gathered by any playbook, if you run mulitple playbooks, the once the facts are gathered (and perhaps cached, if you use that feature), the lookup in ansible_env will fail subsequent to the environment: block.

I really think a better solution is to use a real ansible variable, and then use that variable to set STEPPATH in any call to step-cli. That will take a bit more work, of course.

I'm not entirely sure yet how that will play with the value of step_ca_path, which is only used for the CA system. But if the CA host is also a client at the same time, then we need to keep those separate, I think.

Moreover, I think the renew service will fail still, since the environment variable is not being set there. Sadly, probably more places.

My initial suggestion would be to add a variable step_cli_steppath and use that in bootstrap and elsewhere. Thoughts?

maxhoesel commented 3 years ago

Good thing I didn't push a release then, heh.

So, just to make sure I'm understanding this correctly: Your suggestion is to add a step_cli_steppath variable to step_bootstrap_host so that users can reliably set STEPPATH to wherever they want to store their configuration in. Roles such as step_acme_cert then also need to accept this variable, as these roles rely on step-cli being initialized on the root user, so if we support a custom STEPPATH during bootstrapping, these roles also need to handle it.

I don't think step_cli or step_ca should be affected by this: step_cli just installs the CLI with no configuration being done and step_ca sets up a separate step environment for step_ca_user anyways. That configuration is separate from the one done to root by step_bootstrap_host.

To summarize: step_bootstrap_host and step_acme_cert need to have this role variable added and need to call step-cli with STEPPATH set to it. That seems reasonable enough to implement.

A doc section about how to deal with STEPPATH also seems like a good idea. Just some quick examples of how to use step_cli_steppath for roles and environment: for modules should do the trick.

eengstrom commented 3 years ago

Yes, your summary is correct. Sorry I was not very precise in my description.

maxhoesel commented 3 years ago

Alright, that sounds doable! I'll try to get a branch with this feature going in the next couple of days - I'll ping you when it's done so that you can run tests if needed