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 OIDC authentication support #239

Closed
wants to merge 18 commits into from
Closed

add OIDC authentication support #239

wants to merge 18 commits into from

Conversation

adelinn
Copy link

@adelinn adelinn commented Jan 5, 2023

I added support for the following configuration parameters:
use_oidc - Whether to allow OpenID Connect to be used for authentication. Defaults to false.
oidc_token_file_path - The path to the file where the token from the OIDC provider is. This is useful for use with Kubernetes.
oidc_token - The token from the OIDC provider.
oidc_request_url - The URL for the OIDC provider from which to request an ID token.
oidc_request_token - The bearer token for the request to the OIDC provider.

The parameters work in the same way they do in the azurerm provider, the only difference would be that the order of the credentials that are tried is changed. The OidcCredential, which is the one added in this pull request, is checked first, then the default credential is checked, which actually checks the other credentials in the already documented order.

I didn't update the documentation files. I leave that to someone else.
Also the code changes that I made seem not to be in the correct location if I think about best practices, but it works...

This pull request would solve #101

PS: This is one of my first pull requests to a public project and I'm a beginner in go lang.

@adelinn
Copy link
Author

adelinn commented Jan 9, 2023

I made the code look better and I downgraded azidentity back to 1.2.0 which is the latest stable version.
I also removed oidc_authority_host as it didn't make sense to me to be there. If anyone wants to use that, then AZURE_AUTHORITY_HOST environment variable can still be used because it's internally implemented by azidentity library.

@adelinn
Copy link
Author

adelinn commented Jan 9, 2023

Although I run some local tests to make sure this works, someone would still need to run all the tests.

@ms-henglu
Copy link
Member

Hi @adelinn ,

Thank you so much for opening this PR and apologies for the delayed review.

My teammate was working on this feature and now it's supported the OIDC authentication in #240.

Since we're unable to accept this contribution, whilst I'd like to thank you for this PR I'm going to close this for the moment.

Thanks!

@ms-henglu ms-henglu closed this Jan 12, 2023
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