dotnet-dev-certs: prototype Linux trust support #33279

tmds · 2021-06-04T10:17:34Z

Contributes to #32842.

@javiercn I prototyped something that works on Fedora. Can you give some feedback before I work on it further?

src/Shared/CertificateGeneration/UnixCertificateManager.cs

src/Shared/CertificateGeneration/CertificateStore.cs

src/Shared/CertificateGeneration/CertificateManager.cs

tmds · 2021-06-04T10:24:59Z

src/Shared/CertificateGeneration/UnixCertificateManager.cs

-        public override bool IsTrusted(X509Certificate2 certificate) => false;
+        public override bool IsTrusted(X509Certificate2 certificate)
+        {
+            // Return true when all stores trust the cert.


This could be extended to indicate 'partial': some locations trust the certificate, while others do not.
Or it can return true when at least one trust the certificate.

tmds · 2021-06-04T10:26:02Z

src/Shared/CertificateGeneration/UnixCertificateManager.cs

+            {
+                reporter.Output($"Installing into {store.StoreName}");
+                store.TryInstallCertificate(CertificateStoreKey, pemFile, reporter, isInteractive);
+                // TODO: handle failure.


Is this supposed to throw?
Probably we want to try all locations, even if there is one that throws.

The general pattern that we follow here is to capture three things:

Begin operation

Success

Failure

I think its ok to continue trying to setup trust in other stores even if one of them fails.

For reference, I had issues with the nssdb store being corrupted in the past when trying to setup the cert.

The logging doesn't show up unless the user increases verbosity, right?

What should the user see when the install was partial, and when it failed completely?
Should something be printed using the IReporter? Should these have different exit codes?

javiercn · 2021-06-04T14:57:00Z

src/Shared/CertificateGeneration/NssCertificateDatabase.cs

+                }
+            }
+            return false;
+        }


Instead of doing this, can we make the certificate name be aspnetcore-https-{sha256}? with two goals in mind:

We can exactly identify each certificate by querying for the name.

We can clean all certificates by listing those that start with aspnetcore-https.

can we make the certificate name be aspnetcore-https-{sha256}

I've used aspnet-{Thumbprint}. Is that ok?

We can clean all certificates by listing those that start with aspnetcore-https.

We don't want to delete other user's certificates.
Now, it deletes specific certificates.

aspnetcore-{thumbprint} sounds good.

We don't want to delete other user's certificates.

dotnet dev-certs https --clean needs to remove all certificates that we have created. I'm fine if we claim that namespace and remove any certificate that has been saved as aspnet-{thumbprint}. If you are not happy with that, then we need to load the certificates and check for the presence of the specific OID our tool sets up on the certificate. There is a function in certificate manager to check if the certificate is an asp.net core https certificate (And if someone has decided to create their own certificate and use our OID (which we explicitly discourage people from) then tough luck for them)

Should dotnet dev-certs https --clean remove certificates that were generated and trusted by other users?

No, we should only clean the certificates we create (that's why we use prefix/thumbprint so we can tell which are ours)

src/Tools/FirstRunCertGenerator/src/Microsoft.AspNetCore.DeveloperCertificates.XPlat.csproj

src/Tools/dotnet-dev-certs/src/Program.cs

src/Shared/CertificateGeneration/PemCertificateFile.cs

src/Shared/CertificateGeneration/ProcessRunResult.cs

javiercn · 2021-06-04T15:15:36Z

src/Shared/CertificateGeneration/ProcessRunner.cs

+        public static bool HasProgram(string program)
+        {
+            string path;
+            string? pathEnvVar = Environment.GetEnvironmentVariable("PATH");
+            if (pathEnvVar != null)
+            {
+                string[] pathItems = pathEnvVar.Split(':', StringSplitOptions.RemoveEmptyEntries);
+                foreach (var pathItem in pathItems)
+                {
+                    path = Path.Combine(pathItem, program);
+                    if (File.Exists(path))
+                    {
+                        return true;
+                    }
+                }
+            }
+            return false;
+        }


We should not do this. I think its best to rely on which to see if a tool is available.

We're doing the same thing as .NET does when it looks at PATH to know what file Process should execute: https://github.com/dotnet/runtime/blob/8709d595f7105cbf73774ec762266bc56f99cf04/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs#L736-L754.

I've encountered minimal OS installs and container images where which was not installed.

javiercn · 2021-06-04T15:19:54Z

src/Shared/CertificateGeneration/CertificateFolderStore.cs

+        private void DeleteFile(string path, bool isInteractive)
+        {
+            if (!File.Exists(path))
+            {
+                return;
+            }
+
+            if (Elevate)
+            {
+                ProcessRunner.Run(new()
+                {
+                    Command = { "rm", path },
+                    Elevate = true,
+                    IsInteractive = isInteractive
+                });
+            }
+            else
+            {
+                File.Delete(path);
+            }
+        }
+
+        private void CopyFile(string sourceFileName, string destFileName, bool isInteractive)
+        {
+            DeleteFile(destFileName, isInteractive);
+
+            if (Elevate)
+            {
+                ProcessRunner.Run(new()
+                {
+                    Command = { "cp", sourceFileName, destFileName },
+                    Elevate = true,
+                    IsInteractive = isInteractive
+                });
+            }
+            else
+            {
+                File.Copy(sourceFileName, destFileName);
+            }
+        }
+    }


We should just use C# for these operations.

By the time we get here we should already make sure we are running with admin rights if we need to.

By the time we get here we should already make sure we are running with admin rights if we need to.

This is similar to the mac implementation which uses sudo for performing specific operations that need root.
Do you want to do something different for Linux? How/when should it elevate?

javiercn · 2021-06-04T15:36:08Z

src/Tools/dotnet-dev-certs/src/Program.cs

                if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
                {
-                    reporter.Warn("Trusting the HTTPS development certificate was requested. Trusting the certificate on Linux distributions automatically is not supported. " +
-                        "For instructions on how to manually trust the certificate on your Linux distribution, go to https://aka.ms/dev-certs-trust");
+                    reporter.Warn("Trusting the HTTPS development certificate was requested. If the certificate is not " +
+                        "already trusted we will run commands using 'sudo'." +
+                        Environment.NewLine + "This might prompt you for your password.");


This check, as well as the one below needs to be based on the combination of distro and version. We don't want to "onboard" distros/versions by default without having tested them explicitly and created the infrastructure to prevent regressions.

We should keep the message for unsupported distro/versions.

I've added the message back, it gets printed when there is no way of trusting certificates on the platform.

javiercn · 2021-06-04T15:39:39Z

src/Shared/CertificateGeneration/UnixCertificateManager.cs

+        private List<CertificateStore> _certificateStores;
+
+        private List<CertificateStore> CertificateStores
+            => _certificateStores ??= CertificateStoreFinder.FindCertificateStores();


Can we be explicit here instead of dealing with a collection? This just makes the code harder to follow and there are no real benefits for abstracting this, since the code is not extensible and its not going to be prone to change.

I think it's beneficial to have this.
It allows to write unit tests that work against a CertificateStore.
And having the collection makes explicit how something like IsTrusted is related to the different locations that can trust the certificate.

It allows to write unit tests that work against a CertificateStore.

We only use integration/manual tests for these types of things, since automation for these things is problematic on our CIs.

And having the collection makes explicit how something like IsTrusted is related to the different locations that can trust the certificate.

I don't feel incredibly strong about this, however the reason that I don't like having a collection is that we are unlikely to add more stores than the ones we have now (at least frequently) and that it makes it harder to debug things or emit specific logs for a given store.

tmds · 2021-06-08T07:28:50Z

@javiercn from #32842

Once all the software is installed on the given distro, --trust, --check, --clean must work on 3 areas:
Firefox: Can be configured via an enterprise policy or via certutil, we need to determine the best way to do this. The user profile is in `~/,mozilla and we can find the default there.
Edge/Chrome: Can be configured via certutil at ~/.pki/certificates
dotnet runtime: Can be configured by dropping the certificate in the openssl folder.

How should the --trust, --check --trust, --clean commands report progress / partial success for these areas?

javiercn · 2021-06-09T15:10:23Z

src/Shared/CertificateGeneration/Paths.cs

+        public static string GetUserTempFile(string suffix = ".tmp")
+        {
+            string directory = Paths.XdgRuntimeDir ?? Home ?? // Should be user folders.
+                               Path.GetTempPath();            // Probably global on Linux.
+
+            return Path.Combine(directory, Guid.NewGuid() + suffix);
+        }
+
+        private static string? XdgRuntimeDir => Environment.GetEnvironmentVariable("XDG_RUNTIME_DIR");


Why does https://docs.microsoft.com/en-us/dotnet/api/system.io.path.gettempfilename?view=net-5.0 is not enough?

It's not secure to use a word writable location like /tmp.
By the time you use the file, someone can replace it.

I was wrong about this. Path.GetTempFileName is secure enough. It's created rw only for the current user, and only that user can remove the file.

javiercn · 2021-06-09T16:03:23Z

src/Shared/CertificateGeneration/UnixCertificateManager.cs

+            foreach (var store in CertificateStores)
+            {
+                store.DeleteCertificate(certificate);
+            }


Each operation likely needs to go in a try..catch block and emit logs to indicate success or failure. If something goes wrong we want to know where we failed.

Here is an example for reference:

aspnetcore/src/Shared/CertificateGeneration/MacOSCertificateManager.cs

Lines 190 to 227 in 43241a3

private static void RemoveCertificateTrustRule(X509Certificate2 certificate)

{

Log.MacOSRemoveCertificateTrustRuleStart(GetDescription(certificate));

var certificatePath = Path.GetTempFileName();

try

{

var certBytes = certificate.Export(X509ContentType.Cert);

File.WriteAllBytes(certificatePath, certBytes);

var processInfo = new ProcessStartInfo(

MacOSRemoveCertificateTrustCommandLine,

string.Format(

CultureInfo.InvariantCulture,

MacOSRemoveCertificateTrustCommandLineArgumentsFormat,

certificatePath

));

using var process = Process.Start(processInfo);

process!.WaitForExit();

if (process.ExitCode != 0)

{

Log.MacOSRemoveCertificateTrustRuleError(process.ExitCode);

}

Log.MacOSRemoveCertificateTrustRuleEnd();

}

finally

{

try

{

if (File.Exists(certificatePath))

{

File.Delete(certificatePath);

}

}

catch

{

// We don't care about failing to do clean-up on a temp file.

}

}

}

javiercn · 2021-06-09T17:13:41Z

src/Tools/dotnet-dev-certs/src/Program.cs

            if (trust?.HasValue() == true)
            {
-                if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
+                if (!manager.SupportsTrust)


We want to make this check based on a static list of supported Distro/Version pairs. I don't want us to say we support trust if we don't support it at all levels (Firefox, Chrome/Debian, System); and as it is right now, this will always return true.

An alternative would be to make sure we print a warning when we detect a condition where we can't "guarantee" that we support trust, for example, if Edge/Chrome is not installed or we don't know how to trust the certificate for a given distro/version pair we can still support it even if partially and print a warning explaining this to the user.

Its a priority for us that we are VERY VERY clear on what's supported here and what not. Otherwise the cost of supporting this skyrockets, so if something is not supported we should produce a warning if we want to try other supported options.

We want to make this check based on a static list of supported Distro/Version pairs.

Such conditions have been problematic in the past. For example, something works on a supported Fedora 33 version. The user upgrades to Fedora 34 and suddenly things stop working.

An alternative would be to make sure we print a warning when we detect a condition where we can't "guarantee" that we support trust, for example, if Edge/Chrome is not installed or we don't know how to trust the certificate for a given distro/version pair we can still support it even if partially and print a warning explaining this to the user.

Yes, this is preferable imo.

It's valuable to make the browsers trust the cert, even if it is not know how to install for System.
Also the latter requires root, which the user may not have.
Trusting some is better than doing all or nothing.

Should this warning be generated using the EventLog?

javiercn

Thanks for doing this!

It looks good so far.

A few high level notes:

Logging/Exception handling is supper important.
- We need to log when we are about to run a command and whether it succeeded or failed.
We need to either commit to supporting trust on all places on a given distro/version (Firefox, Edge/Chrome, System) or not support it at all.
- Alternatively I'm ok if we offer "partial" support with a warning indicating what we don't support on a given distro/version.
We need to make sure the required tools are installed before we try to do anything that involves trust
- certutil
- openssl (high enough version)

tmds · 2021-06-22T18:31:03Z

@javiercn can you give some inputs on my questions? I'd like to continue working on this next week.

tmds · 2021-07-02T10:11:15Z

@javiercn can you give some input?

My thought are that the tool should not restrict itself based on a limited set of OSes and it should do what it can do, report back to the user, and exit with an appropriate code.

This reporting may be through EventLog, though probably then it will stay hidden unless the user increases verbosity?

For installing the certificate to the system store, administrator privileges are required. Either we run some commands with sudo from the implementation, or the user needs to run the dev-certs command with sudo. The first is what is currently done with the mac implementation. In the latter case, we may need to look at the SUDO_UID/USER envvars to know on behalf of what user we're performing this operation.

javiercn · 2021-07-02T12:19:32Z

@tmds I'll try to get to this today before I leave.

I'm currently focused on other work that has me completely booked.

dougbu

New files need MIT license headers; some are missing headers entirely

pranavkm · 2022-02-11T18:41:33Z

@tmds looks like we dropped the ball on this and this PR has gone stale with merge conflicts. I'm going to close it for now - could you reach out in #32842 to confirm if someone has the availability to work with you on this change since it's fairly involved? Thank!

linux-dev-certs: prototype Linux trust support

bb1364d

tmds requested a review from Pilchie as a code owner June 4, 2021 10:17

ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 4, 2021

tmds commented Jun 4, 2021

View reviewed changes

src/Shared/CertificateGeneration/UnixCertificateManager.cs Outdated Show resolved Hide resolved

tmds commented Jun 4, 2021

View reviewed changes

src/Shared/CertificateGeneration/CertificateStore.cs Show resolved Hide resolved

tmds commented Jun 4, 2021

View reviewed changes

src/Shared/CertificateGeneration/CertificateManager.cs Outdated Show resolved Hide resolved

tmds commented Jun 4, 2021

View reviewed changes

tmds changed the title ~~linux-dev-certs: prototype Linux trust support~~ dotnet-dev-certs: prototype Linux trust support Jun 4, 2021