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

feat: add STEPPATH support to bootstrap_host and acme_cert #91

Merged
merged 2 commits into from
Jul 20, 2021
Merged

Conversation

maxhoesel
Copy link
Collaborator

This PR adds support for a custom STEPPATH in all roles that use the step-cli config.

Fixes #83

@eengstrom can you confirm that this works for you?

This PR adds support for a custom STEPPATH in all roles that
use the step-cli config
Copy link
Contributor

@eengstrom eengstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using the changes merged on top of main as of this morning. All is working. A few comments regarding the default values, but those can be addresses separately, I think.

Thanks for all your work.

##### `step_cli_steppath`
- Optionally set a custom `$STEPPATH` from which to read the step config
- Example: `/etc/step-cli`
- Default: `/root/.step/`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, the default is $HOME/.step, which depends on the user running. While most people are likely to use become: true and therefore run as root, I would still use $HOME/.step as the default in documentation.

All step configuration will be saved in this path instead of the default `$HOME/.step/`
- **NOTE**: If set, you will have to supply your custom `$STEPPATH` in all future role/module/`step-cli` calls on this host that use the step config
- Example: `/etc/step-cli`
- Default: `/root/.step/`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, the default is $HOME/.step, which depends on the user running. While most people are likely to use become: true and therefore run as root, I would still use $HOME/.step as the default in documentation.

@@ -1,5 +1,6 @@
---
step_cli_executable: step-cli
step_cli_steppath: /root/.step
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to the documentation feedback, I feel this should default to $HOME/.step. The problem is that to actively set a reasonable default, we'd have to return to detecting the right value of $HOME in this file.

Could use ansible_env.HOME, but that only works if facts have been gathered. Of course, using omit filter won't work either, since that's a hack for excluding module arguments only.

Alternatively, we could leave the value unset in the defaults, and then use slightly convoluted logic in the environment: block to set the value of STEPPATH to the empty string, but only if step-cli is smart enough to look for the empty string and use their built-in default.

The last option would be to use the shell module in place of the command module and explicitly set the command line to something like STEPPATH="path" step-cli ..., but only if the value of step_cli_steppath is defined. That's gross, but might work.

Now I'm out of ideas - curious if you have any others, or think this issue isn't worth worrying about.

@@ -6,3 +6,5 @@ step_cli_executable: step-cli

step_bootstrap_install_cert: yes
step_bootstrap_force: no

step_cli_steppath: /root/.step
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other comment on the same variable in the step_acme_cert role.

@maxhoesel
Copy link
Collaborator Author

Glad to hear that the changes are working for you! As for the defaults, I agree that using $HOME/.step would be cleaner and more compatible, but actually doing so is, well ...

As you mentioned, using ansible_env (or the env lookup) only works if facts have been gathered. It also doesn't work with become: yes (it always returns the connecting users home dir, not the one that is actually executing).

An alternative would be to dynamically set step_cli_steppath in each role using a task. Something like this sould work, but it's pretty ugly - and we'd need this block in every role utilizing $STEPPATH:

- name: Get user home dir
  shell: 'echo $HOME'
  register: user_homedir
  when: step_cli_steppath is undefined
- name: Set step_cli_steppath
  set_fact:
    step_cli_steppath: "{{ user_homedir.stdout }}/.step"
  when: step_cli_steppath is undefined

As for just leaving STEPPATH undefined when it's not supplied, that doesn't work for step_bootstrap_host: https://github.com/maxhoesel/ansible-collection-smallstep/blob/8be99fb8f16b70ec099dd602ec845f9fb65cc172/roles/step_bootstrap_host/tasks/main.yml#L23

In order to verify the certificate, the role needs to access $STEPPATH from within Ansible, so it needs to be set to an actual value. If we leave it undefined we'd still have to get the correct path for this command.

Unfortunately, I don't see an easy way out of the hardcoded /root/.step default right now. I'd also make the argument that it's not a big deal since all the roles in this collection are intended to be run as root anyways, Granted, it would still break if someone used a different root home dir for some reason, but even then, they can just supply step_cli_path manually.

My opinion on this is to merge as-is and then open an issue about the hard-coded root dir for documentation purposes.

@eengstrom
Copy link
Contributor

My opinion on this is to merge as-is and then open an issue about the hard-coded root dir for documentation purposes.

I'm good with that.

As you mentioned, using ansible_env (or the env lookup) only works if facts have been gathered. It also doesn't work with become: yes (it always returns the connecting users home dir, not the one that is actually executing).

Interesting - we use ansible_env and it always returns the right value, even though we don't connect as root - we use sudo as the become_method.

As for using the lookup(env,... - yeah - that doesn't work - it uses the control-node's environment, not the target system. So, fails in a different way.

Regardless, I'm good with the merge - and thanks for your work.

@maxhoesel
Copy link
Collaborator Author

Interesting - we use ansible_env and it always returns the right value, even though we don't connect as root - we use sudo as the become_method

I think it might depend on whether become is called on a play or task basis, but I'm not sure. I just remember this biting me a few years ago.

And thanks for the all the feedback! Going to merge this and another PR i have been working on, then I'll push out a release in a bit.

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

Successfully merging this pull request may close these issues.

support alternative location for STEP configuration, effectively the specification of STEPPATH
2 participants