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

Support for OIDC Authentication #16555

Merged
merged 15 commits into from
May 19, 2022
Merged

Conversation

SudoSpartanDan
Copy link
Contributor

@SudoSpartanDan SudoSpartanDan commented Apr 26, 2022

Closes #16554

@manicminer Let me know if this looks good to you

Depends on hashicorp/go-azure-helpers#115

@manicminer
Copy link
Contributor

@SudoSpartanDan Thanks for looking at this! I actually have a working prototype on the oidc-auth branch but I will happily take a look through this and pick up any bits that I might have missed :D

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@SudoSpartanDan Thanks again for the work on this! This is looking great, I've made some suggestions below mostly around naming. If you can take a look at these, this should be good to merge once the corresponding changes to the Azure remote state backend are also readied for release.

internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
website/docs/guides/service_principal_oidc.html.markdown Outdated Show resolved Hide resolved
website/docs/index.html.markdown Outdated Show resolved Hide resolved
website/docs/index.html.markdown Outdated Show resolved Hide resolved
website/docs/guides/service_principal_oidc.html.markdown Outdated Show resolved Hide resolved
@SudoSpartanDan
Copy link
Contributor Author

@SudoSpartanDan Thanks again for the work on this! This is looking great, I've made some suggestions below mostly around naming. If you can take a look at these, this should be good to merge once the corresponding changes to the Azure remote state backend are also readied for release.

Sounds good! I went ahead and committed your suggestions, I'll keep an eye out for this PR until the azurerm backend changes are ready to go as well.

@manicminer
Copy link
Contributor

@SudoSpartanDan Thanks for updating. You can go ahead and vendor v0.31.1 of go-azure-helpers now, this completes the OIDC support. Thanks!

@SudoSpartanDan
Copy link
Contributor Author

@manicminer updated!

@sigurdfalk
Copy link

@SudoSpartanDan @manicminer I have tracked this issue some time and are very pleased to see you have come up with a solution 👑 🚀 Just wondering if we are still waiting for something since this was added to the "Future" milestone, or if you have some ballpark idea of when we can expect to see this feature released?

@manicminer
Copy link
Contributor

This will be merged when Terraform v1.2 is released, as it requires updates to the Azure remote state backend to support OIDC.

@rhyspaterson
Copy link

@SudoSpartanDan

This comment was marked as off-topic.

@tombuildsstuff tombuildsstuff removed this from the Future milestone May 19, 2022
@tombuildsstuff tombuildsstuff added this to the v3.7.0 milestone May 19, 2022
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @SudoSpartanDan

Thanks for this PR - as has been mentioned above, Terraform Core 1.2 has now been released as such we can look at shipping this in the Provider now 👍

I've taken a look through and left a handful of documentation related comments inline, but if we can fix those up then this otherwise LGTM 👍

Thanks!

website/docs/index.html.markdown Show resolved Hide resolved
website/docs/guides/service_principal_oidc.html.markdown Outdated Show resolved Hide resolved
website/docs/guides/azure_cli.html.markdown Show resolved Hide resolved
@SudoSpartanDan
Copy link
Contributor Author

@tombuildsstuff Thanks, committed and ready to go! 🚀

@tombuildsstuff tombuildsstuff dismissed manicminer’s stale review May 19, 2022 13:50

dismissing since changes have been pushed

@tombuildsstuff tombuildsstuff merged commit 21a0e3c into hashicorp:main May 19, 2022
tombuildsstuff added a commit that referenced this pull request May 19, 2022
@stevengonsalvez
Copy link
Contributor

When will this appear in a a release ? or is there a clean way to point to "main" (latest) for the provider ?

@github-actions
Copy link

This functionality has been released in v3.7.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@ekristen
Copy link

Since this was recently merged, I'll ask here, this appears to only implement a specific GitHub Token OIDC implementation not an actual generic OIDC or Kubernetes Federated OIDC that Azure Service Principals support? Can the someone confirm or deny this? If its suppose to be OIDC do we have any working examples that are not GitHub? If not perhaps this needs to be renamed to GitHub Token OIDC Support?

@SudoSpartanDan
Copy link
Contributor Author

Since this was recently merged, I'll ask here, this appears to only implement a specific GitHub Token OIDC implementation not an actual generic OIDC or Kubernetes Federated OIDC that Azure Service Principals support? Can the someone confirm or deny this? If its suppose to be OIDC do we have any working examples that are not GitHub? If not perhaps this needs to be renamed to GitHub Token OIDC Support?

By design, yes, it uses the GitHub environment variables for this by default, but should be able to support OIDC with other trusted entities. Two variables in the provider configuration are used for this: oidc_request_token and oidc_request_token. Right now, the only use case we have to document is GitHub Actions, but if you have another use case that this can be used by, feel free to drop a PR adding that example in :)

@ekristen
Copy link

ekristen commented May 23, 2022

@SudoSpartanDan I ended up opening up two issues because unfortunately the "but should be able to support OIDC with other trusted entities" isn't correct as far as I can tell, the GitHub implementation only works with GitHub.

I won't re-hash it here, but I opened #16900 and #16901

As for use cases, I'd like to use actual OIDC instead of GitHub Tokens. Specifically Kubernetes Federation.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for OIDC Authentication
7 participants