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

Clarify if all OIDC conditions are supported on AWS (they don't appear to be) #15324

Closed
1 task done
mungojam opened this issue Feb 10, 2022 · 20 comments
Closed
1 task done
Labels
content This issue or pull request belongs to the Docs Content team help wanted Anyone is welcome to open a pull request to fix this issue pumpkin-spice Specifically tracked Hacktoberfest issue - internal purposes

Comments

@mungojam
Copy link
Contributor

mungojam commented Feb 10, 2022

Code of Conduct

What article on docs.github.com is affected?

https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-amazon-web-services

What part(s) of the article would you like to see updated?

From my experience and others in this ticket, it appears that AWS don't currently support checking the various assertions that are passed through by GitHub e.g. repository_owner in my case. Their docs suggest otherwise, so it is probably a bug in AWS.

It might be worth highlighting in the docs, unless you have been able to get it to work yourselves.

Filtering on sub works fine.

Additional information

No response


[maintainer edit]

Content changes to fix this issue

  1. Update Understanding the OIDC token in "About security hardening with OpenID Connect."
    • Note that OIDC tokens generated by GitHub include a range of custom claims and that you should verify which claims are supported by your cloud provider before configuring OIDC trust.
  2. Update Prerequisites in "Configuring OpenID Connect in AWS."
    • Note that OIDC tokens generated by GitHub include a range of custom claims and that you should verify which claims are supported by your cloud provider before configuring OIDC trust.
@mungojam mungojam added the content This issue or pull request belongs to the Docs Content team label Feb 10, 2022
@welcome
Copy link

welcome bot commented Feb 10, 2022

Thanks for opening this issue. A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines.

@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Feb 10, 2022
@github-actions github-actions bot added this to Triage in Docs open source board Feb 10, 2022
@ramyaparimi ramyaparimi added waiting for review Issue/PR is waiting for a writer's review and removed triage Do not begin working on this issue until triaged by the team labels Feb 10, 2022
@ramyaparimi
Copy link
Contributor

@mungojam Thanks so much for opening an issue! I'll triage this for the team to take a look 👀

@mungojam
Copy link
Contributor Author

There is a little more evidence that custom claims aren't supported through AssumeRoleWithWebIdentity here:

https://gist.github.com/gene1wood/54611c45a3298504f8e33d95e317ae5c?permalink_comment_id=2973957#gistcomment-2973957

At least they weren't in 2019

@mungojam
Copy link
Contributor Author

And here are the official docs on it that only list a limited number of fields:

https://docs.aws.amazon.com/en_en/IAM/latest/UserGuide/reference_policies_iam-condition-keys.html#condition-keys-wif

@ianling
Copy link

ianling commented Feb 17, 2022

In the example given in the Github Actions docs:

"Condition": {
  "ForAllValues:StringEquals": {
    "token.actions.githubusercontent.com:aud": "sts.amazonaws.com",
    "token.actions.githubusercontent.com:sub": "repo:octo-org/octo-repo:ref:refs/heads/octo-branch"
  }
}

Notice that "StringEquals" has changed to "ForAllValues:StringEquals. This fixed it for me. I can use the custom claims now.

This should probably be clarified or called out specifically in this documentation, because it's not super obvious.

@mungojam
Copy link
Contributor Author

aud and sub are both listed as supported in the AWS docs, so that explains why it works. Unfortunately none of the custom claims like actor or repository_owner are supported. For repository_owner we can use a StringLike with sub to achieve the same effect, but not for actor which doesn't appear in any of the standard claims.

@github-actions github-actions bot added stale There is no recent activity on this issue or pull request and removed stale There is no recent activity on this issue or pull request labels Apr 19, 2022
@martin389
Copy link
Contributor

👋 Checking with engineering team

@N-Usha
Copy link
Contributor

N-Usha commented Apr 29, 2022

Thanks @mungojam for raising this issue. As you have rightly figured out above, AWS currently supports only aud and sub along with a few others but dont have support for all the custom claims that are part of GitHub OIDC token.

Can you please provide more details around your specific requirement to set condition on actor or repository_owner ? Would this validation be needed across all workflows in your organization or just a subset of workflows?

We are currently evaluating to provide a programatic way to customize what custom claims can be made part of the sub claim and any further details in to your use case will help us build more context. Thanks!

@ianling
Copy link

ianling commented Apr 29, 2022

At least for me, I want to be able to allow Github Actions workflows that are run in my organization to assume roles in my AWS account using the OIDC connector. The simplest way for me to allow only repos in my organization to do that is to set a condition on the repository_owner claim.

@mungojam
Copy link
Contributor Author

Thanks @mungojam for raising this issue. As you have rightly figured out above, AWS currently supports only aud and sub along with a few others but dont have support for all the custom claims that are part of GitHub OIDC token.

Can you please provide more details around your specific requirement to set condition on actor or repository_owner ? Would this validation be needed across all workflows in your organization or just a subset of workflows?

We are currently evaluating to provide a programatic way to customize what custom claims can be made part of the sub claim and any further details in to your use case will help us build more context. Thanks!

I wanted to be able to have some backstop that prevents any Github Actions access to our AWS accounts from repositories outside our GitHub org. We would expect all teams to have additional protections to limit access from only their repositories and may also enforce that, but the ultimate backstop would still be wanted. As it is, I have been able to use StringLike to achieve this instead so the impact on me is not big.

I raised this particular issue as I think the GitHub docs should be explicit about the AWS limitations so that people don't waste time or accidentally make dangerous security rules like using ForAllValues:StringEquals (@ianling, this is doing the opposite of what you think and potentially opening up your accounts to all repos).

@github-actions github-actions bot added stale There is no recent activity on this issue or pull request and removed stale There is no recent activity on this issue or pull request labels Jun 29, 2022
@github-actions github-actions bot added stale There is no recent activity on this issue or pull request and removed stale There is no recent activity on this issue or pull request labels Aug 30, 2022
@cmwilson21
Copy link
Contributor

👋 @mungojam - Just checking in on this one. Are you able to make a PR based on your latest comment above?

@mungojam
Copy link
Contributor Author

👋 @mungojam - Just checking in on this one. Are you able to make a PR based on your latest comment above?

Sorry, I only really have time for doing small documentation tweaks in PRs, or code changes. My skills at writing clear English docs aren't great so it takes me ages (despite being English!).

Though on this topic, I note the new additions that allow for the org. name to be included in the audience and also the ability to set custom claims in the sub field. Both are steps in the right direction and I hope more customisability will come and also better documentation for them (for example what will the format of the audience url be if the org. is added?)

@cmwilson21 cmwilson21 added help wanted Anyone is welcome to open a pull request to fix this issue pumpkin-spice Specifically tracked Hacktoberfest issue - internal purposes labels Oct 18, 2022
@docubot docubot moved this from Triage to Help wanted in Docs open source board Oct 18, 2022
@github-actions github-actions bot added stale There is no recent activity on this issue or pull request and removed stale There is no recent activity on this issue or pull request labels Dec 18, 2022
@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Feb 18, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 26, 2023
Docs open source board automation moved this from Help wanted to Done Feb 26, 2023
@mungojam
Copy link
Contributor Author

That's a shame, closing with no explanation when it is clear that plenty of people are confused and potentially use dangerous settings because of the lack of clarity

@cmwilson21
Copy link
Contributor

@mungojam It looks like stalebot closed this in error, so I'm reopening it. It's currently on the help wanted board for anyone to pick up and make this contribution 💖

@cmwilson21 cmwilson21 reopened this Feb 28, 2023
@cmwilson21 cmwilson21 removed stale There is no recent activity on this issue or pull request waiting for review Issue/PR is waiting for a writer's review labels Feb 28, 2023
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Feb 28, 2023
@cmwilson21 cmwilson21 moved this from Done to Help wanted in Docs open source board Feb 28, 2023
@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Aug 10, 2023
@cmwilson21 cmwilson21 removed triage Do not begin working on this issue until triaged by the team stale There is no recent activity on this issue or pull request labels Aug 11, 2023
@felicitymay
Copy link
Contributor

felicitymay commented Sep 19, 2023

👋🏻 @mungojam

We're reviewing all open help wanted issues as part of our preparation for Hacktoberfest, which is coming up shortly.

I've looked at the changes to the article since this issue was originally opened and can see that the following change has been made:

I think that the only additional change we might want to make for this issue is to expand the sentence: "The token also includes custom claims provided by GitHub." to explain that some cloud providers do not recognize GitHub's custom claims and that you should check the provider's own documentation before configuring your connection.

@mungojam
Copy link
Contributor Author

I think that the only additional change we might want to make for this issue is to expand the sentence: "The token also includes custom claims provided by GitHub." to explain that some cloud providers do not recognize GitHub's custom claims and that you should check the provider's own documentation before configuring your connection.

@felicitymay Thanks for looking into this. A few thoughts on what could still be improved. The page you link has a pre-requisite section at the top. I'm pretty methodical so thought I should read all that it said:

image

Specifically it says Before proceeding, you must plan your security strategy to ensure that access tokens are only allocated in a predictable way. and then links to "About security hardening with OpenID Connect." for more information.

It is that page which doesn't seem to clearly state that not all token attributes will be supported by all cloud providers. So somebody could follow the advice of the pre-req and plan their enforcement based on that page, assuming that it will be possible to achieve it (I think that was the cause for me).

Similarly on the AWS page, there is a clear note recommending the use of sub but nothing to suggest that other claims may not be supported

image

@felicitymay felicitymay self-assigned this Sep 20, 2023
@felicitymay
Copy link
Contributor

Thank you for getting back to us so quickly with such clear feedback 💖

I'll update the issue summary based on your feedback and this will be ready for someone to work on. We usually get a lot of contributions during Hacktoberfest.

@felicitymay
Copy link
Contributor

Based on your feedback, I've updated the issue summary to suggest a location in each file where we should note that the claims included in a GitHub token may not be supported by your cloud provider and that you should verify which claims are supported.

@felicitymay felicitymay removed their assignment Sep 21, 2023
@jc-clark
Copy link
Contributor

👋 Hey @mungojam. Thanks for the suggestion here and for your patience while we worked through this. I've been communicating with some folks internally on this issue and we agree that the docs could use a couple changes to specify the OIDC conditions for AWS.

Because these files don't accept community contributions, I'll close this and open an internal issue to fix this.

Docs open source board automation moved this from Help wanted to Done Oct 10, 2023
@mungojam
Copy link
Contributor Author

That's cool thanks.

One side note is that here and on the GitHub roadmap I've noticed githubbers not making use of the close as dupe/not-planned GitHub feature. It really helps to communicate when people are searching for closed issues or looking at notifications

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content This issue or pull request belongs to the Docs Content team help wanted Anyone is welcome to open a pull request to fix this issue pumpkin-spice Specifically tracked Hacktoberfest issue - internal purposes
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

9 participants