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

feat(dgw): support for Windows Certificate Store #576

Merged
merged 6 commits into from
Oct 21, 2023
Merged

feat(dgw): support for Windows Certificate Store #576

merged 6 commits into from
Oct 21, 2023

Conversation

CBenoit
Copy link
Member

@CBenoit CBenoit commented Oct 20, 2023

Three now option keys are added:

  • UseWindowsCertificateStore (enable usage of the store)
  • WindowsCertificateStoreType (type of the store to use)
  • WindowsCertificateStoreName (name of the store to use)

Issue: DGW-105

@CBenoit CBenoit requested a review from awakecoding October 20, 2023 02:13
@CBenoit CBenoit enabled auto-merge (squash) October 20, 2023 02:13
Three now option keys are added:
- `UseWindowsCertificateStore` (enable usage of the store)
- `WindowsCertificateStoreType` (type of the store to use)
- `WindowsCertificateStoreName` (name of the store to use)

Issue: DGW-105
@awakecoding
Copy link
Contributor

Good job! I'd like to review the configuration and loading behavior a little bit to closely match Kestrel endpoint configuration for the Windows certificate store.

The Kestrel configuration allows specifying a default certificate + additional certificates selected dynamically using server-name indication. Rather than automatically select a certificate from SNI at all times, I think we should simply use the configured default certificate at this point, otherwise issues that would normally result in a name mismatch would now result in a missing certificate, which is arguably worse.

The Kestrel configuration for a Windows certificate story entry uses 3 fields (let's just ignore the 4th, AllowInvalid):
"Subject": "<subject; required>",
"Store": "<cert store; required>",
"Location": "<location; defaults to CurrentUser>",

"Subject" is the subject name - we need the same option here to select the certificate by subject name in the desired Windows certificate store. For https://dvls.ad.it-help.ninja, the subject name would be dvls.ad.it-help.ninja.

"Location" is CurrentUser or LocalMachine from the .NET enum Kestrel uses. However, you've already mapped the less frequently used CurrentService store supported by the Windows APIs, which I agree would be great to support. It's a great option to isolate certificates for a given service, even if it is harder to work with using regular tooling. I would keep it for that reason. As for the default used by Kestrel (CurrentUser) it isn't what we wish for Devolutions Gateway - maybe let's just use LocalMachine as a default instead since it's what we want most deployments to use.

"Store" is the certificate store name, as seen in PowerShell or Windows APIs (cert:\LocalMachine\My, where "My" is the store name matching "Personal" in the cert store GUI). There are built-in stores, but it's possible to create custom stores. For this reason, it should be a string, not an enum type internally.

As for the certificate loading logic, I found the function in the Kestrel CertificateLoader.cs: it picks the matching certificate with the furthest expiration time. This means if we have multiple matching certificates in the store, it'll pick the one which is set to expire the latest. The loader also checks for key usage and validity, maybe we can just check for key usage and load invalid certificates. Or maybe we can actually implement the 4th Kestrel parameter (AllowInvalid) to force loading only valid certificates or fail to load it otherwise as a future improvement.

The original Kestrel parameters are a bit too short to mean much, at a bare minimum, this is easier to understand in the context of a certificate configuration, and actually match better the .NET naming:

SubjectName (Subject)
StoreName (Store)
StoreLocation (Location)

For our configuration, we could use UseWindowsCertStore (instead of UseWindowsCertificateStore) to enable the Windows certificate store, then we'd have the following parameters:

CertSubjectName
CertStoreName
CertStoreLocation

We don't necessarily need to repeat the whole Windows part, and these parameters would only be meaningful for the Windows cert store. If we ever add support for other cert stores, CertSubjectName is generic enough to be reusable.

@CBenoit
Copy link
Member Author

CBenoit commented Oct 20, 2023

Thank you for the detailed write-up!
I modified the code according to it and our discussion on slack.

Final decision for configuration keys was:

  • TlsCertificateSource
  • TlsCertificateSubjectName
  • TlsCertificateStoreName
  • TlsCertificateStoreLocation

README.md Outdated Show resolved Hide resolved
@CBenoit CBenoit disabled auto-merge October 20, 2023 20:18
@CBenoit CBenoit enabled auto-merge (squash) October 20, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants