-
Notifications
You must be signed in to change notification settings - Fork 178
Add native support for Azure DevOps OIDC authentication #1027
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 native support for Azure DevOps OIDC authentication #1027
Conversation
| @functools.wraps(func) | ||
| def wrapper(cfg: "Config") -> Optional[CredentialsProvider]: | ||
| for attr in require: | ||
| getattr(cfg, attr) |
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.
Seemed like unnecessary Double check
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.
Are you sure? what does this function do?
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.
This function checks if the attribute exists in the config or not. This check is redundant, as it is done in the next line as well
|
|
||
|
|
||
| class DefaultCredentials: | ||
| """Select the first applicable credential provider from the chain""" |
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.
Strategy seems to be the correct word as we are trying different auth strategies until one suceeds
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 remember discussing this a while ago and the decision was that provider was better.
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 came after working in go SDK. It was named strategy there, therefore made the change here. But if this was already discussed then I will revert back to provider.
3e353da to
5bbb16b
Compare
| @functools.wraps(func) | ||
| def wrapper(cfg: "Config") -> Optional[CredentialsProvider]: | ||
| for attr in require: | ||
| getattr(cfg, attr) |
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.
Are you sure? what does this function do?
| Supported in Azure DevOps pipelines with OIDC service connections. | ||
| """ | ||
| try: | ||
| supplier = oidc_token_supplier.AzureDevOpsOIDCTokenSupplier() |
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.
Why is this needed? GitHubOIDCTokenSupplier does raise an exception, and therefore there is no try/catch. Is this different for a reason?
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.
Same. For early exit.
| audience = cfg.oidc_endpoints.token_endpoint | ||
|
|
||
| # Try to get an idToken. If no supplier returns a token, we cannot use this authentication mode. | ||
| id_token = supplier.get_oidc_token(audience) |
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.
This function is almost the same as github_oidc. Can and should we reuse the code?
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.
The old code should not be changed, right? The common part is within the GithubOIDC function. I can refactor the code to put the common parts outside it and use them in AzureDevOps. What is the better option?
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.
On a quick look, I think that the only difference if the supplier being used.
We may be able to have
#TODO: better name
def _internal_oidc(cfg: "Config", supplier: ) -> Optional[CredentialsProvider]:
...
@oauth_credentials_strategy("azure-devops-oidc", ["host", "client_id"])
def azure_devops_oidc(cfg: "Config") -> Optional[CredentialsProvider]:
return _internal_oidc(cfg, () -> oidc_token_supplier.AzureDevOpsOIDCTokenSupplier())
| return None | ||
|
|
||
| return response_json["value"] | ||
|
|
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.
Same: GitHubOIDC does not validate on create. Is there a reasons to change this?
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.
This is done to ensure Early exit. This is similar to what is done in Go SDK. If the environment variables are not set then we are sure that we are not in Azure DevOps Environment so we should exit at the earliest and try other providers.
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.
Could we add a TODO on the GitHub OIDC implementation to make it clear that it is the one with tech debt?
hectorcast-db
left a comment
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.
LGTM. Also get Parth approval.
| Supported in GitHub Actions with OIDC service connections. | ||
| """ | ||
| return _oidc_credentials_provider( | ||
| cfg=cfg, supplier_factory=lambda: oidc_token_supplier.GitHubOIDCTokenSupplier(), provider_name="GitHub OIDC" |
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.
This will tell the formatter to have one argument per line (like you do below).
| cfg=cfg, supplier_factory=lambda: oidc_token_supplier.GitHubOIDCTokenSupplier(), provider_name="GitHub OIDC" | |
| cfg=cfg, supplier_factory=lambda: oidc_token_supplier.GitHubOIDCTokenSupplier(), provider_name="GitHub OIDC", |
| return None | ||
|
|
||
| return response_json["value"] | ||
|
|
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.
Could we add a TODO on the GitHub OIDC implementation to make it clear that it is the one with tech debt?
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Added native support for Azure DevOps OIDC authentication to allow authentication from Azure DevOps pipeline's environment.
Testing
Tested on an Azure DevOps project. Used the SDK to authenticate with a Databrick's workspace using Azure DevOps OIDC
(System.AccessToken is slightly different, it needs to be exported through pipeline syntax: https://learn.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml#systemaccesstoken)