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

DM-49929: Load OP_CONNECT_TOKEN and VAULT_TOKEN from 1Password via env file #4486

Merged
merged 2 commits into from
Apr 10, 2025

Conversation

jonathansick
Copy link
Member

This PR takes advantage of the 1Password CLI's ability to load secrets referenced by their path in a .env file. With this change, admins no longer need to manually load the VAULT_TOKEN and OP_CONNECT_TOKEN into their shell environments to run phalanx commands like phalanx secrets sync. An example usage is:

PHALANX_ENV="idfprod" op run --env-file="op/square.env" -- phalanx secrets audit

Note that I've had the user set PHALANX_ENV, which is used to specify secret paths in square.env specific to that Phalanx environment. I've also modified the Phalanx CLI to accept PHALANX_ENV as an environment variable instead of as a command-line argument.

Note an alternative approach could be to have a separate .env file for each Phalanx environment. This would let us set PHALANX_ENV in that file, and make the resulting CL invocations shorter, at the small cost fo maintaining more .env files. E.g.

op run --env-file="op/idfprod.env" -- phalanx secrets audit

What do you think? Perhaps this second approach would provide a better user experience.

@jonathansick jonathansick assigned rra and unassigned rra Apr 7, 2025
@jonathansick jonathansick requested a review from rra April 7, 2025 20:30
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

The 1Password stuff is great, thank you!

PHALANX_ENV for the command-line tool makes me nervous because no matter how much you tell people to use env to set the environment variable for only that command, my experience is that some number of people don't listen to you and do export PHALANX_ENV=idfprod so that they can stop typing it each time. This then becomes a land mine and two days later they run phalanx secrets sync --delete in the same window and accidentally do production (for example).

I therefore feel like forcing people to explicitly name the environment on the command line is a bit of a safety feature, and am not sure getting rid of it is a good idea. It's still true that people can make aliases that accomplish the same thing and are dangerous in the same way, but that requires a bit more intentional effort that hopefully would be linked with a bit more caution.

@jonathansick
Copy link
Member Author

Ok that sounds reasonable. So I'll drop PHALANX_ENV and then set up an env file for each environment.

These op/*.env file allows the OP_CONNECT_TOKEN and VAULT_TOKEN to
be referenced directly from 1Password rather than being manually
exported onto the admin's shell.
@jonathansick jonathansick requested a review from rra April 10, 2025 15:54
@jonathansick
Copy link
Member Author

@rra I've reworked the PR with individual env files for each Phalanx environment and also worked the method into our documentation (using tab sets) to show the op run methodology as an option.

Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you!

@jonathansick jonathansick added this pull request to the merge queue Apr 10, 2025
Merged via the queue into main with commit ae3a5f3 Apr 10, 2025
6 checks passed
@jonathansick jonathansick deleted the tickets/DM-49929 branch April 10, 2025 21:28
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.

2 participants