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

Trusted publishing: support for Google Cloud service accounts #13551

Closed
7 of 8 tasks
woodruffw opened this issue May 1, 2023 · 24 comments
Closed
7 of 8 tasks

Trusted publishing: support for Google Cloud service accounts #13551

woodruffw opened this issue May 1, 2023 · 24 comments
Assignees

Comments

@woodruffw
Copy link
Member

woodruffw commented May 1, 2023

Support for these would allow PyPI users with Google Cloud-based publishing workflows to benefit from trusted publishing.

An example claim set from a Google Cloud service account, lightly anonymized:

{
  "aud": "{SOME-AUDIENCE}",
  "azp": "{SOME-AZP}",
  "email": "{PROJECT-NUMBER}[email protected]",
  "email_verified": true,
  "exp": 1682967007,
  "google": {
    "compute_engine": {
      "instance_creation_timestamp": 1674546966,
      "instance_id": "{INSTANCE-ID}",
      "instance_name": "dev",
      "project_id": "{PROJECT-ID}",
      "project_number": {PROJECT-NUMBER},
      "zone": "us-central1-a"
    }
  },
  "iat": 1682963407,
  "iss": "https://accounts.google.com",
  "sub": "{SOME-AZP}"
}

I've tried to keep the substitution names consistent above, to show where field values are duplicated.

Based on that claim set, it looks like the relevant uniquely identifying fields are:

  1. aud (which should be pypi, similar to GitHub-issued JWTs)
  2. azp: no idea what this is
  3. google.project_id: presumably configured by a user
  4. google.project_number: presumably a unique ID that prevents resurrection of google.project_id
  5. email: presumably derivable consistently from google.project_number

So, my first educated guess is that we'll want to allow users to configure (3) and (4). Does that sound right to you @di?


@woodruffw
Copy link
Member Author

Hmm, the google key doesn't appear in the Google Cloud docs: https://cloud.google.com/docs/authentication/token-types#id

@di
Copy link
Member

di commented May 1, 2023

azp: no idea what this is

This is some reference to the user the token was issued to

So, my first educated guess is that we'll want to allow users to configure (3) and (4).

These are specific to Compute Engine identities and might not be universally available. I think we'll just want to verify aud/sub/email.

More details here: https://cloud.google.com/docs/authentication/token-types#id

@woodruffw
Copy link
Member Author

Sounds good! It looks like email might be a little funky, since it should be derivable from the project number but is marked as optional. But I can certainly start with the assumption that it'll be present.

@woodruffw
Copy link
Member Author

Opened #13553 to kick this off: it refactors oidc.models into a module hierarchy, so that we can add more OIDC publishers without blowing up the size of the existing oidc/models.py.

@di
Copy link
Member

di commented May 2, 2023

Thinking about sub: I haven't found an easy/clean way for a user to get this value (the unique numeric ID of the identity). Additionally, I'm assuming (but haven't confirmed) if a service account is deleted//recreated with the same name, this ID will change. This would be good protection against resurrection attacks, but given that the service account identity includes the project number, which can't be resurrected, I'm not sure it's strictly necessary, so this might become an "optional" field (similar to environment for GitHub)

@di
Copy link
Member

di commented Jun 5, 2023

@woodruffw I can work on finishing #13571 and adding the forms/views if you want to work on the emails?

@woodruffw
Copy link
Member Author

@woodruffw I can work on finishing #13571 and adding the forms/views if you want to work on the emails?

Works for me! I'll work on the emails tonight.

woodruffw added a commit to trail-of-forks/warehouse that referenced this issue Jun 6, 2023
This makes the `trusted-publisher-added` and
`trusted-publisher-removed` email structures a little more
generic, allowing them to be re-used over both
GitHub and Google publishers. Future publishers will require
additional accommodations.

See pypi#13551.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member Author

Opened #13872 for the email side of this.

di added a commit that referenced this issue Jun 7, 2023
* oidc/models/google: add missing members

Signed-off-by: William Woodruff <[email protected]>

* tests, warehouse: general publishing emails

This makes the `trusted-publisher-added` and
`trusted-publisher-removed` email structures a little more
generic, allowing them to be re-used over both
GitHub and Google publishers. Future publishers will require
additional accommodations.

See #13551.

Signed-off-by: William Woodruff <[email protected]>

* warehouse: make publisher_url optional

Signed-off-by: William Woodruff <[email protected]>

* warehouse: `make translations`

Signed-off-by: William Woodruff <[email protected]>

* tests: fix google publisher_url test

Signed-off-by: William Woodruff <[email protected]>

---------

Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: Dustin Ingram <[email protected]>
@di
Copy link
Member

di commented Jun 8, 2023

Any thoughts on how we should refactor this table to support multiple types of publishers?

image

@woodruffw
Copy link
Member Author

Any thoughts on how we should refactor this table to support multiple types of publishers?

Yeah, I was thinking it might make sense to have it be a "tabbed" component like the publisher creation forms: rather than having a "publisher" column, the tabs would select the appropriate table to display by publisher name.

OTOH, if that's too complicated, would could instead simplify the table to just pending_project | publisher | details | remove button, where details would be some kind of implementation-defined render of the trusted publisher. That's sort of what I was going for with that original foo.yml @ user/repo syntax that I made up for GitHub publishers, but that was pretty opaque.

@di
Copy link
Member

di commented Jan 10, 2024

The implementation will be complete in #15144 but will be disabled via AdminFlag until we have docs and we're ready to make this available.

@di
Copy link
Member

di commented Jan 11, 2024

Here's something fun: the Google IdP doesn't provide a nbf claim in its OIDC tokens: https://developers.google.com/identity/openid-connect/openid-connect

But we currently require this claim to be present:

require=["iss", "iat", "nbf", "exp", "aud"],

@dstufft
Copy link
Member

dstufft commented Jan 11, 2024

I suspect we could just require one of iat and nbf? The _validate_nbf and _validate_iat functions in pyjwt are exactly the same other than where they pull their data from, and I think treating a lack of nbf as nbf = iat is still sound?

@woodruffw
Copy link
Member Author

Ah yeah, that's a great point -- I hadn't realized they have the exact same body. In that case, we can probably simply ignore nbf entirely and not even check it on a per-provider basis (given that iat is mandatory under OIDC).

@woodruffw
Copy link
Member Author

@dstufft
Copy link
Member

dstufft commented Jan 11, 2024

I think we should still check nbf if it's present? That may be what you mean and I'm just not parsing your message correctly.

@woodruffw
Copy link
Member Author

I think we should still check nbf if it's present? That may be what you mean and I'm just not parsing your message correctly.

Nope, that's my bad -- I had a chat open with @di, and I got things crossed. I was proposing checking just iat given what you said, but there's also no harm in checking nbf when present and it seems like a good basic token hygiene thing to do 🙂

@di
Copy link
Member

di commented Jan 11, 2024

I'm going to move forward with provider-specific checks for nbf if it's present.

@dstufft
Copy link
Member

dstufft commented Jan 11, 2024

👍 I dunno the OIDC spec, but at least with jwts there's in theory nothing stopping you from issuing a token today, that's not intended to be valid until tomorrow. I don't know why you'd do such a thing, but I think it's possible?

I also think we can just drop nbf from require but leave verify_iat=True and verify_nbf=True. Not sure that we need provider specific checks. PyJWT will just shortcircuit the verify_nbf call if nbf isn't present, but will still check it if it is present... unless we think there is value in requiring nbf for providers that currently emit it?

@di
Copy link
Member

di commented Jan 11, 2024

I also think we can just drop nbf from require but leave verify_iat=True and verify_nbf=True. Not sure that we need provider specific checks. PyJWT will just shortcircuit the verify_nbf call if nbf isn't present, but will still check it if it is present... unless we think there is value in requiring nbf for providers that currently emit it?

Aha, perfect!

@dstufft
Copy link
Member

dstufft commented Jan 11, 2024

@di
Copy link
Member

di commented Jan 12, 2024

Fix is here: #15197

@woodruffw
Copy link
Member Author

To keep things tracked: this is almost entirely done, and is just waiting on #15192 (which is itself pending documentation additions from the other in-flight provider implementations).

@woodruffw
Copy link
Member Author

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants