-
Notifications
You must be signed in to change notification settings - Fork 38
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 a credentials provider for Github Azure OIDC #950
Conversation
8d9f467
to
9c964d2
Compare
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.
Overall this seems good to me, great test cases as well! One small suggestion for logging when we can't successfully fetch the credential.
I think this is great for now. I'm picturing something in the future that there is a more generalized workflow of:
- Discover the current environment the SDK is in (github actions, azure devops, etc).
- Fetch the ID token according to the local environment (query the Github endpoint, somehow gather the ID token from Azure DevOps, etc.)
- Exchange the ID token for an OAuth token (using Azure EntraID endpoint, using Databricks OAuth API, etc.)
Essentially, we can probably generalize this to support OIDC strategies in the future by decoupling the ID token fetching from the token exchange endpoint. That said, let's not let perfect be the enemy of good, and we can make that change when we do work to support Azure DevOps, for example.
Thanks for the review @mgyucht! Please have a look at my answers and feel free to re-open the discussions.
Agreed, this is also how I picture the evolution of this package. How to effectively generalize the flow will become clearer as we add OIDC use cases. |
_Note: this PR is a copy of PR #950 which could not be merged because of some unverified commits. Please check PR #950 for the original review and comments._ ## Changes This PR adds a `CredentialsProvider` to authenticate with Azure from Github workflows. The code is inspired by a similar feature already implemented in the Python SDK. It works as follows: 1. Obtain an ID token from Azure leveraging the env variables `ACTIONS_ID_TOKEN_REQUEST_URL` and `ACTIONS_ID_TOKEN_REQUEST_TOKEN` as [explained here](https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-cloud-providers). 2. Exchange that ID token for an auth token. ## Tests Added a test suite which covers all the added code paths. I've also confirmed in my own Github Action that the code is properly able to authenticate. Note: I'm not super happy with how errors are compared (i.e. using a prefix) which is a little brittle. A better approach would be to leverage `errors.As` or `errors.Is`. However, it is difficult to do that at the moment without adding ad hoc new error types. A longer term solution would probably involve standardizing the package around a set of clearly defined error types shared by all implementations of `CredentialsProvider` in `config`. That is out of the scope of this PR though. - [x] `make test` passing - [x] `make fmt` applied - [x] relevant integration tests applied
Note: this PR could not be merged because of its first unverified commit, please see PR #965.
Changes
This PR adds a
CredentialsProvider
to authenticate with Azure from Github workflows.The code is inspired by a similar feature already implemented in the Python SDK. It works as follows:
ACTIONS_ID_TOKEN_REQUEST_URL
andACTIONS_ID_TOKEN_REQUEST_TOKEN
as explained here.Tests
Added a test suite which covers all the added code paths. I've also confirmed in my own Github Action that the code is properly able to authenticate.
Note: I'm not super happy with how errors are compared (i.e. using a prefix) which is a little brittle. A better approach would be to leverage
errors.As
orerrors.Is
. However, it is difficult to do that at the moment without adding ad hoc new error types. A longer term solution would probably involve standardizing the package around a set of clearly defined error types shared by all implementations ofCredentialsProvider
inconfig
. That is out of the scope of this PR though.make test
passingmake fmt
applied