macOS dev-certs overhaul#42251
Conversation
| if (result == EnsureCertificateResult.ValidCertificatePresent) | ||
| { | ||
| result = EnsureCertificateResult.ExistingHttpsCertificateTrusted; | ||
| } | ||
| else | ||
| { | ||
| result = EnsureCertificateResult.NewHttpsCertificateTrusted; | ||
| } |
There was a problem hiding this comment.
Can we avoid adding new results here? This is a contract between us and the tooling teams, so adding new results changes that contract and might break them.
There was a problem hiding this comment.
@javiercn Ah, I see, good to know. I'll check out how the tooling stuff consumes this today.
I'll also add a note mentioning this contract next to the enum declaration so someone doesn't accidentally break the contract in the future.
There was a problem hiding this comment.
Hmm... can you point me to where this is used by tooling? I can't seem to find it.
cc @vijayrkn
There was a problem hiding this comment.
I am assuming you are referring to this: https://devdiv.visualstudio.com/DevDiv/_git/WebTools?path=/src/ProjectSystem/Web/CommonServices/AspNetCoreDeveloperCertificatesTool.cs&_a=contents&version=GBmain
There was a problem hiding this comment.
Thanks @vijayrkn! Looks like the tooling just calls the dev-certs tool and relies on some of the status codes here which are the exit codes of the tool:
aspnetcore/src/Tools/dotnet-dev-certs/src/Program.cs
Lines 19 to 26 in 36e1456
The EnsureCertificateResult enum changes I made won't change the actual exit codes from the tool, since these turn into "Success" here:
So I think we're safe.
There was a problem hiding this comment.
Adding @BillHiebert in case there are other places where we call into the tool.
There was a problem hiding this comment.
Added a note of caution about modifying the exit codes.
There was a problem hiding this comment.
This also impacts VS 4 Mac and Docker container tools. FYI.
There was a problem hiding this comment.
I see now that this only affects the internal reporting, but that the tool still returns 0 (success) so I think we are good here.
mkArtakMSFT
added
area-commandlinetools
adityamandaleeka
added
the
blog-candidate
|
@adityamandaleeka, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
|
Going to hold off on this PR until the upgrade experience (coming in with some existing state left behind by the older version of the tool) is smoother. Right now you end up with torn state that's annoying to clean up. |
|
After trying a few different approaches, the cleanest way forward seems to be to change the certificate's OID so that .NET 7+ dev certs won't conflict at all with the certs used in 6 and below. Trying to do otherwise causes problems since the old SDK would continue to locate and run trust operations on the newer certs, breaking the improved (fewer prompts, etc.) experience for people using both .NET 6 and .NET 7 SDKs. |
|
|
||
| protected override void PopulateCertificatesFromStore(X509Store store, List<X509Certificate2> certificates) | ||
| { | ||
| bool useDiskStore = store.Name! == StoreName.My.ToString() && store.Location == StoreLocation.CurrentUser; |
|
@adityamandaleeka Changing the OID will break developing .NET 6.0 apps with the 7.0 SDK. |
|
This is true. However, there's no great option here since keeping the OID the same will break things in cases where people switch between multiple SDK versions (e.g. if one of the repos they work in specifies an SDK in a global.json). I discussed this with @DamianEdwards and we decided that the least awful option is to take the hit of changing the OID and print a message when reporter.Warn("NOTE: If your app is targeting a version of .NET earlier than .NET 7, this version of "
+ "the dev-certs tool will not install the correct certificate for your app. Please use the dev-certs "
+ "tool from the .NET 6 SDK or the SDK version that matches your app instead.");I've also scoped the OID change to be macOS only to limit the blast radius of this change. |
|
@adityamandaleeka I'm struggling to understand why changing the OID is necessary at all. Could you please help me understand what didn't work in the older version that changing the OID fixes? |
|
Sure, the short summary is what I mentioned above; keeping the same OID will make switching between SDK versions cause inscrutable problems. Here are some examples: Example 1
Example 2
One of the options considered (and attempted earlier in the PR) involved detecting and clean old state, but we can't go down that path since it would involve adding authentication during steps that occur in various tooling/first-run scenarios and therefore need to be non-interactive. |
|
I'll resolve the conflicts and update the failing test with the new cert. @javiercn any further comments? |
|
@adityamandaleeka Sorry for the delayed response. I'm wondering if we are optimizing for the right scenarios. From what I understand there can be these cases Only .NET 6.0 SDK is installed -> This would continue to work as it does today. .NET 7.0 and .NET 6.0 SDKs are installed. Would it work if the .NET 7.0 SDK does the following?
In general, if you have .NET 7.0 SDK installed, that will be used unless you have a global.json file on your project. In the event of clean with the .NET 6.0 SDK, it will remove the certificate only from the keychain and leave the file behind. .NET 7.0 SDK will clean that up the next time the tool runs. In that way, the only thing ever left behind is the file on disk if you use .NET 6.0 to do the cleaning (which should be rare). The advantage is that the .NET 7 SDK don't break your ability to develop .NET 6.0 apps, which is something that newer SDKs must support. |
@javiercn I think the issue is that exporting the cert requires user elevation/confirmation and as such can't be done as part of the existing first-run experience that we have today (which typically just creates the certs in the key ring). This is ultimately what led us to this approach. We could get the machine into the state you're suggesting via an interactive command, but we also need clean machines installing .NET for the first time to just work with the simpler flow. We couldn't figure out a combination of changes that would satisfy that. |
|
@DamianEdwards its possible to skip this in the first run experience and wait until the certificate is accessed for the first time in an interactive way. There is already a flag for determining when interaction is possible and when it is not, which the first run experience used in the past. Would that not work? |
|
So on a machine that has .NET 6 SDK already installed (and thus .NET 6 certs) what would the .NET 7 SDK first-run do? It can't export the existing cert (that requires elevation) so it must do nothing at all. Creating a new web app from the CLI and trying to run it will work in the default case because we now default to an http-only launch profile. But if the https launch profile is used (which VS for Mac will default to) Kestrel will fail to launch because the on-disk certificate is not present yet. If it is via VS we might be able to get their launch experience to prompt to fix up the cert at that point (like they do today), but if it's the CLI, what then? Won't Kestrel just fail? Are you suggesting we have it fail with a message that instructs the user to run the |
|
@DamianEdwards the cert manager checks for the certificate on disk, it doesn’t find it. It sees the certificate on the keychain and tries to export it to the disk location. Next time, it will find it there. On the first run experience it skips the export step. Does that make sense? |
|
@javiercn are you saying it exports it to disk when Kestrel tries to load it? That won't work because it requires user confirmation. Or are you saying Kestrel startup would fail with a message, and then the user would run the |
- Store dev-certs for macOS in a well-known location on disk in addition to the keychain. - Misc code cleanup/refactoring. - Update error messages.
adityamandaleeka
force-pushed
the
macos_devcerts_updates
branch
from
dc24b2b to
0346d22
Compare
|
Update: After some discussion with @javiercn and @DamianEdwards, we decided to soften the requirements a tiny bit to improve the experience. The latest update brings back a bunch of the code from earlier commits in this PR that dealt with pre-7 certs, re-unifies the OIDs so we won't require older certs to be managed by older SDKs (which would have been an annoying pain-point), and makes it so that Kestrel no longer tries to access the certificate's key at startup when With these changes, we still eliminate nearly all of the prompts people encounter while also making the experience reasonable for those who are working on both 6.0 and 7.0 codebases. Thanks @javiercn and @DamianEdwards. I think we've landed at a better experience now. |
javiercn
left a comment
javiercn
left a comment
There was a problem hiding this comment.
Looks great @adityamandaleeka.
Thanks for persevering through this.
I do have a minor comment on a log message to the console, but other than that it looks solid!
ghost
added this to the
7 participants
Improve the experience of doing development with HTTPS on macOS:
$HOME/.aspnet/dev-certs/https) to load the certificate from when running Kestrel in development mode bound to an HTTPS address.Create a mechanism for OS-specificEDIT 2: Change approach again. The macOSCertificateManagerimplementations to check if their state is consistent, and check for that as part of the --check command (and warn the user to clean up if it's not). The macOS implementation will now use this to detect cases where certs are only in one of the two places they should be (on-disk store or keychain). Update: instead of doing this we now just rev the certificate version, enabling both types of dev certs to be present. There was no good way to handle cleaning up the old state in an automated, non-interactive way, which is a requirement for the SDK first-run experience.CertificateManagerwill handles pre-7 certificates and update them on first-run (with a prompt).This change greatly reduces the number of password/Touch ID prompts that appear when trying to develop on macOS, and makes the dev-certs tool generally more pleasant to use on macOS.
Verified scenarios with these changes on Monterey and Big Sur. There are no automated tests for this since the keychain/trust manipulation requires user interaction.
Fixes #41879 and #41878