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 support for consuming secrets from 1Password reference links #333

Closed
wants to merge 1 commit into from

Conversation

d34dman
Copy link
Member

@d34dman d34dman commented Jul 25, 2024

@stmh am toying around the idea of using 1Password reference for populating secrets when 1password cli is available in the environment.

@d34dman d34dman requested a review from stmh July 25, 2024 12:12
@stmh
Copy link
Member

stmh commented Jul 25, 2024

hi @d34dman thanks for the MR.

Instead of passing the 1p reference I'd try to extract the vault id and item id from the url and use the already existing ways to get the secret. This will have the benefit that it will also work with 1p connect. So onePasswordReference is an alternative to onePasswordId and onePasswordVaultId

@d34dman
Copy link
Member Author

d34dman commented Jul 31, 2024

@stmh ,
Thanks for the review :)

Instead of passing the 1p reference I'd try to extract the vault id and item id from the url and use the already existing ways to get the secret. This will have the benefit that it will also work with 1p connect. So onePasswordReference is an alternative to onePasswordId and onePasswordVaultId

I tried to re-use as much as possible. But I must confess I don't have a good understanding of the underlying foundation. The reasons I didn't use the existing method is because I ran into the same limitation as the existing method. The limitation being, references allows you to target a specific field inside an item.

Example, op://Developer/PHAB.test_reference/text targets a "text" field; inside vault - Developer; item PHAB.test_reference.

Seems like the limitation is not coming from 1P cli, but from phabalicious. Which extracts a specific field from json and uses that as secret value. So this would either create a breaking change or force us to use references with all the limitation that current method brings and causes a lot of confusion for end users.

Reference: https://github.com/factorial-io/phabalicious/blob/main/src/Utilities/PasswordManager.php#L393-L444

Should i still proceed to refactor to use extractSecretFrom1PasswordPayload method, or did you had something else in mind?

@d34dman
Copy link
Member Author

d34dman commented Aug 1, 2024

@stmh maybe i can create another method to get item from vault using onePasswordId and onePasswordVaultId and then extract text and pass it back. This way, it doesn't really use op read.

This make me think, we should actually be adding another key like "onePasswordSubKey" to get what we want. If the subKey is not present, then it would as it does now.

@stmh
Copy link
Member

stmh commented Aug 2, 2024

I missed the fact that the link was targeting a different field. So what is missing is a way to target a specific field in an optional section. We can add them as properties to the secret data and if they are set change the current implementation that iterates over all sections and return items with purpose password or name password.

then we might also introduce the url notation for getting vault, item, name and section.

In the end, it needs to be compatible with 1password connect, as this is used in the ci.

@stmh
Copy link
Member

stmh commented Nov 5, 2024

@d34dman there is now a similar way part of phab to consume sth different than a password. See #338, closing this

@stmh stmh closed this Nov 5, 2024
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