docs icon indicating copy to clipboard operation
docs copied to clipboard

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

Open mungojam opened this issue 3 years ago • 12 comments

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 Plan

See this comment for additional details when working on this issue.

mungojam avatar Feb 10 '22 11:02 mungojam

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.

welcome[bot] avatar Feb 10 '22 11:02 welcome[bot]

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

ramyaparimi avatar Feb 10 '22 13:02 ramyaparimi

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 avatar Feb 10 '22 22:02 mungojam

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

mungojam avatar Feb 10 '22 22:02 mungojam

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.

ianling avatar Feb 17 '22 17:02 ianling

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.

mungojam avatar Feb 17 '22 19:02 mungojam

👋 Checking with engineering team

martin389 avatar Apr 29 '22 05:04 martin389

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!

N-Usha avatar Apr 29 '22 07:04 N-Usha

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.

ianling avatar Apr 29 '22 17:04 ianling

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).

mungojam avatar Apr 29 '22 20:04 mungojam

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

cmwilson21 avatar Sep 30 '22 21:09 cmwilson21

👋 @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?)

mungojam avatar Sep 30 '22 21:09 mungojam

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

mungojam avatar Feb 26 '23 18:02 mungojam

@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 avatar Feb 28 '23 16:02 cmwilson21

👋🏻 @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.

felicitymay avatar Sep 19 '23 16:09 felicitymay

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

mungojam avatar Sep 19 '23 19:09 mungojam

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 avatar Sep 20 '23 13:09 felicitymay

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 avatar Sep 21 '23 10:09 felicitymay

👋 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.

jc-clark avatar Oct 10 '23 20:10 jc-clark

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

mungojam avatar Oct 10 '23 20:10 mungojam