-
-
Notifications
You must be signed in to change notification settings - Fork 64
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(core): Add certificates to Operator.Web #756
Conversation
Hey @ian-buse Thank you for your contribution! :-) Yes, certificate stuff is quite weird, and you need a lot of time to get your head around it. The way these certificates are implemented now is to ensure that webservers do not reject them because they all need certain custom extensions. I had the same issues with localtunnel not being reliable. I'll review it right away :-) |
Thanks for taking a look at it. I just realized I made a small mistake with the EnhancedKeyUsageExtension critical = true. Easy fix but I wanted to point it out, I can add it to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ian-buse
Thanks again for the contribution!
The linter is not happy right now with something (thus, the build is failing).
A made a few comments that I'd like to discuss.
One thing about the reason: if I understand you correctly, you had problems with local tunnel being shakey (like I did). Now you implemented a special way to use self signed certificates for local development without localtunnel.
I really do like the idea! Since I had the same issues with localtunnel, I'd go even further and say we remove localtunnel altogether and make a breaking change. Then local webhooks can and should be tested with a local kubernetes instance (mini kube or whatever). What do you think about this?
I don't even know if this local tunnel feature was used heavily.
And another question: why did you replace bouncycastle? (I'm curious)
Yeah, I had problems with LocalTunnel not working correctly, which made me implement the self-signed certs instead. Removing LocalTunnel would be fine with me! BouncyCastle is a great library, but I replaced BouncyCastle with
I'll work on resolving the linter problem and other things you brought up. 😊 -Ian |
Thanks for the explanation :-) Yeah, I can absolutely understand that. I think more or less the same way, but BouncyCastle is (sometimes) great because the abstract the weird stuff like "OiD(183459122)" with something that has a meaning. That was the main reason. I do like the move! Especially because of the native libs. Regarding the removal of localtunnel: wdyt about this: you mark it as "deprecated" with the compiler attribute and we remove it in a later stage. and regarding the "lazy" thing, I just tried it with the following example: global using CertificatePair =
(System.Security.Cryptography.X509Certificates.X509Certificate2 Certificate,
System.Security.Cryptography.AsymmetricAlgorithm Key);
internal class CertificateGenerator : IDisposable
{
public Lazy<CertificatePair> Root => new(Generate);
private CertificatePair Generate()
{
return default;
}
public void Dispose()
{
if (Root.IsValueCreated)
{
Root.Value.Certificate.Dispose();
Root.Value.Key.Dispose();
}
}
} What do you think about such an implementation? (just a suggestion) |
Hopefully fix linting issue
Add CertificatePair record to Abstractions
Good, I'm glad you like the changes! Yeah I noticed some abstractions (like the OIDs and AuthorityKeyIdentifier) were much better in BouncyCastle. .NET is catching up, but there are definitely a few still missing or incomplete. I've resolved the issues we talked about, including the UTF-8 encoding, linting issues, moving |
Quick fix for #742. Related to #756. That PR has added documentation for development webhooks. Co-authored-by: Christoph Bühler <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool :)
Just realized that the local tunnel stuff already was in "preview". As such, we can remove it later on without having a breaking change.
Hey there @buehler!
This PR adds a new CertificateGenerator to Operator.Web, and it reworks a handful of the classes in Operator.Web to support using certificates in code. I more or less re-implemented the BouncyCastle solution from the CLI in Operator.Web using the built-in .NET classes, and I went to some length to ensure the generated certificates are virtually identical to those produced by BouncyCastle.
As for the reasoning, I admittedly had some trouble getting LocalTunnel working reliably in my environment. My company prefers just using our internal network versus a service, but I wanted to have a easy way to use self-signed certs with Kestrel and using your nifty webhook generator methods that are in the
DevelopmentTunnelService
. It also requires some knowledge of certificates and keys to get the BouncyCastle library working with certain .NET methods/classes, so this was a bit of a usability barrier for those who are unfamiliar.Here are the changes I've made:
TunnelWebhookService
andCertificateWebhookService
. The newCertificateWebhookService
is added to the operator with a new method inOperatorBuilderExtensions
, calledUseCertificateProvider()
.TunnelConfig
->WebhookConfig
,WebhookLoader
moved).CertificateGenerator
class in Operator.Web.CertificateGenerator
implements an interface calledICertificateProvider
, which is used byUseCertificateProvider()
so someone can implement their own solution (like reading from a file) if they want.There are no breaking changes with these commits.
Regards,
Ian