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

certificates added to user CertificateAuthority store are ignored by X509Chain.Build on macOS #48207

Open
Tracked by #64488
wfurt opened this issue Feb 12, 2021 · 6 comments
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented Feb 12, 2021

This is subset of #48017 as the underlying issue seems different.
I basically have fragment like

    using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser))
    {
        // add chain certificate so we can construct chain since there is no way how to pass intermediates directly.
        store.Open(OpenFlags.ReadWrite);
        store.AddRange(clientChain);
        store.Close();
    }
    
    var chain = new X509Chain();
    ....
    bool chainStatus = chain.Build(clientCertificate);

It works on Linux & Windows but it fails consistently on macOS (10.15)
Interestingly, it works if I use StoreName.My instead.

It seems like that store does not have standard OS KeyChain mapping and that is reason why the chain does not build correctly. If that is true, it would be nice if we can feed reference to the keychain or it's collection automaticaly so validation and chain building works consistently across platforms.
In SslStream this primarily impacts cases with client certificates.

@ghost
Copy link

ghost commented Feb 12, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

This is subset of #48017 as the underlying issue seems different.
I basically have fragment like

    using (X509Store store = new X509Store(StoreName.CertificateAuthority, StoreLocation.CurrentUser))
    {
        // add chain certificate so we can construct chain since there is no way how to pass intermediates directly.
        store.Open(OpenFlags.ReadWrite);
        store.AddRange(clientChain);
        store.Close();
    }
    
    var chain = new X509Chain();
    ....
    bool chainStatus = chain.Build(clientCertificate);

It works on Linux & Windows but it fails consistently on macOS (10.15)
Interestingly, it works if I use StoreName.My instead.

It seems like that store does not have standard OS KeyChain mapping and that is reason why the chain does not build correctly. If that is true, it would be nice if we can feed reference to the keychain or it's collection automaticaly so validation and chain building works consistently across platforms.
In SslStream this primarily impacts cases with client certificates.

Author: wfurt
Assignees: -
Labels:

area-System.Security, os-mac-os-x

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 12, 2021
@bartonjs
Copy link
Member

Only My, Root (RO), and Disallowed stores work on macOS. The call to store.Open should be throwing:

public static IStorePal FromSystemStore(string storeName, StoreLocation storeLocation, OpenFlags openFlags)
{
StringComparer ordinalIgnoreCase = StringComparer.OrdinalIgnoreCase;
switch (storeLocation)
{
case StoreLocation.CurrentUser:
if (ordinalIgnoreCase.Equals("My", storeName))
return AppleKeychainStore.OpenDefaultKeychain(openFlags);
if (ordinalIgnoreCase.Equals("Root", storeName))
return AppleTrustStore.OpenStore(StoreName.Root, storeLocation, openFlags);
if (ordinalIgnoreCase.Equals("Disallowed", storeName))
return AppleTrustStore.OpenStore(StoreName.Disallowed, storeLocation, openFlags);
return FromCustomKeychainStore(storeName, openFlags);
case StoreLocation.LocalMachine:
if (ordinalIgnoreCase.Equals("My", storeName))
return AppleKeychainStore.OpenSystemSharedKeychain(openFlags);
if (ordinalIgnoreCase.Equals("Root", storeName))
return AppleTrustStore.OpenStore(StoreName.Root, storeLocation, openFlags);
if (ordinalIgnoreCase.Equals("Disallowed", storeName))
return AppleTrustStore.OpenStore(StoreName.Disallowed, storeLocation, openFlags);
break;
}
if ((openFlags & OpenFlags.OpenExistingOnly) == OpenFlags.OpenExistingOnly)
throw new CryptographicException(SR.Cryptography_X509_StoreNotFound);
string message = SR.Format(
SR.Cryptography_X509_StoreCannotCreate,
storeName,
storeLocation);
throw new CryptographicException(message, new PlatformNotSupportedException(message));
}

@vcsjones
Copy link
Member

@bartonjs For CurrentUser it will fall in to a custom keychain store for things other than My, Root, and Disallowed:

That said, macOS has no notion of a CertificateAuthority store, it's just a keychain with some certificates in it. We don't appear to do anything with this store during chain building, like treat it like ExtraStore, so that's why it's being ignored.

@vcsjones
Copy link
Member

It seems like it was originally not supported for 2.x, and in 3.0 this PR landed: dotnet/corefx#30603 which added support for custom trust stores, likely to support arbitrary store names for X509Store(string storeName, StoreLocation storeLocation). In doing so, CertificateAuthority ended up getting treated as an arbitrary store. Perhaps that was unintentional.

@bartonjs
Copy link
Member

Ah, yep. Missed that the first case block has a fallback return in it.

Seems like the three paths are:

  • Do nothing
  • Block that store.Open call (technically a breaking change)
  • During chain builds read that store and pass the contents as if they were in ExtraStore.

The last one is the most compatible, I suppose. Maybe there's a cheaper way to implement it (e.g. maybe the chain building API supports "and uh, also use this keychain as part of resolving" and we can pass the keychain handle. Or at least pass the keychain handle to our shim API so we can avoid the O(N) P/Invokes in building the array)

@wfurt
Copy link
Member Author

wfurt commented Feb 16, 2021

It feels like last option would give most consistent experience. Since the stores probably don't change too frequently and are probably reasonably small, I'm wondering if we can pre-compute the list so it can be added in O(1).
even with O(N) the cost may be small comparing to the crypto.

@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Jul 3, 2021
@bartonjs bartonjs added this to the 7.0.0 milestone Jul 3, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 12, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 9, 2022
@bartonjs bartonjs modified the milestones: 7.0.0, Future Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants