-
Notifications
You must be signed in to change notification settings - Fork 256
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
base: dev
Are you sure you want to change the base?
Changes from 3 commits
5c2ce6d
8f701de
12359cf
f2e5cf2
42262de
0240ae7
a3c0ba9
1940132
2983999
2dbf5f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ianjmcm Do you have suggestion for the term that we should use? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have update the proposal to use Durable Identity Value. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
# Improve support for NuGet packages that are signed with a Trusted Signing certificate | ||
|
||
- Author Name: Dirk Lemstra [dlemstra](https://github.com/dlemstra) | ||
- GitHub Issue: https://github.com/NuGet/NuGetGallery/issues/10027 | ||
|
||
## Summary | ||
|
||
This proposal is about improving the support for NuGet packages that are signed with a Trusted Signing certificate. This proposal | ||
consists of three steps to improve the experience for users that sign their packages with Trusted Signing. These three steps can | ||
be released separately but I can also understand that the NuGet team would want to launch this as a complete feature. | ||
|
||
## Motivation | ||
|
||
Earlier this year [Trusted Signing](https://learn.microsoft.com/en-us/azure/trusted-signing/) was launched by Microsoft and | ||
recently support for this was added to `dotnet sign`. Packages signed with this can be uploaded to the NuGet Gallery. But | ||
dlemstra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
because the certificate is only valid for three days a user will probably always need to update their certificate on their | ||
dlemstra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
account page. I would like to propose a set of changes to improve the experience for users that sign their NuGet package | ||
with Trusted Signing. | ||
|
||
## Explanation | ||
|
||
### Functional explanation | ||
|
||
### Step 1: Register EKU of certificate that was added by Trusted Signing | ||
dlemstra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### How would this be implemented? | ||
|
||
The first step would be to store the EKU that is inside the certificate that was used to sign the NuGet package with Trusted | ||
Signing. The certificate of Trusted Signing is only valid for thee days but it contains an EKU to that can be used to get the | ||
dlemstra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
identity of the owner of the certificate. There are two EKU's in the certificate. One that tells us that it was signed with | ||
Trusted Signing (`1.3.6.1.4.1.311.97.1.0`) and another one that contains the Public Trust identity (e.g. | ||
`1.3.6.1.4.1.311.97.990309390.766961637.194916062.941502583`). More details about this can be found here: | ||
https://learn.microsoft.com/en-us/azure/trusted-signing/concept-trusted-signing-cert-management. | ||
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a mermaid diagram? Are you referring to another image? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
But this would require a user to enter that EKU in the interface to link it with their account. We could also start with | ||
automatically linking the EKU to the user by using the current "certificate upload" interface. The user would need to do the | ||
following: | ||
- Extract the `.cer` file of the signing certificate from the NuGet package. | ||
- Upload the `.cer` file and link it to their account. | ||
- Upload the NuGet package that was signed with Trusted Signing. | ||
|
||
Below is a picture that show how the certificate and EKU would be linked to user before and after the NuGet package is uploaded. | ||
|
||
![EKU link](images/trusted-signing-eku-link.png) | ||
|
||
This would mean that the certificate is no longer linked to the user and will no longer be shown in the interface because the EKU | ||
is now linked to the user. That will mean that the user would need to contact the NuGet team to remove the EKU link until updates | ||
to the interface have been made. But I don't think a lot of people will need this functionality right away. | ||
|
||
The EKU will only be linked when the following conditions are true: | ||
- The package was signed within the last X days where X should be decided by the NuGet team (not sure if this is really necessary). | ||
dlemstra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- The signing certificate must be issued by the CA certificate `Microsoft Identity Verification Root Certificate Authority 2020`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"]
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can only check if the root certificate is the |
||
(https://learn.microsoft.com/en-us/azure/trusted-signing/concept-trusted-signing-trust-models) | ||
- The signing certificate has a valid counter-signature (timestamp). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated the proposal to remove this requirement |
||
- 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
This mean that the following would happen inside the service that handles the upload of NuGet packages: | ||
|
||
![NuGet package upload](images/trusted-signing-upload.png) | ||
|
||
As shown in the image there are is no check if the certificate is expired when the user uploaded a Trusted Signing certificate. | ||
This now happens when the user uploads a certificate but this check should not be added to Trusted Signing certificates because | ||
they will expire after three days. | ||
|
||
#### How does this help the user? | ||
|
||
This helps the user because they will now no longer need to upload a `.cer` file everytime they publish a new NuGet package that | ||
was signed with the same Trusted Signing certificate. But inside the NuGet Gallery they will no longer see the certificate that | ||
they uploaded earlier. This could confuse the user so we could keep the link and let the user remove it manually. But doing this | ||
would also not make it clear that an EKU was linked to their account. Making this visible in the interface will be the next step | ||
of these changes. | ||
|
||
### Step 2: Change how Trusted Signing EKU's are shown in the NuGet Gallery. | ||
|
||
#### How would this be implemented? | ||
|
||
The next steps would be to change how this will be displayed inside the NuGet Gallery. Below is an image of how this could be | ||
added to the account page of a user (please forgive me for my css skills). The naming is a proposal and I am open to suggestions | ||
but I think it would be best to show the EKU's in a separate table. | ||
|
||
![User profile](images/trusted-signing-profile.png) | ||
|
||
This image demonstrates how this would look when a user has both an EKU and certificate linked to their account. As mentioned in | ||
step 1 the certificate would no longer be shown and the user would only see their EKU's. The Subject and Issuer will be extracted | ||
from the certificate the first time and added to the EKU data. Future uploads should also check the Subject and Issuer and update | ||
the data if they have changed. | ||
|
||
#### How does this help the user? | ||
|
||
When only step 1 is implemented it will already be a huge help for users that use Trusted Signing but it will look strange inside | ||
the NuGet Gallery because no certificates are shown anymore and they cannot unlink their EKU. Adding this separate table with EKU's | ||
linked to the user their account would allow them to remove this EKU. I personally don't think users would really need this | ||
functionality right away so that is why I think this could be added as a second step. | ||
|
||
### Step 3: Allow a user to submit a Public Trust identity EKU in the NuGet Gallery | ||
|
||
#### How would this be implemented? | ||
|
||
When a `.cer` file is uploaded only the thumbprint is stored because the certificate is not parsed by the NuGet Gallery due to | ||
security reasons. This means we cannot add the Public trust identity EKU record when a certificate is uploaded. For a user that | ||
has access to the Azure Portal it would be easier to copy the EKU from the interface: | ||
|
||
![Azure Portal](images/trusted-signing-azure.png) | ||
|
||
And this can also be done before a NuGet package has been signed. This copied value should then be entered into a form that adds | ||
this EKU to the user. As mentioned earlier the Subject and Issuer will be set once the first package that has this EKU is uploaded. | ||
|
||
#### How does this help the user? | ||
|
||
Adding this in the interface would make it possible for a user to register an EKU in their account. This would be easier than | ||
extracting the certificate from a signed NuGet package. The process to get that `.cer` file from a signed package is not that | ||
simple. This would be the final step that would give a user full control over their EKU's. | ||
|
||
### Technical explanation | ||
|
||
<!-- Explain the proposal in sufficient detail with implementation details, interaction models, and clarification of corner cases. --> | ||
|
||
## Drawbacks | ||
|
||
<!-- Why should we not do this? --> | ||
|
||
## Rationale and alternatives | ||
|
||
<!-- Why is this the best design compared to other designs? --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
<!-- What other designs have been considered and why weren't they chosen? --> | ||
<!-- What is the impact of not doing this? --> | ||
|
||
## Prior Art | ||
|
||
<!-- What prior art, both good and bad are related to this proposal? --> | ||
<!-- Do other features exist in other ecosystems and what experience have their community had? --> | ||
<!-- What lessons from other communities can we learn from? --> | ||
<!-- Are there any resources that are relevant to this proposal? --> | ||
|
||
## Unresolved Questions | ||
|
||
<!-- What parts of the proposal do you expect to resolve before this gets accepted? --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suppose you go on leave for a while.
Neither specifically at this time, though automated testing is preferrable. I'm thinking about E2E testability during development, release, and post-release. |
||
<!-- What parts of the proposal need to be resolved before the proposal is stabilized? --> | ||
<!-- What related issues would you consider out of scope for this proposal but can be addressed in the future? --> | ||
|
||
## Future Possibilities | ||
|
||
Users of the NuGet client can now [trust a signing certificate](https://learn.microsoft.com/en-us/nuget/reference/cli-reference/cli-ref-trusted-signers) | ||
but that now needs a thumbprint so support for the Public Trust identity EKU should also be added there. It is an existing issue | ||
for users that want to trust a signing certificate that was issued by Trusted Signing. But this is something that can be added | ||
at a later point in time. | ||
|
||
And in the future it might also be a good idea to show the EKU of the certificate that was used to signed in the package overview | ||
instead of the thumbprint but I think this can be added at a future point in time. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.