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

Added proposal to improve support for packages signed with a Trusted Signing certificate. #13791

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

Conversation

dlemstra
Copy link

@dlemstra dlemstra commented Sep 17, 2024

edit by @joelverhagen:@JonDouglas(feedback/upvote section)

This is a proposal to address NuGet/NuGetGallery#10027.

Rendered Markdown

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 🎉

@dlemstra dlemstra requested a review from a team as a code owner September 17, 2024 05:36
@dotnet-policy-service dotnet-policy-service bot added the Community PRs (and linked Issues) created by someone not in the NuGet team label Sep 17, 2024
- The signing certificate has a valid counter-signature (timestamp).
- The signing certificate contains the Trusted Signing EKU.
- The signing certificate contains a Public Trust Identity EKU.
- The Public Trust Identity EKU is not already linked to the user.
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, we will allow an EKU to be linked to multiple users/orgs right? This is possible today since the same .cer file can be uploaded by multiple users and this means the same fingerprint is possible.

I think it is okay to allow this because a) both users/orgs would need access to signing permission for this to matter and b) there may be legitimate scenarios for this, such as one signing identity (TS account) but multiple NuGet.org profiles (one user with multiple orgs).

Copy link

@ianjmcm ianjmcm Sep 17, 2024

Choose a reason for hiding this comment

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

The unique EKU for the validated identity in Trusted Signing is tied to the Subscription, so other accounts and certificate profiles will be able to use that unique EKU. This allows orgs to organize their account resources with their business/development units as they see fit.

Please consider that the signing certs will not be signed by the root directly, Microsoft Identity Verification Root Certificate Authority 2020, but its issuer will be chained to the Microsoft ID Verified Code Signing PCA 2021 that is signed by the root. Only code signing certs under this CA in the hierarchy should be accepted for NuGet author signing from Trusted Signing.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the PR to make it more clear that it should be the root CA.

Copy link

@ianjmcm ianjmcm left a comment

Choose a reason for hiding this comment

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

Only a couple of minor clarifications on the signing cert's signer not being directly the root CA, and clarification on subjectDN value changes may not have an impact on the EKU.

- The signing certificate has a valid counter-signature (timestamp).
- The signing certificate contains the Trusted Signing EKU.
- The signing certificate contains a Public Trust Identity EKU.
- The Public Trust Identity EKU is not already linked to the user.
Copy link

@ianjmcm ianjmcm Sep 17, 2024

Choose a reason for hiding this comment

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

The unique EKU for the validated identity in Trusted Signing is tied to the Subscription, so other accounts and certificate profiles will be able to use that unique EKU. This allows orgs to organize their account resources with their business/development units as they see fit.

Please consider that the signing certs will not be signed by the root directly, Microsoft Identity Verification Root Certificate Authority 2020, but its issuer will be chained to the Microsoft ID Verified Code Signing PCA 2021 that is signed by the root. Only code signing certs under this CA in the hierarchy should be accepted for NuGet author signing from Trusted Signing.

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

This looks great, @dlemstra! I will work on getting at least one more team member on board with this design, then we can merge it and talk about implementation.

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.

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a related comment elsewhere in this PR. This looks to me like a screenshot from the Trusted Signing website, not a mockup for NuGet.org. I'm not sure though, because I don't have a Trusted Signing account to verify.

As we look to this screenshot as prior art, I take issue with the label "Enhanced key usage." "Enhanced key usage" is not an accurate description of its right-hand value. The value is the Trusted Signing subscriber identity. (@ianjmcm, correct me if mistaken.) Certificates can have many EKU's, and the Trusted Signing subscriber identity is just one of them.

It is true that the subscriber identity is stored in an EKU, but I think of this like a library card or driver's license. There's a barcode on those which is unique to you. You wouldn't label that value "Barcode", you'd label it "Library user ID" or "Driver ID", respectively.

I think "Trusted Signing subscriber ID" would be a more accurate and useful label than some other terms in this proposal ("Trusted Signing EKU's", "Public Trust identity EKU", "Public trust identity", ...). This proposal should use consistent terminology throughout. That the value is stored in an EKU is an implementation detail.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe both NuGet and Azure should use the same name for this? Would it be possible to change this in the interface of Azure @ianjmcm?

Copy link
Author

@dlemstra dlemstra Jan 5, 2025

Choose a reason for hiding this comment

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

I have updated the proposal to use Durable Identity Value as mentioned by @ianjmcm in another comment.


The EKU will only be linked when the following conditions are true:
- The package was signed within the last 30 days.
- The signing certificate must be issued by the CA certificate `Microsoft Identity Verification Root Certificate Authority 2020`.
Copy link
Contributor

Choose a reason for hiding this comment

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

@dlemstra, @ianjmcm's feedback here was that the signing certificate's issuer should not be the root CA. Certainly, the signing certificate should chain to this root, but that's not the same thing. The signing certificate's issuer should chain to the "Microsoft ID Verified Code Signing PCA 2021" intermediate CA (or policy CA), like I've illustrated below.

I don't know if there is a hard restriction on chain length. In the abstract, there could be not just 1 but more than one intermediate CA's between the PCA and the subscriber certificate. @ianjmcm's point was that we can expect that the subscriber certificate will eventually chain up to "Microsoft ID Verified Code Signing PCA 2021". I think it's safest to not hard-code expectations in our code as to the chain length (i.e.: whether there is 1 or more CA's between the PCA and the end certificate).

graph TD
    A["Microsoft Identity Verification Root Certificate Authority 2020"]
    click A "https://crt.sh/?q=5367f20c7ade0e2bca790915056d086b720c33c1fa2a2661acf787e3292e1270" "Root CA"
    A --> B["Microsoft ID Verified Code Signing PCA 2021"]
    click B "https://crt.sh/?q=3d29798cc5d3f0644a7e0dc9cb1cade523ea5ec83b335109b605bfeaa7d5f5c1" "Intermediate CA"
    B --> C["issuing CA (e.g.:  Microsoft ID Verified CS AOC CA 02"]
    C --> D["subscriber certificate"]
Loading

Copy link
Author

Choose a reason for hiding this comment

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

I think we can only check if the root certificate is the Microsoft Identity Verification Root Certificate Authority 2020 certificate and not "care about" the chain? Or am I misunderstanding you @dtivel?

- The package was signed within the last 30 days.
- The signing certificate must be issued by the CA certificate `Microsoft Identity Verification Root Certificate Authority 2020`.
(https://learn.microsoft.com/en-us/azure/trusted-signing/concept-trusted-signing-trust-models)
- The signing certificate has a valid counter-signature (timestamp).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nonsensical. There isn't standard way to countersign (or timestamp) a certificate.

I think what you meant is that the package signature has a valid timestamp.

Copy link
Contributor

@dtivel dtivel Oct 10, 2024

Choose a reason for hiding this comment

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

Actually, I think you should avoid restating unchanged requirements like this. It creates confusion around what the authoritative, complete requirements are.

This public document already states this requirement.

You can remove any requirement here that is already in that list.

Copy link
Author

Choose a reason for hiding this comment

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

The reason this requirement was included is to make sure that the EKU is one that was added by the Trusted Signing service in Azure.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • The signing certificate has a valid counter-signature (timestamp).

This requirement no relevance to ensuring "that the EKU is one that was added by the Trusted Signing service in Azure."

The next line expresses the requirement you describe.

You should remove this requirement.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the proposal to remove this requirement

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the same label/term everywhere, whether that is "Trusted Signing subscriber ID", "Public trust identity", or something else. Find and replace.

Copy link
Author

Choose a reason for hiding this comment

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

@ianjmcm Do you have suggestion for the term that we should use?

Copy link

Choose a reason for hiding this comment

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

While the portal labels this value to the field of the x.509 cert we place it in (EKU), it is in reality the "Durable Identity Value" and our docs call it out as "Subscriber identity validation EKU". I think it is best to go with "Durable Identity Value" because in the future you may want to generically use authenticated values in x.509 certs to achieve recognition of a durable identity value independent of how Trusted Signing is doing it today.

Copy link
Author

Choose a reason for hiding this comment

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

I have update the proposal to use Durable Identity Value.


## Unresolved Questions

<!-- What parts of the proposal do you expect to resolve before this gets accepted? -->
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we (NuGet.org) get a Trusted Signing certificate for testing this feature?

Copy link
Member

Choose a reason for hiding this comment

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

I can help with manual testing since I am personally onboarded. I am not sure if we need regular E2E testing (needing a new Trusted Signing signature per test run). Are you referring to manual or automated testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can help with manual testing since I am personally onboarded.

Suppose you go on leave for a while.

Are you referring to manual or automated testing?

Neither specifically at this time, though automated testing is preferrable. I'm thinking about E2E testability during development, release, and post-release.


## Rationale and alternatives

<!-- Why is this the best design compared to other designs? -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't an alternative solution that we integrate with Trusted Signing and retrieve the subscriber ID directly?

Copy link
Author

Choose a reason for hiding this comment

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

That would still require some kind of integration with the Azure API that would require authorization setup. That feels more complicated?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not suggesting this as a replacement for the current proposal.

The purpose of this section is to present alternatives and rationale "[w]hy this is the best design compared to other designs." Complexity and cost are solid motivators.

When someone looks back and wonders if we considered option ___, ideally, it would have been captured in this section.

Choose a reason for hiding this comment

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

I can also help with testing as I am also onboarded with Trusted Signing.

Comment on lines 35 to 37
I would like to propose to link EKU's to the user in a similar way as that is now done with certificates:

![EKU](images/trusted-signing-eku.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that this screenshot is from Trusted Signing website, but I don't know for sure. It does not look like a mockup for NuGet.org. I think it's important to call out that this is a screenshot from the Trusted Signing website not exactly what you're proposing for NuGet.org.

Copy link
Author

Choose a reason for hiding this comment

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

This is a mermaid diagram? Are you referring to another image?

Copy link
Author

Choose a reason for hiding this comment

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

I have update the text around the Azure Portal screenshot to make this more clear.

@zooba
Copy link

zooba commented Oct 14, 2024

I'd love to see this! Copying the EKU manually (rather than uploading .cer) is fine by me, though I'd also be happy with uploading an entire package for certificates to be extracted, or using an existing package (uploaded prior to enabling checks). But it's a one-time operation as far as I'm concerned, so as long as I don't need to install any extra tools or sign up at new websites just to do it, it can be a few manual steps.

@dlemstra
Copy link
Author

dlemstra commented Jan 5, 2025

I have just pushed some changes to address the comments that were made earlier. I finally made some time to focus on this proposal again, thank you all for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs (and linked Issues) created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants