-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: Add support for creating IAM GitHub OIDC provider and role(s) #308
feat: Add support for creating IAM GitHub OIDC provider and role(s) #308
Conversation
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.
Minor comment for GitHub Enterprise users, LGTM.
modules/iam-github-oidc-role/main.tf
Outdated
data "aws_caller_identity" "current" {} | ||
|
||
locals { | ||
github_token_url = "token.actions.githubusercontent.com" |
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 think this ok for the main github.com, but it will have to be parametrized when using with GitHub Enterprise, as described here (search for mygithub.com/_services/token
).
Let's make this as a variable with this value as the default.
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.
good callout, however - I don't know if that post is correct. For example, it does not appear to reference the AWS STS service (secure token service) anywhere so I'm a little confused. I've reached out internally to see if I can get in contact with someone at GitHub for some guidance. I'll update once I know a bit more
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.
👍 Maybe the post I found was not the most accurate one.
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.
eh, I don't know if I know enough to weigh in on whether its correct or not 😅 - since its security related, I figured its better to error on the side of caution and see if I can find someone who would be able to weigh in on the matter
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.
Currently, you've made it as a variable (with the correct default value), so it should be good enough for merge.
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.
well, that post doesn't have some things like this https://github.com/terraform-aws-modules/terraform-aws-iam/pull/308/files#diff-490344238465f9b048f0ff98d48bfe068162248472dba85614109d31edf6f269R33-R37
which made me realize they don't have any reference to the secure token service which I believe is a hard requirement for these two entities to create a trust relationship (between GitHub and AWS)
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.
Right. Hopefully, someone with GHE experience can verify and confirm it.
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.
learned something new - at the top of the GitHub docs, theres a drop down on the right header to switch the docs between different flavors of GitHub:
- Standard docs for SaaS GitHub: https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-amazon-web-services
- Enterprise docs: https://docs.github.com/en/enterprise-server@3.7/actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-amazon-web-services
Per those docs, it looks like the sub and aud URLs are the only thing changing, and potential the aud value whether you are using the official github action or not so I have updated the PR to suit those conditions
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.
Looks great 👍
## [5.6.0](v5.5.7...v5.6.0) (2022-11-19) ### Features * Add support for creating IAM GitHub OIDC provider and role(s) ([#308](#308)) ([cc44693](cc44693))
This PR is included in version 5.6.0 🎉 |
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 issues. 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. |
Description
Motivation and Context
Breaking Changes
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull request