Allow X509Chain to replace the root trust list for a single call

[bartonjs]

We should add members on the ChainPolicy to allow a single call to X509Chain.Build to consider an X509Certificate2Collection of certs as a replacement of the system/user default certificate trust.

Win32: CERT_CHAIN_ENGINE_CONFIG looks like it has hExclusiveRoot for "replace".

Unix/OpenSSL: We just need to replace the root trust list when deciding whether or not to set the UnknownRoot flag.

Apple/Security.framework: The SecTrust APIs have options to replace the anchors list for a single request.

API requirements:

• A way of providing the replacement root certificate trust collection, with a clear mode of if the request is using system trust or custom trust.

(edited to remove the option of "in addition to" the system trust due to increased complexity and less OS support)

namespace System.Security.Cryptography.X509Certificates
{
    public enum X509ChainTrustMode
    {
        System,
        CustomRootTrust,
    }

    public partial class X509ChainPolicy
    {
        public X509ChainTrustMode TrustMode { get; set; }
        public X509Certificate2Collection CustomTrustStore { get; }
    }
}

[vcsjones]

This would be very handy for checking a certificate is pinned against a private root.

[danmoseley]

@bartonjs estimates 3 days remaining

[ShutterQuick]

How elaborate does the interface for this need to be?
Could one just follow the pattern of ExtraStore and add X509Certificate2Collection RootStore on X509ChainPolicy?
If it's null, great, use the default store, if not use the provided one.

Looks like the implementation of this feature can be completed pretty easily once the interface this is hammered down ...

[bartonjs]

@ShutterQuick Well, what does RootStore mean?

1. Everything in this collection is a trust anchor.
2. Only the things in this collection which are self-issued/self-signed are trust anchors.

If (2) (which is the better match for "Root"), does it throw for non-self-issued, or just ignore them? (macOS and OpenSSL are mostly designed as (1); Windows has an option, but defaults to (2))

A collection property should be non-null to allow easy addition; which means that clearing the collection means "revert to default trust" instead of "trust nothing", unless there's a secondary "I want to use customized trust" setting somewhere... but now there's an error state of not setting that option but putting things in the collection 😄.

If we use the emptiness of the collection as customized trust, but we ignore non-self-issued certificates,
is the emptiness decides before, or after? What if a null or disposed certificate sneaks in?

So it's really a question of how to add in a model for this new notion where there are no surprises.

[ShutterQuick]

Thanks for your answer, @bartonjs

You raise some excellent points, and of course you're right, my suggestion of null to determine if the store should be active or not is obviously ridiculous.

I agree that it might make more sense to allow adding trust anchors rather than roots, but as long as Windows 7 is supported that doesn't seem feasible.
The CERT_CHAIN_EXCLUSIVE_ENABLE_CA_FLAG, which is needed to enable non-self-signed CAs as trust anchors, was added first in Windows 8/2012.
https://docs.microsoft.com/en-us/windows/desktop/api/wincrypt/ns-wincrypt-_cert_chain_engine_config

By controlling the behaviour of the new cert collection with a flag, it might be possible to enable that behaviour easily in the future when W7 support is dropped. Or already now on platforms that support it.

E.g. something like this:

public class X509ChainPolicy
{
  ...
  X509Certificate2Collection CustomTrustStore { get; }
  X509CustomTrustStoreMode CustomTrustStoreMode { get; set; }
  ...
}

...

[Flags]
public enum X509CustomTrustStoreMode : byte
{
  Supplement = 0x00,
  Replace = 0x01,
  // AllowIntermediate = 0x80
}

The supplement mode looks like it would be fairly trivial to implement on both Windows, OSX and OpenSSL, and it seems like it could be a fitting default mode, as Supplement with no certificates added to the collection is the same behaviour as today.
To simplify and more easily allow future changes I suggest to just ignore non-appropriate certificates to the store silently.

Of course there's the question how useful a supplement mode would be. I'm struggling a little coming up with use cases where I'd want both my own anchors and whatever is in the system store. It might be that Disabled would be a better option than supplement, even though that feels a bit clunky as well with changes to the collection not doing anything at this point.

Lastly, I realize that due to my inexperience in these matters, my input might mostly be noise.
I hope, if this is the case, you'll let me know and I'll stop polluting the issue.
That said, this is a feature I really want to see, and I'd be more than willing to put some time down towards implementing, but I will likely require some hand-holding.

[zwread]

I am going to start working on this feature. Let me know if anyone else has started it.

[vcsjones]

@zwread my understanding of this issue is that new public APIs are needed in order to make this feature work (hence the api-suggestion tag). Pull requests for new APIs that have not gone through the API review process are discouraged.

This issue does not have the api-approved tag, so there is no approved API surface for this, yet.

[bartonjs]

@zwread If you want to experiment and try to come up with an API surface that feels like it's hard to use incorrectly (which probably also means describing the exception model), that's great. But, as @vcsjones said, since there hasn't been an API proposal there hasn't been an API review, so jumping straight to a PR would be a bit of a faux pas.

[zwread]

@bartonjs and @vcsjones, thank you for the information, that would have been unfortunate. I will definitely start off with coming up with an API surface then.

My initial plan was similar to what @ShutterQuick mentioned in his last post. With a trust mode that would either allow the default root or a custom collection of certificates to replace it. The System mode would function exactly as it does now. While Custom would allow root certificate(s) to be added and would replace the default root store.

public class X509ChainPolicy
{
    ...
    X509CertificateTrustStoreMode _certificateTrustMode;
    X509Certificate2Collection _customTrustCollection;

    ...

    public X509CertificateTrustStoreMode CertificateTrustMode
    {
        get
	{
	    return _certificateTrustMode;
	}
	set
	{
	    if (value < X509CertificateTrustMode.Root || value > X509CertificateTrustMode.Custom)
	        throw new ArgumentException(SR.Format(SR.Arg_EnumIllegalVal, nameof(value)));
	    _certitificateTrustMode = value;
        }
    }

    ...

    public X509Certificate2Collection CustomTrustCollection => _customTrustCollection ?? (_customTrustCollection = new X509Certificate2Collection());
    ...
}

public enum X509CertificateTrustMode
{
    System = 0,
    Custom = 1,
}

In X509Chain the CertificateTrustMode would be checked if Custom was set. If it is, CustomTrustCollection must have at least one valid(self signed certificate). If there are no valid certificates in the collection an exception is thrown.

public class X509Chain
{
    ...
    internal void CheckCertificateTrustMode()
    {
        if(_chainPolicy.CertificateTrustMode == X509CertificateTrustMode.Custom)
        {
	    foreach(X509Certificate2 certificate in _chainPolicy.CustomTrustCollection)
	    {
                if(certificate.IssuerName.RawData != certificate.SubjectName.RawData)
	            _chainPolicy.CustomTrustCollection.Remove(certificate);
	    }
	
	    if(_chainPolicy.CustomTrustCollection.Count() < 1)
	        throw new ArgumentException(SR.Cryptography_InvalidCertificateCollection, nameof(_chainPolicy.CustomTrustCollection));
        }
    }
    ...
}

[bartonjs]

@zwread I like that it encompasses

• Do the current thing as a default
• Start with a limitation of self-issued
• Has an expansion point for different policy (e.g. add a new enum member).

Questions it leaves me with:

• Why would calling chain.Build() actually remove things from the collection?
  • I can't think of any BCL API that removes things from configuration state like this. Generally, input objects are validated, not mutated.
• Should SystemTrust throw if the custom trust collection has anything in it at all?
  • The concern here is that if someone is trying to add pinning to a public-trust they might add the cert to the collection but forget to change the mode. If they only test the success path it'll say it was fine, because it had public trust. So did the API shape do them a dis-service?
• Should CustomRootTrust throw if the custom trust collection has non-self-issued certs in it?
  • I'm of two minds. As a pro, it means the collection gets validated and then can get used without a copy. As a con, one can't just switch between CustomRootTrust and CustomIntermediateOrRootTrust. The con is really just a dev-time scenario, since it seems unlikely that an X509ChainPolicy would be altered after the first time it was used in chain building.

[zwread]

@bartonjs,

Thank you for the response, the simplest way I see of resolving these questions is to fix the validation in X509Chain.

public class X509Chain
{
    ...
    internal void CheckCertificateTrustMode()
    {
        if(_chainPolicy.CertificateTrustMode == X509CertificateTrustMode.Custom)
	{
	    foreach(X509Certificate2 certificate in _chainPolicy.CustomTrustCollection)
	    {
	        if(certificate.IssuerName.RawData != certificate.SubjectName.RawData)
		    throw new ArgumentException(SR.Cryptography_InvalidCertificateCollection, nameof(_chainPolicy.CustomTrustCollection));
	    }
	}
	else if(_chainPolicy.CustomTrustCollection.Count() > 0)
	    throw new ArgumentException(SR.Cryptography_InvalidCertificateTrustMode, nameof(_chainPolicy.CustomTrustCollection));
    }
    ...
}

With this the collection is no longer being mutated and an exception is being thrown if the collection contains any non-self-issued certificates, or the mode is set to System and their are certificates in the CustomTrustCollection.

I might be misunderstanding, but could ExtraStore be the collection that holds the custom intermediate certificates and CustomTrustCollection holds custom root certificates. I know right now you can add self-signed certificates to ExtraStore, but maybe we change that?

[bartonjs]

ExtraStore is just an extra collection (store) of certificates to look at when doing chain resolution. It doesn't impart trust, and it doesn't care if the certs in it are valid-vs-expired, end-entity-vs-intermediate-vs-root, et cetera.

So, yeah, something like

namespace System.Security.Cryptography.X509Certificates
{
    public enum X509ChainTrustMode
    {
        System,
        CustomRootTrust,
    }

    public partial class X509ChainPolicy
    {
        public X509ChainTrustMode TrustMode { get; set; }
        public X509Certificate2Collection CustomTrustStore { get; }
    }
}

with a truth table of

• TrustMode == System, CustomTrustStore.Count == 0 => Current behavior
• TrustMode == System, CustomTrustStore.Count != 0 => Exception "Custom trust certificates were provided while in System trust mode."
• TrustMode == CustomRootTrust, CustomTrustStore.Count == 0 => Let it work, nothing is trusted, easy bug to identify and fix (but potentially valuable state to some people).
• TrustMode == CustomRootTrust, CustomTrustStore.Count(x => !x.IssuerName.RawData.SequenceEqual(x.SubjectName.RawData)) != 0 => Exception "One or more certificates in the CustomRootTrust collection was not self-issued, but TrustMode is CustomRootTrust."
• TrustMode == CustomRootTrust, CustomTrustStore.Count(x => x == null || x.Handle == IntPtr.Zero) => Exception of some sort. ("A null or disposed certificate was present in CustomTrustStore."?)
• TrustMode == CustomRootTrust, otherwise => what the feature request is about
• TrustMode == (anything else) => Debug.Fail / new CryptographicException() (ArgumentOutOfRangeException on the property setter)

Tests that I can think of without seeing code:

• All exception paths (other than Debug.Fail ones)
• Build a chain with a fixed / calculated date and RevocationMode=NoCheck in System trust, save the root cert reference. Change policy to CustomRootTrust (empty), fail (UntrustedRoot). Add the root cert to CustomTrustStore, run again, pass. (Proves the pinned trusted CA flow)
• Make up some certificates using CertificateRequest, build the custom chain that says it's OK. (Proves the custom CA flow)
• Build CustomRootTrust chain restricted to some newly crafted (from CertificateRequest) CA, run a should-be-publicly-trusted certificate through it, ensure it comes back UntrustedRoot.

If you (@zwread) want to start work at this stage that's probably reasonable. If there's no objection to the name tweaks I just did, or the overall shape by the end of the day PDT (@vcsjones?) I'll mark it as api-ready-for-review. Since it isn't through the review process yet there's a chance that it'll change, but I'd expect only tweaking the names, if anything.

If the Windows, macOS Security.framework and Unix OpenSSL implementations can all be done with tests and code review signed off by 17 May this can make it in for 3.0. (That would also include having all of the curly braces required by our style rules added back in from the most recent code snippet 😄) It gets exponentially less likely day-by-day after the 17th. Just setting some expectations.

[vcsjones]

@bartonjs It's simple and I like it, I assume ExtraStore would continue to work then? e.g. I have a leaf issued from an intermediate off a custom root. So I would:

1. Put my root in CustomTrustStore.
2. Populate ExtraStore with my intermediates.
3. Build the chain off my leaf.

That is a scenario I care about; I don't want to put my intermediates in CstuomTrustStore because I don't trust them, ultimately, I just want them to help build a path to a hardcoded root, while the extra store we periodically refresh from a remote service.

[vcsjones]

One or more certificates in the CustomRootTrust collection was not self-issued, but TrustMode is CustomRootTrust

Is that something worth enforcing? Does CustomTrustStore need to require the cert to be self signed, or just that it is just a point of being ultimately trusted?