caddytls: clientauth: leaf verifier: make trusted leaf certs source pluggable

[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 #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 🤷‍♂️ 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 💯

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?

[mholt]

Looking good! I think we're close 😃

[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]

[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?

[mohammed90]

This is looking better! Just a few cleanup items.

Also, add some tests under caddytest/integration that will simply do tester.InitServer with config that has leaf verifiers. Just 1 for comfort.

[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.

[mohammed90]

Looks great now! Thank you, Aziz, for working on this.

[mholt]

Thank you, I think we're getting close!! Just a couple of nits and a question to clarify.

[armadi1809]

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

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

[mholt]

Awesome! Just one thing I missed, then we merge. ha

[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 🙄

[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

[armadi1809]

Okay... I think we should be good to go now 😅

[mholt]

YES -- nice work, thanks for the contribution!

(And you are fast. I am working through a lot of things today so you beat me to it!)