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

Add support for azure workload identity authentication #155

Merged

Conversation

cldmstr
Copy link
Contributor

@cldmstr cldmstr commented Mar 17, 2023

Use the Azure Workload Identity federated identity when it is available to authenticate against Azure keyvault. This allows vault to run in an AKS cluster with service accounts federated to service principals or managed identities.

The PR uses the default credential reader, which covers all the Azure login credentials: MSI, Azure CLI or Federated Identity.

This change also updates the Azure client libraries to use the azure-sdk-for-go implementation as the autorest libraries are being depricated at the end of March 2023.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 17, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

Ty for the PR. Just one suggestion for now. I've reached out to others for further testing; so I'm hopeful someone might be able to provide feedback sooner than I would (if I have to get an env setup to test it).

wrappers/azurekeyvault/azurekeyvault.go Outdated Show resolved Hide resolved
@AntPAllen
Copy link

Happy to test this if it helps. Came to this repo looking to see if it was possible to auth boundary using workload identity.

@ibauersachs
Copy link

@jimlambrt Any update on this? Can we help somehow to move this forward?

@jimlambrt
Copy link
Collaborator

Ty for the reminder. Now that Boundary v0.13.0, we should be able to find a resource to test this. I'll be back with an update on when that might happen.

@ibauersachs
Copy link

@jimlambrt Do you have an update on that resource or timeline? Is there anything we can help with?

@jimlambrt
Copy link
Collaborator

Good afternoon! Yes, we've started to make some progress allocating a resource and building some sort of automated tests. Updates in the next week or so 🤞

Comment on lines -313 to +303
config := auth.NewMSIConfig()
config.Resource = strings.TrimSuffix(v.resource, "/")
if v.clientID != "" {
config.ClientID = v.clientID
}
authorizer, err = config.Authorizer()
cred, err = azidentity.NewDefaultAzureCredential(nil)
Copy link

Choose a reason for hiding this comment

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

Hi @cldmstr - can I suggest you check that this doesn't introduce a regression for this fix? #97
There were no tests added for it so it's quite possible it would.

It's possible for multiple managed service identities to exist, and in that situation when autorest/adal queried IMDS it would receive an error back if the client ID was not provided. I'm assuming that azure-sdk-for-go behaves similarly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @jtv8 . I can confirm that the behavior in azure-sdk-for-go is the same as the fix and does not introduce a regression. Internally, the call does almost exactly what the fix does.

Use the Azure Workload Identity federated identity magic when it is available to authenticate against
Azure keyvault. This allows vault to run in an AKS cluster with service accounts federated to
service principals or managed identites.

This also requires the azure libraries to be migrated to the newest version as the autorest libraries are
being depricated.
@jimlambrt
Copy link
Collaborator

We've been able to successfully validate/test this change! @cldmstr can you resolve the conflicts so we can get it merged?

@cldmstr cldmstr force-pushed the support-azure-workload-identity-authentication branch from 60a01ca to 07c6997 Compare September 6, 2023 14:06
@cldmstr cldmstr requested review from jefferai and a team as code owners September 6, 2023 14:06
Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

ty for your patience.

@jimlambrt jimlambrt merged commit f9c2a2f into hashicorp:main Sep 6, 2023
28 of 29 checks passed
@cldmstr
Copy link
Contributor Author

cldmstr commented Sep 6, 2023

Thanks a lot. There was one relevant change to the method I was working on that cropped up during the rebase. I did my best to add the fix to
Add Work around an azure HTTP/2 bug in the new client. However, I couldn't test it quite yet myself.

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.

None yet

6 participants