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

[Spec]: Add support to allow package authors to sign packages #5907

Closed
mishra14 opened this issue Sep 19, 2017 · 23 comments
Closed

[Spec]: Add support to allow package authors to sign packages #5907

mishra14 opened this issue Sep 19, 2017 · 23 comments

Comments

@mishra14
Copy link
Contributor

Goal: Tracking the spec for #5904

@mishra14 mishra14 added this to the 4.5 milestone Sep 19, 2017
@mishra14 mishra14 self-assigned this Sep 19, 2017
@mishra14
Copy link
Contributor Author

mishra14 commented Sep 25, 2017

The spec is currently in review and it is available here.

Update: I have updated the link to the public spec.

@forki
Copy link

forki commented Sep 25, 2017

404

@mishra14
Copy link
Contributor Author

@forki please check the link now. You should be able to access it.

@maartenba
Copy link
Contributor

Is a timestamper provided or is this something the package author must provide?

@mishra14
Copy link
Contributor Author

@maartenba Current spec requires the user to provide a timestamper service url.

@jariq
Copy link

jariq commented Oct 2, 2017

I am not sure I understand what is -SigningAlgorithm option good for. It is described as

Signing algorithm to be used. Allowed values are RSA and ECDSA. Defaults to RSA.

but in my experience the signing algorithm is chosen by the type of public key present in the certificate. ECDSA cannot be used with certificate issued to RSA key pair and vice versa. Are you sure this option is needed?

@jariq
Copy link

jariq commented Oct 2, 2017

If you plan to provide built-in multiplatform support for HSMs and other hardware based key stores then IMO there is currently only one viable option and that is PKCS#11 ANSI C API. It's been here since 1995 when it was created by RSA Laboratories and currently it is being maintained by OASIS PKCS11 Technical Committee that released latest version of specification in 2015. In my experience, most (if not all) of the commercially available HSMs and smartcards come with an unmanaged library implementing PKCS#11 interface. There is even an pure software implementation available called SoftHSM which is invaluable for testing.

When it comes to .NET support there is an open source PKCS#11 wrapper library called Pkcs11Interop (I am the shameless self-promoting author) that is netstandard1.3 compatible and works well on all major desktop platforms (Windows, Linux and OSX). Pkcs11Interop comes with built-in support for PKCS#11 URIs (defined in RFC 7512) which fit nicely as a value of proposed -CertificatePath parameter. Let me provide an example:

  1. After reading a proposal I guess that with CryptoAPI certificate source one would use following command to sign a package:
    nuget.exe sign package.nupkg -CertificatePath "cert:\CurrentUser\My\d5de31ea974f5ea8581d633eeffa8f3ea0d479bb"
    This URI tells NuGet to use certificate with thumbprint d5de31ea974f5ea8581d633eeffa8f3ea0d479bb located in CurrentUser\My certificate store. Please correct me if I am misinterpreting the proposed URI format.

  2. With PKCS#11 certificate source one would use following command to sign a package:
    nuget.exe sign package.nupkg -CertificatePath "pkcs11:token=OSSCodeSigning;type=cert;object=jaroslav.imrich@gmail.com?module-path=cardos11_64.dll"
    This PCKS#11 URI tells NuGet/Pkcs11Interop to load cardos11_64.dll unmanaged library, find token with OSSCodeSigning label and certificate object with jaroslav.imrich@gmail.com label.

It would be really nice to have built-in PKCS#11 support in NuGet but I understand it might not be entirely possible. For example it might not be allowed to introduce additional dependencies to NuGet code base or it might not be allowed to load unmanaged libraries into nuget process. So as an alternative to built-in HSM support you may also consider making signing API pluggable so I would be able to write and maintain PKCS#11 plugin and other authors would be able to do the same for other key stores (I can imagine @onovotny and @vcsjones would want to write Azure KeyVault plugin).

@clairernovotny
Copy link

@jariq To be clear, we'd most likely reappropriate the signing digest code and wrap it into a different utility. Key Vault doesn't have a PKCS#11 windows api yet (afaik), and we'd want something that doesn't require any installation on the machine. That's one nice thing of our utilities, is that it's pure xcopy.

@mishra14
Copy link
Contributor Author

mishra14 commented Oct 2, 2017

@jariq -

I am not sure I understand what is -SigningAlgorithm option good for.

Thanks for the feedback. I will investigate this a bit to make sure that I have the right params outlined.

After reading a proposal I guess that with CryptoAPI certificate source one would use following command to sign a package:
nuget.exe sign package.nupkg -CertificatePath "cert:\CurrentUser\My\d5de31ea974f5ea8581d633eeffa8f3ea0d479bb"
This URI tells NuGet to use certificate with thumbprint d5de31ea974f5ea8581d633eeffa8f3ea0d479bb located in CurrentUser\My certificate store. Please correct me if I am misinterpreting the proposed URI format.

That is correct.

If you plan to provide built-in multiplatform support for HSMs ....

Thanks for your detailed feedback on HSMs. I will look into this and the feasibility of your suggestion.

@vcsjones
Copy link

vcsjones commented Oct 3, 2017

I am not sure I understand what is -SigningAlgorithm option good for.

Agree. That should (must) be dictated by the signing key being used. Regardless, much kudos for supporting ECC. This is often overlooked in code signing. I suppose the standard NIST curves would apply here. However, macOS doesn't readily support some of the curves Windows does (p521, brainpool). I don't know if you want to limit at signing time which curves you support for signing.

I would recommend getting rid of this. It doesn't do anything. You can't set this value to anything other than what the certificate is. This can be determined by the OID of the SPKI of the certificate.

HashingAlgorithm ... Defaults to SHA512.

SHA512 is overboard as a default, in my opinion. I understand the notion of "bigger numbers are better". Frankly I would just use SHA256 though would be perfectly fine with 384 as well if there is some concern around SHA256 weaknesses. Though I see no problem with defaulting with SHA512 except for the next item.

Timestamps

There is no option to specify the digest algorithm for the timestamp (the signtool equivalent is /td when used with the /tr option). Is it intended that this will be the same as HashingAlgorithm? If so, the SHA512 default algorithm makes even a little less sense, some timestamp services don't like SHA512 - 256 or 384 is fine for most).

I would recommend introducing a -TimestampHashAlgorithm option and pick a sane default.

Whats the best way to pass the certificate password to the sign command?

I honestly wouldn't bother too much with this. An encrypted password in a config file would need to be decryptable by the person running the tool, anyhow. If users are serious about the private key's protection they will use the certificate store and keep the key in a KSP, which could be a smart card, HSM, or the Microsoft Software Key Storage Provider.

I would have a -Password option and use it if it is specified. If it is not specified, try opening the PKCS12 file without a password. If it fails, prompt for a password and try again with the password from the prompt. If that fails, then you fail for real. You may want to introduce a -Quiet option to ensure build servers do not prompt and hang.

Do we need to add retry mechanism for the timestamper service?

I don't think so? I would however make sure there is a timeout. Perhaps configurable.

Other things...

It would be really nice if there was support for a signing "callback" mechanism. I don't know if this needs to exist in the CLI, but in the library it would be very help for 3rd parties to sign with whatever they want (selfishly I am thinking of Key Vault support). Windows recently introduced this for Authenticode with SignerSignEx3. The gist would be, instead of signing with a certificate, you have a callback that says "Hey, here's a digest. Go sign it and return it". signtool again supports this with /dlib or the other XML switches. It would still use a certificate for embedding in the signature and verifying the signature, by the private signing operation would be a callback.

It would also be nice if, perhaps later, but the more information the better, it was made clear what different exit codes mean. I assume some people will automate this signing process with a CICD tool, and being able to fail the build for certain exit codes would be nice.

@clairernovotny
Copy link

It would be really nice if there was support for a signing "callback" mechanism. I don't know if this needs to exist in the CLI, but in the library it would be very help for 3rd parties to sign with whatever they want (selfishly I am thinking of Key Vault support).

For that callback, it should be async. The signature should essentially be Task<(byte[], X509Certificate2)> SignCallback(byte[] digest, HashAlgorithm algorithm)

Idea being that the public certificate (and its chain) come in externally to the library. It should not assume X509Store at all.

@vcsjones
Copy link

vcsjones commented Oct 3, 2017

Task<(byte[], X509Certificate2)> SignCallback(byte[] digest, HashAlgorithm algorithm)

I would suggest that last parameter be a HashAlgorithmName. The other thing that it needs to take in is which certificate the user selected, or some kind of configuration. Therefore I would propose (in pseudo code):

public class SignResult
{
    public byte[] Signature {get;}
    public X509Certificate2 Certificate { get; }
    public X509Certificate2Collection AdditionalCertificates { get; }
}

Task<SignResult> SignCallback(byte[] digest, HashAlgorithmName algorithmName, string certificateIdentifier);

The certificateIdentifier parameter would be an opaque value that the consumer of the signer API would choose. It could be the DN of the certificate, a thumbprint, etc. The public signing API would accept the string as an identifier, or at least as one of the overloads for doing a signing process.

AdditionalCertificates would be a collection of zero or more certificates that will support the verification process of building a chain back to a root.

@jariq
Copy link

jariq commented Oct 3, 2017

@vcsjones I think SignCallback should be split into two separate callbacks. One would return Certificate and AdditionalCertificates and the other one would return Signature. The reason is that the signing process usually needs to have certificate content (to construct ESSCertID and similar attributes) long before it computes the final hash that should be signed with private key.

Therefore I would propose following modification of your pseudo code:

public class GetSigningCertResult
{
    public X509Certificate2 Certificate { get; }
    public X509Certificate2Collection AdditionalCertificates { get; }
}

Task<GetSigningCertResult> GetSigningCertCallback(string certificateIdentifier);

public class SignHashResult
{
    public byte[] Signature {get;}
}

Task<SignHashResult> SignHashCallback(byte[] digest, HashAlgorithmName algorithmName, string certificateIdentifier);

Both could be wrapped into an interface:

public interface IExternalSigner
{
    Task<GetSigningCertResult> GetSigningCertCallback(string certificateIdentifier);
    Task<SignHashResult> SignHashCallback(byte[] digest, HashAlgorithmName algorithmName, string certificateIdentifier);
}

@mishra14 mishra14 modified the milestones: 4.5, 4.5 - Oct3-Oct21 Oct 3, 2017
@mishra14
Copy link
Contributor Author

mishra14 commented Oct 4, 2017

@jariq @vcsjones Thanks for bringing this up. I have removed the param -SigningAlgorithm. I will go through the rest of the comments as well.

@jariq
Copy link

jariq commented Oct 5, 2017

I am not quite sure that "certificate store" and "CNG/CAPI" providers need to be treated as a separate certificate sources and I also don't understand what are -CryptographicServiceProvider and -KeyContainer options good for.

In my experience certificate that is present in the store can be linked with the private key managed by CNG or CAPI provider. And since .NET Framework 4.6 I almost never need to know which particular one because new APIs work nicely with both of them. Let me demonstrate that with a code sample that signs and verifies some data with two certificates located in CurrentUser\My certificate store. One is linked with RSA private key managed by CAPI provider and the other one is linked with RSA private key managed by CNG provider (if needed there is also step-by-step guide available describing how to generate those certificates).

Sample demonstrates that SignData and VerifyData methods of System.Security.Cryptography.RSA class can transparently handle both CAPI and CNG keys. The same goes for SignHash and VerifyHash methods. I guess @bartonjs could confirm whether this is the correct way to use those APIs.

using System;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;

namespace CryptoProvidersTest
{
    public static class Program
    {
        private static readonly byte[] _testData = Encoding.ASCII.GetBytes("Hello world!");

        public static void Main(string[] args)
        {
            // Find both CAPI and CNG certificates in CurrentUser\My store
            X509Store x509Store = new X509Store(StoreName.My, StoreLocation.CurrentUser);
            x509Store.Open(OpenFlags.ReadOnly);
            X509Certificate2 capiCert = x509Store.Certificates.Find(X509FindType.FindBySubjectName, "CAPITestCert", false)[0];
            X509Certificate2 cngCert = x509Store.Certificates.Find(X509FindType.FindBySubjectName, "CNGTestCert", false)[0];
            x509Store.Close();

            // Sign and verify some data with both CAPI and CNG certificates using the same code
            foreach (X509Certificate2 cert in new X509Certificate2[] { capiCert, cngCert })
            {
                if (cert.PublicKey.Oid.FriendlyName == "RSA")
                {
                    // Note: cert.GetRSAPrivateKey() is available since net46
                    RSA rsaPrivKey = cert.GetRSAPrivateKey();
                    byte[] signature = rsaPrivKey.SignData(_testData, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);

                    // Note: cert.GetRSAPublicKey() is available since net46
                    RSA rsaPubKey = cert.GetRSAPublicKey();
                    if (!rsaPubKey.VerifyData(_testData, signature, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1))
                        throw new Exception("Signature is invalid");
                }
                else
                {
                    // Note: ECDSA keys can be supported with cert.GetECDsaPrivateKey() and cert.GetECDsaPublicKey() available since net461
                    throw new Exception("Unsupported type of private key");
                }
            }
        }
    }
}

My questions:

  1. Do "certificate store" and "CNG/CAPI" providers need to be treated as a separate certificate sources when certificate related APIs work with all of them transparently?
  2. @mishra14 could you please describe in more details any scenario that uses -CryptographicServiceProvider and -KeyContainer options?

@mishra14
Copy link
Contributor Author

mishra14 commented Oct 9, 2017

@jariq Thanks for the detailed feedback. I agree, certificate store and CNG/CAPI do not need to be a separate source. -CryptographicServiceProvider and -KeyContainer options can be used to supply the private key in case the certificate does not contain the private key.

@mishra14
Copy link
Contributor Author

mishra14 commented Oct 9, 2017

@emgarten can you please look at the API requests/feedback?

@jariq
Copy link

jariq commented Oct 11, 2017

-CryptographicServiceProvider and -KeyContainer options can be used to supply the private key in case the certificate does not contain the private key.

@mishra14 in my experience all you need to do in such case is to import certificate into the store and run certutil -repairstore command. Certutil repairs/creates association between the key and certificate and as a result they can be used via -CertificateStoreName, -CertificateStoreLocation, CertificateSubjectName and -CertificateFingerprint options.

Feel free to correct me if I am wrong but in my opinion -CryptographicServiceProvider and -KeyContainer options are just a substitute for certutil -repairstore command and therefore are not needed at all.

@jariq
Copy link

jariq commented Oct 11, 2017

-CertificateFilePath - File path to the certificate to be used while signing the package.

Please provide more details about the expected format of the file specified by -CertificateFilePath option. I guess it is PKCS#12 with signing certificate and corresponding private key (and possibly also other certificates forming certificate chain?).

@mishra14
Copy link
Contributor Author

@jariq Thanks for the feedback.

@mishra14 in my experience all you need to do in such case is to import certificate into the store and run certutil -repairstore command. Certutil repairs/creates association between the key and certificate and as a result they can be used via -CertificateStoreName, -CertificateStoreLocation, CertificateSubjectName and -CertificateFingerprint options.

csp and kc are helpful in org level signing where the private keys are not directly accessible.

Please provide more details about the expected format of the file specified by -CertificateFilePath option. I guess it is PKCS#12 with signing certificate and corresponding private key (and possibly also other certificates forming certificate chain?).

The option is primarily for using PKCS#12 (pfx) files.

@jariq
Copy link

jariq commented Oct 17, 2017

csp and kc are helpful in org level signing where the private keys are not directly accessible.

Unfortunately this does not really provide much more detail to me but I guess I'm OK with -CryptographicServiceProvider and -KeyContainer options being present if anyone find them useful.

@mishra14
Copy link
Contributor Author

Closing this issue as the spec is done.

@mishra14
Copy link
Contributor Author

Folks couple of updates to the spec -

  1. I have removed CryptographicServiceProvider and KeyContainer parameters from the sign command as this was decided to be no longer relevant.
  2. Cleaned the Details about options section about private key to reflect this.

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

7 participants