google-github-actions / auth

A GitHub Action for authenticating to Google Cloud.
https://cloud.google.com/iam
Apache License 2.0
969 stars 196 forks source link

Clarify use of GitHub owner|repository IDs #444

Closed DazWilkin closed 2 months ago

DazWilkin commented 2 months ago

The documentation explains how to update the Attribute Condition to reflect owner|repository IDs but leaves it to the developer to infer that no changes need be made to the Attribute Mapping.

Updates the documentation to be explicit that the mapping is not changed by the use of IDs as conditions.

verbanicm commented 2 months ago

@DazWilkin I am not sure this is accurate The Attribute Mapping must continue to includeattribute.repository=assertion.repository,attribute.repository_owner=assertion.repository_ownerwhen using numeric organization and repository ID values.

Frequently my team uses the _id properties without the "name" fields. I am closing this unless there is something I am missing.

DazWilkin commented 2 months ago

I think perhaps my explanation was poorly worded but I agree with you.

My point is that the Attribute Mapping doesn't change and must retain attribute.repository=assertion.repository,attribute.repository_owner=assertion.repository_owner even if the Attribution Condition is changed to reflect the IDs.

i.e.:

--attribute-mapping="...,attribute.repository=assertion.repository,attribute.repository_owner=assertion.repository_owner" \
--attribute-condition="assertion.repository_owner_id==\"${OWNER_ID}\" && assertion.repository_id==\"${REPO_ID}\"" \
sethvargo commented 2 months ago

Like @verbanicm said, that's not true. You only need to include things in attribute mappings that you want to use in more granular permissions (i.e. principalSet://).

DazWilkin commented 2 months ago

Let's try this a different way :-(

The documentation as written, proposes the more secure path of using owner and repo IDs.

However, it does not explain how using owner and repo IDs is incorporated into the --attribute-mapping (no change) and --attribute-condition flags of gcloud iam workload-identity-pools providers create-oidc.

I was attempting (and failed) to suggest a way to add this clarity to the document.

sethvargo commented 2 months ago

However, it does not explain how using owner and repo IDs is incorporated into the --attribute-mapping (no change) and --attribute-condition flags of gcloud iam workload-identity-pools providers create-oidc.

Sorry, I still don't understand. Can you explain what problem you encountered?

DazWilkin commented 2 months ago

This was my first time running through Workload Identity Federation with GitHub Actions and to help someone who was struggling.

The instructions are comprehensive but non-trivial.

I followed the instructions and got it working.

The instructions include step #4 (no HTML ID tag to reference):

gcloud iam workload-identity-pools providers create-oidc "my-repo" \
  --project="${PROJECT_ID}" \
  --location="global" \
  --workload-identity-pool="github" \
  --display-name="My GitHub repo Provider" \
  --attribute-mapping="google.subject=assertion.sub,attribute.actor=assertion.actor,attribute.repository=assertion.repository,attribute.repository_owner=assertion.repository_owner" \
  --attribute-condition="assertion.repository_owner == '${GITHUB_ORG}'" \
  --issuer-uri="https://token.actions.githubusercontent.com"

NOTE The Attribute Mapping includes repository_owner=assertion.repository_owner and the Attribute Condition is assertion.repository_owner == '${GITHUB_ORG}'. A naive reader (such as I) assumed (incorrectly) that assertion fields used in the condition must be present in the mapping.

I subsequently read the security considerations to use the immutable IDs which is understandable but only provides the --attribute-condition value:

assertion.repository_owner_id == '1342004' && assertion.repository_id == '260064828'

So, initially, I thought, if I'm using repository_owner_id and repository_id in the condition, these probably need to be present in the mapping too. They don't and this was a rational (!?) red-herring.

I think it would be helpful for those of us who aren't familiar with this process and don't fully understand what's being done by this command but just trying to get WIF configured, that it would be helpful to add a clarification that the Attribute Mapping (!) does not need to be changed (from the above example command). Even though repository_owner_id and repository_id are being added to the Attribute Condition.