tls: modularize trusted CA providers

[mohammed90]

The current implementation of client authentication requires the CA to be either known in DER format or available on disk as PEM files. This prevents the advanced usage of client authentication by standing on the shoulders of the PKI app or to fetch the certs from shared Caddy storage. The current implementation makes it hard to share the same trusted client CAs across a cluster of Caddy servers without having to produce the root certs first then plugging them. This is a long way of saying, the trusted root CA is very rigid and not extensible.

This implementation allows for the trust CA to be any Caddy module that satisfies an interface. It also provides 4 basic providers:

• File, to maintain backwards compatibility
• Root certs from the PKI app
• Intermediate certs from the PKI app
• Storage

TODO:

• Caddyfile
• Tests
• Docs

[mholt]

Oh this is actually awesome -- thank you for contributing this!

It's pretty close to what I'd do.

• Exported types and their fields will need godocs.
• It's a slight bummer we can't combine the PKI*CAPool types. (Unless we can? I wonder what that would look like. It's just a matter of what the admin wants to sign the certs with, right?)

I have little in the way of criticism of this change. Nice work 😃

[mohammed90]

* Exported types and their fields will need godocs.

* It's a _slight_ bummer we can't combine the PKI*CAPool types. (Unless we can? I wonder what that would look like. It's just a matter of what the admin wants to sign the certs with, right?)

I'll come back to it when I'm in a better head-space.

One more thing, I did consider Lazy for some of them, which delays the load-and-parse until the pool is requested, allowing the pool certs to updated without having to reload Caddy. I changed my mind because this will be on a hot-path, and such delay will be unacceptable. They can be 3rd party modules if at all needed.

[mohammed90]

I did consider Lazy for some of them, which delays the load-and-parse until the pool is requested, allowing the pool certs to updated without having to reload Caddy.

Implemented for consideration.

I realized last week that this can be generalized to be the RootCA provider for other use cases, e.g. reverse-proxy transport. I will not touch the reverse-proxy transport in this PR though.

[mholt]

Wow, I really appreciate all the work that went into these tests. Can't wait to review!!

[mholt]

This is a big improvement. 7 client auth provider modules built-in, that's pretty awesome! Thanks for working on this. LGTM.

"Matt, how can you approve a 2,500 line change without any review comments?" -- because Mohammed writes good code and nothing obvious stuck out to me. This is a bigger PR than I usually review, so yes I'm sure some details got overlooked. But I'd rather get this merged in and tested before a release than try to find all the problems with my tired eyes right now. 🙂

[mholt]

Nice work on this :)

[Gr33nbl00d]

Correct me if i am wrong but It seems you accidently broke #6022
With your change. The verifiers are not parsed anymore. In the conpollicy go you missed the verifiers section.
It is missing here :

caddy/modules/caddytls/connpolicy.go

Line 437 in a6d9f9b

case "mode":

[mohammed90]

Correct me if i am wrong but It seems you accidently broke #6022
With your change. The verifiers are not parsed anymore. In the conpollicy go you missed the verifiers section.
It is missing here :

caddy/modules/caddytls/connpolicy.go

Line 437 in a6d9f9b

case "mode":

It shouldn't break. I was aware of it and tried my best to avoid breakage. The certificates are still parsed when the directive has the _leaf suffix. Do you have a test case/proof of the break?

[Gr33nbl00d]

No you got it wrong, i try to explain it better: in the referenced pull request there was support added to load client certificate verifier modules via caddyfile. Which was previously only possible via json config. This has nothing to do with leaf certificates. It is used to do additional checks on client certificates. For example revocation checks via my plugin

https://github.com/Gr33nbl00d/caddy-revocation-validator

In line 437 this seems to be missing:

		case "verifier":
			if !d.NextArg() {
				return d.ArgErr()
			}

			vType := d.Val()
			modID := "tls.client_auth." + vType
			unm, err := caddyfile.UnmarshalModule(d, modID)
			if err != nil {
				return err
			}

			_, ok := unm.(ClientCertificateVerifier)
			if !ok {
				return d.Errf("module %s is not a caddytls.ClientCertificatVerifier", modID)
			}

[Gr33nbl00d]

You can see it here in the old builtins.go line 221 the verifier section was there:
e965b11#diff-7761c0c9beafcd055ce9c6c894b6c54b888bc00598767551dfcc51b07bb81c36L221

[Gr33nbl00d]

Correction i forgot 2 lines to copy :) Last 2 lines were missing in my previous comment

No you got it wrong, i try to explain it better: in the referenced pull request there was support added to load client certificate verifier modules via caddyfile. Which was previously only possible via json config. This has nothing to do with leaf certificates. It is used to do additional checks on client certificates. For example revocation checks via my plugin

https://github.com/Gr33nbl00d/caddy-revocation-validator

In line 437 this seems to be missing:

		case "verifier":
			if !d.NextArg() {
				return d.ArgErr()
			}

			vType := d.Val()
			modID := "tls.client_auth." + vType
			unm, err := caddyfile.UnmarshalModule(d, modID)
			if err != nil {
				return err
			}

			_, ok := unm.(ClientCertificateVerifier)
			if !ok {
				return d.Errf("module %s is not a caddytls.ClientCertificatVerifier", modID)
			}
                       cp.ClientAuthentication.VerifiersRaw = append(cp.ClientAuthentication.VerifiersRaw, 
                       caddyconfig.JSONModuleObject(unm, "verifier", vType, h.warnings))

[francislavoie]

Yeah I see what you mean @Gr33nbl00d. It got lost in translation somewhere.

I think this happened because ultimately, we don't have any actual implementation of that module interface in Caddy itself, so it's not possible to write a test using anything built-in.

We should probably make a mock cert verifier module in a _test.go file which would let us run a Caddyfile adapt test for the verifier option.

PRs welcome!

[Gr33nbl00d]

Yes you are right i had the same idea ;) It would definitly help to harden the code. It was maybe because a merge conflict in the builtin.go file because 2 different prs affected the same file.

[mohammed90]

Aaah I see your point, @Gr33nbl00d. Sorry 😔 I'll definitely review a PR to add it back (with a test 🙂). If you don't beat me, I'll add it back but it might take me a while.

[Gr33nbl00d]

Aaah I see your point, @Gr33nbl00d. Sorry 😔 I'll definitely review a PR to add it back (with a test 🙂). If you don't beat me, I'll add it back but it might take me a while.

No problem, this can always happen. We are humans not machines and we make mistakes. I am still in the process of implementing the caddy file support. This is not yet released :) So take your time