Make trusted leaf certs pluggable into leaf certs verifier

[armadi1809]

Closes #6046

[armadi1809]

@mohammed90, I don't know if this is what you meant by "we should make this pluggable" so this is still in draft. I would like to get your early feedback about where this is going. This is my first time creating a caddy module from scratch and hosting it so any advice is appreciated 😅 Thank you in advance for your help.

[mholt]

Thanks @armadi1809 !

@mohammed90 , is this similar to https://github.com/caddyserver/caddy/pull/5784?

[mohammed90]

@mohammed90 , is this similar to #5784?

No. This one is for the verifiers.

Thanks, @armadi1809! I'm a bit busy this weekend, but you'll need to go through this page: https://caddyserver.com/docs/extending-caddy. This doesn't properly implement a Caddy module. The PR is not pluggable. Effectively, this still only accepts one kind of source of certificates. The type name and tag says certificate names", but it's just decoding DERs which means it only accepts certificates that are directly given in the configuration as DER. What if the certificates are stored in a file in the filesystem? What if I want to read them from an HTTP endpoint? This is the test of pluggable, they could come from a plugin.

[armadi1809]

@mohammed90 Thank you for your explanation. I actually went through that document before working on this but looks like I didn't totally get your point. I made some changes in my last commit but I still have some doubts regarding the concept of "pluggable" in this case. I changed the logic to have the new module contain the decoded certs and provide them to the tls.client_auth.leaf module in the provision step. Sorry in advance if this is not what you meant.

[francislavoie]

I changed the logic to have the new module contain the decoded certs and provide them to the tls.client_auth.leaf module in the provision step. Sorry in advance if this is not what you meant.

Did you forget to push? I don't see your changes, I only see your rebase from earlier.

[armadi1809]

The changes are part of the rebase. I changed the type of the new module we're adding from a []string to a []*x509.Certificate and removed the logic that was decoding DERs and replaced it by a simple assignment in the provision step of the tls.client_auth.leaf.

[francislavoie]

Okay, it's still not what @mohammed90 was asking for - it's still not pluggable.

To be pluggable (in the sense of a Caddy plugin, not "plugging data into the config", if that was unclear), there needs to be an interface that a plugin could implement to provide info or perform a task.

Read through https://caddyserver.com/docs/extending-caddy, especially the part on namespaces & interfaces.

We want this to be a Host Module which can take a guest module as input, and guest modules will load certificates in various ways (i.e. from file, or statically, or from an HTTP endpoint, or a storage plugin, etc).

For example, the tls.client_auth namespace takes guest modules like tls.client_auth.* which must conform to the ClientCertificateVerifier interface:

// ClientCertificateVerifier is a type which verifies client certificates.
// It is called during verifyPeerCertificate in the TLS handshake.
type ClientCertificateVerifier interface {
	VerifyClientCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error
}

We do already have the caddytls.CertificateLoader interface... it might be possible to reuse that interface, under a different namespace? WDYT @mohammed90 :man_shrugging: I don't really know what's needed here, I'm just kinda scraping by on the little I understand about this stuff.

[mohammed90]

Francis is right :100:

We do already have the caddytls.CertificateLoader interface... it might be possible to reuse that interface, under a different namespace? WDYT @mohammed90 🤷‍♂️

I think it's best to re-use the interface and the namespace, which is tls.certificates. This way we get a collection of implementations automatically.

[francislavoie]

I think it would have to special-case skip the automate module though (or maybe that already just works because it doesn't actually conform to that interface, and would get rejected by the type assertion?)

I don't think the existing tls.certificates.* modules are appropriate here because they try to load a cert + key. For client auth, we only want the cert, not the key. So they might need to be new modules.

[mohammed90]

because they try to load a cert + key

Yeah, just looked at the code now and noticed the same thing. Re-using the existing interface won't work because the data returned by the interface, i.e. caddytls.Certificate, contains tls.Certificate while we need x509.Certificate.

We'll need a new interface and a new namespace.

[armadi1809]

I think I am starting to understand 😅 . Thanks for mentioning the tls.certificates namespace. Looking at the code there kind of gave me an idea about what is wanted here. I will give it another shot.

[mholt]

You're doing great! Sorry I haven't been more involved in the process lately (baby takes up my spare brain cycles and time) -- but you're on the right track. Thank you!

[armadi1809]

I think this now aligns closer to what you guys meant, right? If we agree there, my next question is, would we be writing the guest modules for this new loader on this PR? And if so, what are the different types of loaders we want to implement?

[armadi1809]

All the feedback items should now be resolved. I guess my last question is, will we be providing a couple of tls.leaf_cert_loader.* modules or will we leave that to the users?

[francislavoie]

I think we should have some built in, same as caddytls.CertificateLoader. Can probably copy-paste those but just adjust them to only read in certs and not keys

[armadi1809]

I added some built in leaf certs loaders. I didn't know if I should be incorporating the file and the folder loaders into the Caddyfile config similarly to the tls.certificates loaders or not. I will wait for your feedback on that.

[mholt]

Thanks for working on this and so many other patches recently, @armadi1809. Could you join our developer Slack? That way you're welcome to discuss changes and other Caddy things in more real-time with our team. Just let me know which email address to send the invite to.

[armadi1809]

@mholt Thank you for the invite! I appreciate it. You can use [email protected]

[mholt]

I haven't been able to read every single line of this but so far it's looking pretty good to me! @mohammed90 what do you think?

[armadi1809]

@mohammed90 sorry this is taking too long, as there were some misunderstanding on my part on how to write the JSON config for the loaders 😅 . With that out of the way, I was wondering what would be the best way to test this? Is there a way to test if a given caddy JSON config is valid inside the caddytest package or are you envisioning the tests to spawn a caddy instance with a given config (using InitServer) to make sure the config loads properly and we are able to get a response back for a request?

[armadi1809]

I made some improvements to the way we were loading certificates from the different loaders (After @mohammed90 review). This still needs tests but like I said in my previous comment, I am not entirely sure regarding the best way to test this.

[mohammed90]

I am not entirely sure regarding the best way to test this

Testing this from the caddytest/integration package could be hard. It'll require you to customize the HTTP client to present the expected certificate during the HTTP request. If you're up to that challenge, go for it. Otherwise, you can probably just confirm the config load is successful.

You can probably create a fresh caddy.Context, then call Provision then LoadLeafCertificates on each of the modules' tests to check if the loaded data matches the expectation.

[armadi1809]

@mohammed90 Thanks for reviewing this. I implemented your suggested changes. I think I also have an idea on how to test this now. I will try to implement tests here shortly.

[armadi1809]

@mohammed90 I added tests for the file and folder loaders. Is that what you meant by loading the config and check whether or not it meets the expectations?

[armadi1809]

@mohammed90 all the feedback items from above should now be resolved. I also added a test for the PEM loader as well as an integration test.

[armadi1809]

@mholt Thanks for reviewing. All the feedback items should now be resolved.

And thank you @mohammed90 for all your efforts with this!

[mholt]

That's weird, govulncheck wasn't failing just 30 minutes ago... fluke?

[mohammed90]

That's weird, govulncheck wasn't failing just 30 minutes ago... fluke?

I think the vuln was published recently: https://pkg.go.dev/vuln/GO-2024-2611

[mholt]

Of course, our luck :roll_eyes:

[mohammed90]

Of course, our luck 🙄

Yeah 😶 The fastest course of action is if you, @mholt, update the dep google.golang.org/protobuf on master then rebase/merge master here. Otherwise, it'll require me to push another PR.

Or... @armadi1809, can you just run this?

go get -u google.golang.org/protobuf