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

azkv: Allow specifying auth method and add cachable authentication methods #1777

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

daytime-saxophone
Copy link

Azure is just really awful and slow to work with, and using sops with a azkv key to edit secrets from e.g. your laptop where you are authenticating with your user and not through a SP/MSI is nightmarishly slow.

To improve the situation this PR implements:

  • Allow explicitly specifying auth method through environement variable, this bypass the need to go through the default authentication chain.
  • Adds two cachable auth methods InteractiveBrowserCredential and DeviceCodeCredential which adds a very significant speedup compared to AzureCliCredential. See below.

The time to unlock a secret using sops v3.9.4 and authenticating as a user through azure-cli:

time sops -d ./path/to/secret
4.77s user 1.17s system 82% cpu 7.217 total

The time to unlock a secret using my PR and authenticating as a user with InteractiveBrowserCredential after the initial authentication:

> time SOPS_AZURE_AUTH_METHOD="BROWSER" sops -d ./path/to/secret
0.08s user 0.05s system 25% cpu 0.511 total

Related issues: #885, #1606

I'm happy to add documentation, tests or whatever is needed. I just don't want to go through the trouble before I know if the design is acceptable, and that there is a chance that this could be merged.

@daytime-saxophone daytime-saxophone marked this pull request as ready for review February 28, 2025 14:20
@felixfontein
Copy link
Contributor

Thanks for your contribution! I've been looking at this and thinking about this since yesterday, and I think the general idea is sound. (I'm not an Azure user though... I only use AZP for another open source project, and if you're writing that Azure is awful and slow to work with, I can guess what you mean since AZP definitely also falls into that category :D)

I'm a bit hesitant on the caching parts, since that writes credentials to disk (I guess not in an encrypted form, and even if its encrypted, it likely won't be anything safe). This should definitely be explicitly mentioned in the documentation, and maybe it would be better to rename the corresponding values for SOPS_AZURE_AUTH_METHOD to include "caching on disk" (I first just wanted to suggest to include that it's caching, but then I thought that later one might want to cache it somewhere else, like in the OS'es keyring, or some other agent, so including the method where it's cached is probably a good idea). WDYT?

I also have some smaller comments, I'm adding them here (even though they're not relevant to the general design) before I forget about them:

  • Please undo the changes in go.mod to go and toolchain, that's likely the reason that the builds fails in CI.
  • I'd probably move most of the new functions to a separate file, say azkv/credentials.go.
  • Is there a reason the environment variable values are upper-case? I think lower-case is more common. (They also used to be lower-case with the AZURE_AUTH_METHOD env variable that was removed in azkv: update SDK to latest, add tests, tidy #1067.)
  • The error message in the switch's default case should mention that it's an environment variable. Someone might have forgotten that the environment variable is set and be wondering where to find this SOPS_AZURE_AUTH_METHOD setting.
  • For the cached credential filenames, I'd use dashes instead of underscores.
  • Generally I'd move the strings used (env variable values, cache filenames) into constants.

CC @hiddeco, since you created #1067 which removed the AZURE_AUTH_METHOD environment variable.

@felixfontein
Copy link
Contributor

(And I think it would be better if Azure's SDK would take care of things like this in its default auth flow, or would offer another auth flow that allows the user to configure this via env variables. But if it doens't offer that, I guess we have to do it ourselves...)

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