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

Add spec for Trusted Publishers (OIDC auth for NuGet) #13673

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

joelverhagen
Copy link
Member

@joelverhagen joelverhagen commented Aug 3, 2024

See the rendered functional description (main spec) and rendered technical description (supporting document).

This is a spec for NuGet/NuGetGallery#9332.

It brings OpenSSF's https://repos.openssf.org/trusted-publishers-for-all-package-repositories document to NuGet.org.

Please 👍 or 👎 this comment to help us with the direction of this feature & leave as much feedback/questions/concerns as you'd like on this issue itself and we will get back to you shortly.

Thank You 🎉

@JonDouglas
Copy link
Contributor

Here are some additional responses of ~n=100-200 on later questions on the recent survey we ran:

image image image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a branch item here? i.e.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch component of the sub identifier gets overridden when someone sets the environment in their YAML. But the environment isn't there by default. I'm not sure if we should only allow folks to specify which "channel" or "track" the packages come from via environment. It seems more git flavored to also allow a branch filter which will be possible on nearly all workflows out of the box vs. an environment specifier.

See https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-deployments/managing-environments-for-deployment

We could have a series of checkboxes. "Filter by environment: ___________", "Filter by branch: __________", "Filter by workflow file: __________". At least one would be required but you could filter by all 3 if you want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added clarification about the 3 filtering options (branch vs environment vs workflow file name)

the operation will be allowed. If the package ID is new and there is ambiguity on which package owner should be assigned
to the package (due to multiple matching trust policies), the most recently created trust policy will be used.

### Step 2: add a GitHub Actions workflow with NuGet.org Trusted Publisher authentication
Copy link
Contributor

@JonDouglas JonDouglas Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we will want to have a NuGet GH action template on the marketplace to simplify this beyond docs?

i.e. rubygems/rubygems.org#4286

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to a workflow template like this?
https://docs.github.com/en/actions/using-workflows/creating-starter-workflows-for-your-organization
Or a GitHub Action to do pack + auth + push all in one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nuget/login action in the design would be on the public marketplace. But it would only do the OIDC -> short-lived API key trade, and not the push step. ... but maybe it could? It would need a parameter to specify which .nupkgs to push.

I think it's better to keep the public marketplace action to be minimal, and re-usable for multiple use cases. If the action does nuget push internally, then we may end up needing all of the CLI parameters that go into nuget push, and add them as push gets more complex.

I would prefer nuget/login public marketplace action and not nuget/release for now. This also makes using the short-lived API key for unlist/relist and even deprecate API easier since all nuget/login does is produce an API key. It's not hardcode to always push.


## Unresolved Questions

Ask away!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no desired UI elements to indicate to end-users that a package has enabled this functionality today right?

Would we want to consider parts of the Build Provenance UI elements here?

image

i.e. Source Commit and Build File of the "trusted publisher"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of this design, no.

In your example screenshot there is a green check mark and the word "Signed" as well as a ledger entry. The green check mark only makes sense if there is some tamper protection or strong build provenance consideration (SLSA build level 2+ IMO).

We could perhaps show "Published from GitHub Actions", with the source commit and build file links, but it's not strong in the same sense that npm build provenance is strong where the artifacts are signed inside the workload, rather than just published.

See the latter paragraphs in the Auditing usage on NuGet.org section to see what I think we should commit to in this spec. Essentially, it's just recording the token (or some useful part of it) for later mining.

## Future Possibilities

- Enable additional CI/CD systems that support OIDC tokens
- Azure DevOps is next on the popularity list. Their workload identity federation is very focused on Azure today
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to AzDo based on usage and qualitative data posted in this PR thread as a comment screenshot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we can get Azure DevOps integration. That would help many Microsoft teams who publish from Azure DevOps to NuGet.org (e.g. NuGet client team, NuGet.* packages!).

Copy link
Contributor

@JonDouglas JonDouglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Awesome details and data driving this! Let me know when we can amplify and get more eyes on it. The functional bits look good to me and added comments as consideration points.

Clarify branch, environment, and workflow filters.
Add note about 2FA
Fix typo.
Clarify existing NuGet auth flows
Require HTTPS for service index and ApiKeyService endpoint
Mentioned public marketplace for GitHub Action
accepted/2024/trusted-publishers-oidc-for-nuget-push.md Outdated Show resolved Hide resolved
accepted/2024/trusted-publishers-oidc-for-nuget-push.md Outdated Show resolved Hide resolved
This step is pretty self explanatory. Many developers, even hobbyist developers who are only producing packages for
their own consumption, already have their source code stored in a Git repository, hosted on GitHub.

Additionally, GitHub Actions must be enabled for your repository. For public repositories, there is a generous free
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be explicit on whether a repository can be private.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! As long as the token is valid and issued from GitHub Actions (the known issuer base URL https://token.actions.githubusercontent.com), we will accept it. This can include private repositories.

(OIDC) tokens because they take the place of long-lived NuGet API keys as the credential. The NuGet package source must
also be configured to allow OIDC tokens from this specific build environment.

For the sake of understanding, we'll use GitHub and GitHub Actions as the example below, but it's important to note that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any minimum expectations on a forge, in terms of security, transparency, or whatever?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the future possibilities section with some vague details. I think the reality is that we will evaluate it on case-by-case basis. GitHub is a trusted, industry wide service so trusting their OIDC tokens is straight forward. If there is a request for a Trusted Publisher we, nor many of our users, know anything about, we will probably be a little skeptical and ask a lot of questions.

information about scopes on API keys, see [Scoped API
keys](https://learn.microsoft.com/en-us/nuget/nuget-org/scoped-api-keys). In summary:

- **Package owner**: for users that are members of one or more organizations, it's important to know which package owner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose you transfer ownership of a package to me. (You make me a coowner, and then I remove you.) How would I remove your ability to continue publishing packages with your trusted publisher policy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The moment you are removed as an owner, your trust policy will no longer work. The trust policy is moreso associated with a package owner rather than a package. In fact, the only this package-specific about a trust policy (or long lived API key) is a package ID scope.

Each time you upload a package version (via web, API key, or Trusted Publisher), whether or not the Package owner in question (the package owner property listed here) is a current, direct owner of the package. If it is not, the upload will be blocked.

I updated the spec to clarify this.

accepted/2024/trusted-publishers-oidc-for-nuget-push.md Outdated Show resolved Hide resolved
- `environment` claim must be `{environment}` (case insensitive)
- If a workflow path filter is provided in the trust policy:
- `job_workflow_ref` claim must be `{repo owner}/{repo name}/{workflow path}@.*` (case insensitive)
- The workflow path should be normalized to `/` path separators at the time of trust policy creation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this NuGet.org's responsibility? Is it as simple as replacing all \ with /?

Copy link
Member Author

@joelverhagen joelverhagen Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be NuGet.org's responsibility, as a nicety. It would be very annoying to get auth errors, just because you copied the relative path from a Windows machine. Yes, it should be that simple.

In addition to the general JWT checks, specific checks are made for each Trusted Publisher. For GitHub Actions, the
following checks will be made:

- `sub` claim matches `{repo owner}/{repo name}:.*` (case insensitive)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit difficult for me to understand what's variable and what's constant. For example, this might be reworded to:

sub claim matches the regular expression pattern {repo owner}/{repo name}:.* where {repo owner} is the repository owner's name and {repo name} is the repository's name. Then, I would understand dtivel/myrepo:somepath as a matching pattern.

(I inferred that * is something of a wildcard or maybe .* is a regular expression pattern, but I can't be certain from the spec.)

What would an actual sub look like?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expressed it as a regex. You're right, that's confusing. I'll clarify that and add a sample.

A branch sub would look like this: repo:octo-org/octo-repo:ref:refs/heads/demo-branch.

Comment on lines 110 to 111
scalability it must opt to rate limit the endpoint instead of caching. NuGet.org will rate limit the endpoint to 1 API
key created per 30 seconds, per user.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't grok how this limit aligns with someone publishing many packages in a short period of time. Consider the .NET team publishing packages in bulk to NuGet.org. Will that translate to hundreds or thousands of API calls within a short timeframe? Or, is the API key cached and reused on the client side?

It would be good to discuss here how you see this limitation not being a problem for various publishing scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will that translate to hundreds or thousands of API calls within a short timeframe? Or, is the API key cached and reused on the client side?

I expect the API key to be cached on the client side. In most workflows (no custom scripting), this will be done by a single nuget/login workflow step with a single output value, which is the short lived API key. This could be used one or more push operations (all referring to the single step output == single API key). I imagine most users will do dotnet nuget push *.nupkg which internally does multiple push operations with a single API key provided via CLI arg.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose dotnet nuget push *.nupkg takes longer than 15 minutes. Imagine lots of packages, slow network, whatever. Will NuGet client automatically re-request a new token via nuget/login midstream, as necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not with this version of the proposal. See the https://github.com/NuGet/Home/blob/jver-oidc/accepted/2024/trusted-publishers-oidc-for-nuget-push.md#enable-nuget-authentication-provider-integration-to-avoid-token-expiration-for-long-workflows.

The user will be able to work around the issue with multiple nuget/login GitHub Actions steps and breaking up the push steps. It's not a great solution, but the data suggests we can pick a single short lifetime (15 minutes) and cover the vast majority of cases.

Content-Type: application/json

{
"api_key": "{short lived API key in clear text}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any explicit mention of how long the short-lived API key will be valid. I think that's intentional, since we should have some flexibility in adjusting its lifespan. However, for the sake of argument, what is a reasonable starting point? 30 minutes?

What is the user experience if the short-lived API key expires before they're done publishing and how do they recover? Or, is the client (NuGet client, here) supposed to automatically reacquire a new API key transparently so that the user never notices the expiration?

I'm trying to understand where this automagical flow might actually break down for users.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any explicit mention of how long the short-lived API key will be valid. I think that's intentional, since we should have some flexibility in adjusting its lifespan. However, for the sake of argument, what is a reasonable starting point? 30 minutes?

I actually thought I documented. I did not 😂. I was planning on using 15 minutes. This is what PyPI uses and it should have good coverage for our push patterns. I will add supporting data.

- `sub` claim matches `{repo owner}/{repo name}:.*` (case insensitive)
- The suffix of the `sub` is implied by the other checks
- `repository_owner` claim matches the `{repo owner}` name (case insensitive)
- `repository_owner_id` claim matches the numeric owner ID recorded at the time of the trust policy creation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This numeric owner ID is forge-assigned (here GitHub-assigned)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, GitHub assigned.

I should note that other forges may not have a numeric owner ID. We will need to assess resurrection attacks or any other impersonation risks on a case by case basis.

joelverhagen and others added 4 commits August 9, 2024 12:16
Renamed resource from ApiKeyService to TokenService to be a bit more flexible for the future.
Copy link

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic write up! Excited to see this spec!

| extend ["% of owners"] = round(100.0 * Owners / TotalUniqueOwnersSets, 2)
-->

| Forge | Package IDs | Owners | % of package IDs | % of owners |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to also note which of these platforms support OIDC for their workflows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. I'll do that research and include a little note in the table.

CI/CD systems will be supported so some users will still need to use the older authentication flows such as long-lived,
manually rotated API keys.

The trust policy is configured once and works forever. An API key forces the package author to re-assess CI/CD

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on if you would let a package "downgrade" to move off trusted publishing? And a similar question, would you allow both to be configured, or only one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm good question! I think starting out we would allow full flexibility in a "walk-crawl-run" approach. As we mature, we see how the feature goes in our ecosystem, we can lock things down more. We took a similar approach with requiring 2FA on our website. At first it was opt in, then opt out (new accounts had it enabled), then it was mandated. This allowed us to look at customer feedback as we went and not move so quickly that folks got stuck.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a section about this.

The metadata available in a GitHub Actions OIDC token is mentioned in GitHub's [Understanding the OIDC
token](https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect#understanding-the-oidc-token).

These claims could offer useful indicators to package consumers about the package. In order to support a future effort

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is orthogonal for trusted publishing, but I might avoid discussing SLSA Build L1 - It doesn't add much given it's not a signed attestation and if the registry was constructing the provenance, then it requires trust in the registry that it properly recorded the claims, rather than the builder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. I do want to mention SLSA Build levels to come extent, at least to say this does NOT provided L2+! I talked to Zach S. on the OpenSSF Slack. He said using this information would be L1 but it's not great since it is tamperable. I would say this metadata is better than package author-attested repo/project URL since the consumer needs to just trust the registry and GitHub Actions, but it's not as good as a proper signed provenance thing.


#### What NuGet API operations can be used inside the GitHub Actions workflow?

To scope the ways in which a GitHub trust policy can be used, similar scoping rules to the current API key flow can be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend investigating the current usage of API Key scopes to understand how they are used. For example, are globs commonly used? Are there API keys that only allow pushing new versions? Are they common? I have a suspicion that this functionality isn't used, so we should check before carrying it forward. We might want to make changes / simplify the MVP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. I was assuming parity would be the easiest since we would be stamping out short-lived API keys from the OIDC token, so all of the existing API key scoping can apply. But there is still cost to adding these parameters to the trust policy definition and making the UI work and look pretty+accessible. I will check the usage and modify the spec if adoption is "low".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the data. I think we can definitely get by without package ID and allowed action scoping, especially since the API keys are short-lived.

Copy link
Contributor

@dtivel dtivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm partial to calling this "OIDC Publishers" over "Trusted Publishers", but otherwise this spec looks good.

to see all of the properties.

For the sake of simplicity, we will start with the following workflow filters:
- Filtering for a specific *branch*, as shown in the UI above (branch filter)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this also support tags? For example, when a tag is created v1.2.3 and a workflow is kicked off automatically on tag creation to build that version and then publish it.

Copy link
Member Author

@joelverhagen joelverhagen Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm generally a tag trust policy should be possible: https://docs.github.com/en/actions/security-for-github-actions/security-hardening-your-deployments/about-security-hardening-with-openid-connect#filtering-for-a-specific-tag

Question for you: would you want this feature? It seems like you would need to update the trust policy for every release as your version tag increments.

Copy link
Member Author

@joelverhagen joelverhagen Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll note the existing filters are meant to be "invariants" you set once and change infrequently (branch name, env name, workflow path). Tag seems different in this regard.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess more what I'd be after is a policy that matches a tag with pattern (e.g. :ref:refs/tags/v*) rather than exact match.

My typical release process is to draft a release for a tag in GitHub that has a specific version (e.g. v1.2.3) with the release notes prepared etc. Then when I'm ready to ship, I publish that release which creates the tag. The triggers on my CI workflow are then setup like this:

on:
  push:
    branches: [ main ]
    tags: [ v* ]
  pull_request:
    branches: [ main ]
  workflow_dispatch:

Publishing then happens based on a condition like this:

if: |
  github.event.repository.fork == false &&
  startsWith(github.ref, 'refs/tags/v')

The net effect is that the "real" build happens after tagging, and that packages get shipped off to NuGet.org once produced.

So in a vacuum, I would match on the tag rather than a branch as I wouldn't want any push to main to be allowed to publish.

If tags aren't supported, then I would use an environment rather than a branch, as I currently use environments to scope the NuGet API key secret to that environment so it's not available to all the workflow runs (it also lets me manually approve the push job).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I heartily endorse @martincostello's comments above - this is how a very large set of OSS projects are versioned, not just in the .NET ecosystem but broadly across other ecosystems as well. Not supporting pattern-based, tag-based workflows would be a huge miss IMO.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be useful looking into branch wildcard support as well - I know that e.g. in dotnet we use pattern-based rules in many other places to signify some special status - e.g. for the SDK ^release/\d\.\d\.\d\d\d$ is a stable release branch and may be able to trigger certain workflows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the document with wildcard support on tags and branches. This aligns with the pattern matching on package ID scopes. I am not sure if a regex is the right UX but I'm happy to hear other opinions on that!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if a regex is the right UX but I'm happy to hear other opinions on that!

Glob patterns vs. regex have both been used as examples here, so I think it's worth clarifying which since the syntax is so similar for trivial cases. A lot of places use glob patterns for branch matching, so there may be some confusion if regex is used.

Glob pattern above: :ref:refs/tags/v* (typically converted to regex: :ref:refs/tags/v.* though this is not precisely the same; it would be more accurate as :ref:refs/tags/v[!/]* though that's hardly intuitive)
Regex pattern above: ^release/\d\.\d\.\d\d\d$ (converted to glob: release/[0-9].[0-9].[0-9][0-9][0-9])

Worth noting in the glob pattern example, typically glob expansion only happens within the current directory level, so v* will match v1.2.3 but not version/1.2.3. The simple regex conversion .* does not respect path delimiters, so v.* would match both v1.2.3 and version/1.2.3.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify - I'd be perfectly happy for just globs, the regex was just meant to be a related example of branch and ref names being signals based on structure. I'd expect users to use release/* for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @jimmylewis, in my experience the term globbing is most often used for file system/file paths. I am not sure if the concept cleanly translated in users' minds to tag/branch names with slashes. IMO we can do something simple where * expands to .* regex and not worry about including slashes in that that capture.

Today "globbing" on NuGet.org API key scopes is like this:
https://github.com/NuGet/NuGetGallery/blob/a4b3c64acb65ca68eb67ac6b51242356aa5e4da7/src/NuGetGallery.Services/Extensions/ScopeExtensions.cs#L54-L58

@joelverhagen joelverhagen marked this pull request as ready for review September 5, 2024 12:34
@joelverhagen joelverhagen requested a review from a team as a code owner September 5, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants