feat: 1Password Desktop Application Integration#56
Conversation
|
Thanks @seiichi1101 ! I didn't have time to properly look at this, but looks sane. Could I check it's backwards compatible (I assume thus the oddness?) Is choosing the integration based in ENV vars used or other mechanism? |
Yes, please. I’ll also double-check backward compatibility on my side.
Yes - selection is via flags or environment variables. Example in
|
|
Thanks, might be worth opening a PR after this is merged. @manselmi might be worth you glancing over this if you have time, as you implemented first support of OP 😉 |
manselmi
left a comment
There was a problem hiding this comment.
@seiichi1101 Thank you for implementing this! When I saw that desktop app integration was introduced into the SDK, I was curious if someone would get around to adding support to keyring and aws-vault before I would. I'm glad you did, as I currently don't have bandwidth for further work on keyring or aws-vault.
I've requested one minor change regarding renaming the value of the OPDesktopEnvAccountName constant, otherwise looks good to me (assuming your own backwards-compatibility testing looks good).
opdesktop.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| OPDesktopEnvAccountName = "OP_DESKTOP_ACCOUNT_NAME" |
There was a problem hiding this comment.
According to https://github.com/1Password/onepassword-sdk-go/blob/v0.4.0-beta.2/client_builder.go#L89:
Takes as input the account name as specified in the desktop application, or the account UUID.
Perhaps the value of the constant should be changed to OP_DESKTOP_ACCOUNT_ID to imply that either account identifier (name or UUID) is acceptable. aws-vault would have the AWS_VAULT_OP_DESKTOP_ACCOUNT_ID env var and --op-desktop-account-id option. What do you think?
This rationale is why I opted for OP_VAULT_ID here.
There was a problem hiding this comment.
Sounds good.
I’ll follow your suggestion and rename it to OP_DESKTOP_ACCOUNT_ID.
| // fullClient stores the complete client to prevent premature garbage collection | ||
| // which would trigger the finalizer and release the client ID | ||
| fullClient *onepassword.Client |
There was a problem hiding this comment.
Thanks for this!
From what I can tell, in the 1Password Connect backend, we're already storing the complete client. So this finalizer issue shouldn't be present in that backend. Is that right?
(Adding the first two 1Password backends was some of my first ever Go code, and this sort of issue had never crossed my mind.)
There was a problem hiding this comment.
I believe so.
While this PR bumps the onepassword-sdk-go version, client creation in opconnect.go doesn’t use onepassword-sdk-go; it uses https://github.com/1Password/connect-sdk-go.
In addition, opconnect.go has little dependency on onepassword-sdk-go, so I don’t expect this change to affect the 1Password Connect backend.
There was a problem hiding this comment.
I appreciate the way you refactored opstandard.go across opstandard.go and opsrvaccount.go.
|
Thanks both! 🚀 |
|
hm, is beta still missing some bits? 🤔 Getting errors trying to build: https://github.com/ByteNess/aws-vault/actions/runs/20834085009/job/59855066619 |
|
Looks like there might be platform specific deps now so cross compilation is broken 🤔 |
|
@mbevc1 I'm not sure whether |
|
IIRC it was only enabled for Darwin upstream and was causing some cross-compile build issues otherwise, I'd need to recheck. |
|
FYI: I double-checked backward compatibility using a 1Password Service Account token on my side. I ran a couple of basic commands using (Sorry for the late reply — I needed approval from our internal IT team before I could use a 1Password Service Account.) |
|
No worries and thanks for checking, I'm still trying to get GHA pipelines to build using CGO with a mix of cross-compile libraries and native runners. I haven't merged this in to |
|
Okay, got it cross compiled across all platforms, but I needed to exclude FreeBSD |
This PR enables use of 1Password’s Desktop Application Integration introduced in
onepassword-sdk-go@v0.4.0-beta.2(release notes: https://github.com/1Password/onepassword-sdk-go/releases/tag/v0.4.0-beta.2).I’m opening this PR to enable
aws-vaultto leverage this integration.With my local environment,
aws-vaultis able to authenticate successfully via the 1Password desktop app. https://developer.1password.com/docs/sdks/desktop-app-integrations/Key changes
opstandard.gowas written with a service-account-only assumption. I kept the common logic there and moved the service-account-specific implementation intoopsrvaccount.go.opconnect.goto support the desktop app integration flow.opstandard.goimplementation only keeps anOPStandardClientAPI( ItemsAPI inonepassword-sdk-go) . As a result, the underlyingClientobject can be garbage-collected, which triggers its finalizer and causes the backend to invalidate that client ID at some point.Suggestions are welcome.