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

@bartonjs estimates 3 days remaining

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 ...

@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.

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.

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

@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.

@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.

@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));
        }
    }
    ...
}

@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.

@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?

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.

@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.

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?

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 [add the root to CustomTrustStore and the intermediate(s) to ExtraStore]

Yep.

Is [rejecting intermediates] worth enforcing?

The default for the custom trust chain builder on Windows is to ignore non-self-issued (I don't remember if they check the signature during the filter or not). They have a flag to opt in to doing "custom anchors" vs "custom roots".

So, assuming we start with "custom roots", on macOS and Linux we'd need to copy the collection. Saying this, I guess we ultimately need to copy it anyways (into the respective native formats) so with sufficient tests we can make sure that CustomRootTrust behaves appropriately (aka doesn't trust intermediates -- that'd be for a CustomRootOrIntermediateTrust that could be done at the same time, or could be deferred).

The thing to double check when doing it is that none of the systems end up with "I filtered everything out" to mean "just use system trust". But that's achievable with tests.

So... I guess I'm comfortable with removing that prefilter and just ensuring that there are tests for

• TrustMode == CustomRootTrust, CustomTrustStore contains an appropriate intermediate, ExtraStore contains the root, UntrustedRoot is reported.
• TrustMode == CustomRootTrust, CustomTrustStore contains an appropriate intermediate, the system trust list contains the root. Chain builds completely, UntrustedRoot is reported.

OK, I think I like this API surface - it's simple enough.

I'm less familiar with OpenSSL's APIs but this should be achievable on macOS using SecTrustSetAnchorCertificates and SecTrustSetAnchorCertificatesOnly that works on SecTrustRef.

I'm less familiar with OpenSSL's APIs[...]

https://github.com/dotnet/corefx/blob/bf0fb29fb55cf4c3d9063c78e8f72fb9cd4d041b/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs#L94

I've already scoped our shim down to "here's the STACK_OF(X509*) representing the trust list", so just requires using a different value thereabouts.

One other thought - should the enum be [Flags]?

Maybe Not for v1 of the implementation - but there may be a time where you want to specify additional roots to the other existing trust stores, not exclusive roots. OpenSSL (as you described it) and MacOS support this, though it seems like Win32 doesn't?

Edit to clarify:

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

Hm. If Win32 can't do that, I guess not then - was trying to be forward thinking about the enum's values though.

though it seems like Win32 doesn't [easily support "in addition to"]

As far as I know, that's correct. While we could enumerate LocalMachine\Root, LocalMachine\ThirdPartyRoot, and CurrentUser\Root manually to add to the collection, it'd miss the WU backchannel for automatic trust.... and maybe other stuff that we don't accurately model in the Linux and macOS chains (because I don't know about it).

The best answer would be "try explicit, if it failed, return system"... but a user who wanted could write that themselves. If it became a common thing we could pull it in to a new enum value (SystemOrCustomRootTrust) (or two, (SystemOrCustomRootOrIntermediateTrust)).

• TrustMode == CustomRootTrust, CustomTrustStore contains an appropriate intermediate, ExtraStore contains the root, UntrustedRoot is reported.
• TrustMode == CustomRootTrust, CustomTrustStore contains an appropriate intermediate, the system trust list contains the root. Chain builds completely, UntrustedRoot is reported.

For the second point here are we expecting the build to succeed? I'm just completing the Windows implementation and if we are replacing the system trust with CustomRootTrust shouldn't build return a partial chain?

I'd expect either a) the system trust stores are used as part of chain population (without trust assignment) or b) AIA-follow would kick in and it would find the certificate anyways. Either way, my expectation is that it would give UntrustedRoot, not PartialChain.

If I'm wrong, that's OK, we'd just want to see what macOS does, and as long as that matches Windows we can make the Linux version match.

Video

Looks good as proposed.

We're locking down 3.0 fairly soon. @zwread if you have this ready to go I think we might still be able to take it, but I'm pretty sure that past 3.0-preview7 the answer is that it's in the next version.

(I marked the issue as Future because it's not something we "need" in 3.0, though it would be fantastic to have)