dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.06k stars 4.69k forks source link

WindowsCryptographicException when using symlink as X509Certificate source #27826

Open NatMarchand opened 5 years ago

NatMarchand commented 5 years ago

Hi there! I'd like to use a symlink for constructing my X509Certificate. Currently I'm encoutering a Internal.Cryptography.CryptoThrowHelper+WindowsCryptographicException: Unspecified error with the following code :

var file = "C:\\ProgramData\\Docker\\secrets\\link.pfx"; // symlink to a pfx file

var cert = new X509Certificate2(File.ReadAllBytes(file)); // <-- works well
Console.WriteLine(cert.ToString());

var cert2 = new X509Certificate2(file); // <-- exception thrown here
Console.WriteLine(cert2.ToString()); 

I know that this issue is already known (although I can't find the issue in this repo) as @bartonjs which seems to be responsible of the System.Security area already replied about it on stackoverflow (see here)

The work around suggested work BUT let me show you a use case that become much more complicated :

When working with AspNet Core using Kestrel in a Windows container orchestrated by Docker Swarm, I would love to use secrets for my SSL certificates (SSL Offloading on a proxy is not an option in my case). Unfortunately, Docker secrets are symbolic links. Therefore, to make it work I have to go from just declaring the secret and an environment variable (KestrelCertificatesDefault__Path) to tuning Kestrel and load manually the certificate.

Do you plan to fix this ? My guess is, that's coming from CryptQueryObject in crypt32.

Just in case it's needed :

C:\ProgramData\Docker\secrets>dotnet --info
SDK .NET Core (reflétant tous les global.json) :
 Version:   2.1.500-preview-009335
 Commit:    f73e21a7d8

Environnement d'exécution :
 OS Name:     Windows
 OS Version:  10.0.17134
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.1.500-preview-009335\

Host (useful for support):
  Version: 2.1.5
  Commit:  290303f510

and

Internal.Cryptography.CryptoThrowHelper+WindowsCryptographicException: Unspecified error
   at Internal.Cryptography.Pal.CertificatePal.FromBlobOrFile(Byte[] rawData, String fileName, SafePasswordHandle password, X509KeyStorageFlags keyStorageFlags)
   at System.Security.Cryptography.X509Certificates.X509Certificate..ctor(String fileName, String password, X509KeyStorageFlags keyStorageFlags)
   at System.Security.Cryptography.X509Certificates.X509Certificate2..ctor(String fileName, String password)
bartonjs commented 5 years ago

I know that this issue is already known (although I can't find the issue in this repo)

It's not a .NET (Framework or Core) issue, it's a Windows issue (as you surmised, we get an error out of CryptQueryObject).

Do you plan to fix this ?

There are currently no plans to address it, no. There's an amount of guesswork in recovery, which isn't clear that we would want in all cases.

@natemcmaster Would it make sense for Kestrel's "cert from file" path to do something like

try
{
    return new X509Certificate2(path);
}
catch (CryptographicException)
{
    if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
    {
        try
        {
            if ((File.GetAttributes(path) & FileSystemAttributes.ReparsePoint) != 0)
            {
                byte[] data = File.ReadAllBytes(path);
                return new X509Certificate2(data);
            }
         }
         catch
         {
         }
    }

    throw;
}

The difference in Kestrel's code from the cert PAL is you know that someone's doing it as part of process startup, and no "wait, are you trying to trick me?" questions really arise.

natemcmaster commented 5 years ago

@Tratcher @blowdart thoughts on adding this to Kestrel options binding? ☝️☝️

blowdart commented 5 years ago

Oh bad docker, naughty docker. Yea, seems ok though.

Tratcher commented 5 years ago

It's plausible, though it seems like it belongs in a lower level than Kestrel.

bartonjs commented 5 years ago

@Tratcher My thinking for putting it in Kestrel is that you have the context to know that this is app startup; so there's probably no trickery around "ooh, I can get you to File.ReadAllBytes a 1.999GB file". I can't come up with a sane restriction (off the top of my head) for making that as a "generically, at any point in a runtime operation" change in our layer.

At the Windows layer they wouldn't need the intermediate copy, since they can still just pull from the file descriptor.

So both above us (with context) and below us (due to a different access model) can do this sensibly, but it's hard for my layer.

Tratcher commented 5 years ago

I can understand you don't want to change the default behavior but there's room for an intermediate API like X509Certificate2.LoadCertWithSimlinks(string path) that kestrel could call. There's no reason for that logic to be implemented directly in Kestrel.

blowdart commented 5 years ago

Or a flag on load from file, "FollowSymLinks"?

NatMarchand commented 5 years ago

For people stuck on this issue (maybe I'm not alone trying to do this?), here is a workaround : Instead of using the path of the secret such as "C:\\ProgramData\\Docker\\secrets\\<mySecretName>", it is possible to use the "internal" path "C:\\ProgramData\\Docker\\internal\\secrets\\<mySecretId>" However, as explained in the doc here, it shouldn't be relied upon. Use with care.

erdtsieck commented 4 years ago

Another workaround is doing a copy at startup.

# The copy is done, because wildcard_certificate.pfx is put into the container using docker secrets, which makes it a symlink. 
# Reading a certificate as a symlink is not supported at this moment: https://stackoverflow.com/q/43955181/1608705
# After doing a copy, the copied version is not a symlink anymore.
ENTRYPOINT (IF EXIST "c:\certificates\wildcard_certificate.pfx" (copy c:\certificates\wildcard_certificate.pfx c:\app\wildcard_certificate.pfx)) && dotnet webapplication.dll

My application runs in the "c:\app" folder and I put my "to be copied" certificates in "c:\certificates". At startup the certificate is copied to "c:\app", which my environment variables point to.

version: "3.7"
services:
  webapplication:
    image: ({CONTAINER_REGISTRY})/webapplication:({LABEL})
    environment:
      - ASPNETCORE_URLS=https://+;http://+
      - ASPNETCORE_HTTPS_PORT=443
      - ASPNETCORE_Kestrel__Certificates__Default__Path=wildcard_certificate.pfx
    secrets:
      - source: config_secrets
        target: C:/app/appsettings.json
      - source: wildcard_certificate_pfx
        target: c:\certificates\wildcard_certificate.pfx
jhudsoncedaron commented 3 years ago

Hit this in prod. Bug in Windows. It seems the only possible way of getting the Windows Crypto teams to address their bugs is to actually be willing to switch to OpenSSL if they don't.

vcsjones commented 3 years ago

@jhudsoncedaron would it be possible for you to modify the code to read the contents of the file into a byte[] first? Such as in the original issue:

using var cert = new X509Certificate2(File.ReadAllBytes(file));

Or is this happening in a higher level, or otherwise unmodifiable, API for you?

jhudsoncedaron commented 3 years ago

@vcsjones : Haven't you had the crashes resulting from reading the private key into a byte[] leak disk yet? We have. That writes a temp file in native code and maps it. The temp file survives if the process crashes. Also, %TEMP% may be unwritable on site.

Setting Epheremal key stops it from writing to disk, but that too is busted due to a different bug in the Windows Crypto API.

jhudsoncedaron commented 3 years ago

Apologies. new X509Certificate(byte[]) isn't the API affected by the leaks. new X509Certificate(byte[], SecureString) is. So some uses can use the workaround without issue and some can't.

vcsjones commented 3 years ago

@jhudsoncedaron

You should be able to use new X509Certificate2(byte[], password, X509KeyStorageFlags.EphemeralKeySet) to avoid persisting the keys to disk and keeping them entirely in memory.

jhudsoncedaron commented 3 years ago

@vcsjones : https://github.com/dotnet/runtime/issues/23749