-
Notifications
You must be signed in to change notification settings - Fork 182
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
Fix SOPS azkv envCred #838
Fix SOPS azkv envCred #838
Conversation
At the moment, the envCred logic can't actually set the Azure credentials. This commit fixes the logic so that the environment variables can actually be used to set the Azure credentials. There are other issues that come up from this block of code, but those can be dealt with separately. Signed-off-by: Aaron Peschel <[email protected]>
64cde15
to
2b98fbf
Compare
I ran this locally and verified it fixes the issue. It does run into rate limiting with the Azure API, but that is a separate issue to solve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some investigation around the history of this code and found that it was added in #813 recently. The getDefaultAzureCredential()
function is based on upstream implementation where instead of creating a ChainedTokenCredential
, after iterating through all the possible auth methods, once auth material is found for any auth method, the creds are immediately returned.
The related issue #841 where you mention about the integration tests not working, it looks like that may be because of the MasterKey
being from the upstream sops project and not the locally defined version of MasterKey
, refer
encryptKey := &azkv.MasterKey{ |
MasterKey
reads the env var creds properly and the test passes.That can be fixed as part of this change or separately.
Update: Upon further investigation of the tests code, it looks like the test mentioned above it is a compatibility test and intentionally uses upstream SOPS to check if the local implementation is compatible with the upstream one. Which means we can add new test that reads env vars in order to test the broken auth.
For now, as it is, it LGTM!
I'd like to have another review+approval before merging this.
At the moment, the envCred logic can't actually set the Azure credentials.
This commit fixes the logic so that the environment variables can actually be used to set the Azure credentials.
There are other issues that come up from this block of code, but those can be dealt with separately.