Configurable certes verification certificates #279
Conversation
|
@natemcmaster if you prefer, this feature could also be adjusted to only be available in staging mode (logging a warning when used within a production environment, e.g. "... was not applied as it is only available in staging mode"). I don't think the current API has any security implications, but it would be the more defensive way of doing this, ensuring no production environment is impacted. I am willing to change any parts this PR as you see fit - feel free to add any feedback if you like, even if the PR is not merged in the end. |
Codecov Report
@@ Coverage Diff @@
## main #279 +/- ##
==========================================
- Coverage 46.23% 46.16% -0.08%
==========================================
Files 45 45
Lines 1116 1120 +4
==========================================
+ Hits 516 517 +1
- Misses 600 603 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Thanks @Mafii. This PR seems fine to me as is. There is one additional thing I think would be useful.
What do you think about doing both things? I could see it being useful to have both the ability to manually add issuers, and also to have your CA configuration provide this. I'm guessing that addressing #263 would be a matter of adding a class which implements Thoughts? |
|
@natemcmaster thanks so much! I just tried to add this to the I've considered some options: Option 1: Source and Runtime breaking change for standard 2.0, but not for core 3.1 and later:Reasonably, this means LettuceEncrypt v2.0 which seems overkill for the (arguably useful but limited in functionality) value added. We could lie about the version, but this is problematic for standard 2.0 users. I don't like this option. /// <summary>
/// Certificates passed to certes before building the successfully downloaded certificate,
/// used internally by certes to verify the issuer for authenticity.
/// </summary>
/// <seealso cref="LettuceEncryptOptions.AdditionalIssuers" />
public string[] IssuerCertificates
#if NETSTANDARD2_0
{ get; }
#else
=> Array.Empty<string>();
#endifOption 2: Introduce a new interfaceDownside: Adding it to Option 3: We avoid this feature... but only for standard 2.0The functionality can be used through the options already - this is some inconvenience for those limited to standard 2.0, but allows better usage for most users, and all new users that have no restrictions in target frameworks. Downside: It is not that funny to have different APIs for different target frameworks? What do you think? |
|
I’m actually ok with the idea of dropping netstandard2.0. Here’s my thinking: I added .NET Standard when this library began 5 years ago because, at the time, there was still some levels of interest in getting ASP.NET Core libraries to work on .NET Framework. I think it has been long enough now since aspnet/Announcements#324 that I’m ok entirely dropping .NET Standard. In fact, I’m okay jumping straight to net6. .NET Core 3.1 is now EOL, as is net5. Yes, this a breaking change, but also, I am okay not supporting old frameworks versions which even Microsoft doesn’t support anymore. |
|
@natemcmaster alright, good to hear & makes sense. I'll provide a PR when I find time! |
|
Btw, the root cause (pun!) is that the bouncy castle PFX builder needs a known intermediate or root to verify the chain from, but it turns out you don't actually need the root, just the issuer closest to the root. See my certes fork here (Webprofusion.Certes) as used by Certify The Web. It's starting to diverge rather a lot from the original and I haven't proven there are no side effects of this approach yet, but the PFX does build without the root issuer being added: webprofusion/anvil@16dc9f7#diff-3c1160ec9eab4ad6ed521d35d890a3cf0d4d1ab967db59fca986b06697970d3aL145 |
Usage of the new API:
When starting with e.g. the Staging Root certificate from Let's Encrypt, Lettuce Encrypt will still issue the following warning in the end:
This can be ignored.
Why is this needed? What value does this PR add?
AddIssuer(s)that can be used to add Certificates to that list of embedded certificates.Related issues
This PR is likely the "extension point" you talked about in this comment: #230 (comment)
This PR may partially solve issues such as #274, I am not very confident in that assessment however.
It may also make #263 possible - I don't think your proposed solution was sufficient as certes should to my understanding throw because the google acme server's root certificate isn't well known. I may be wrong here as well.
It for sure solves #278, as I have tested that myself. The pull requests #276 and #277 were initial steps/solutions to getting LettuceEncrypt running in a project using the staging server with ngrok (v3) and the current version of LettuceEncrypt. I would recommend taking a look at them as well.
Additionally, once the
dst-root-ca-x3.pemexpires in 2030 and theisrg-root-x1.pemexpires in 2035,Lettuce Encrypt will have to upgrade certes (if certes adds the new certificates as embedded resources) or manually pass in the new Let's Encrypt root certificate every time. This change will be less dramatic/annoying with this PR, as users of Lettuce Encrypt can workaround/fix this "problem" themselves without having to upgrade any packages to a new version.
Alternative Architecture considerations
Another way of adding the functionality contained in this PR would be to add the additional certificates to
ICertificateAuthorityConfiguration, as it kind of belongs there. I realized this looking at a comment in #263.