Allow revocation list for client cert auth

u1735067 commented 6 years ago

Hello, could caddy add an option to check client certificate against a revocation list ? This was the object of https://github.com/mholt/caddy/issues/1904, which it was closed as duplicate of https://github.com/mholt/caddy/issues/1375, but https://github.com/mholt/caddy/issues/1375 doesn't seems to implement that. Best,

1. What version of Caddy are you using (`caddy -version`)?

v0.11.0

2. What are you trying to do?

Disallow revoked client certs

3. What is your entire Caddyfile?

Not relevant, there's no option to do that as per https://caddyserver.com/docs/tls

mholt commented 5 years ago

@Alex131089 Any reason you need to use a CRL specifically? What about some custom logic to check the provided client certificate to see if you'd allow it?

u1735067 commented 5 years ago

Usage: disallow a compromised certificate, like a certificate installed on a lost phone for example. What kind of logic are you referring to? Checking CN and/or Dates for example, or hash of the certificate? I don't need CRL specifically, but I believe it's simpler as it is commonly used for this purpose (and maybe already available to the TLS lib?), while managing multiple criterion doesn't sounds good?

mholt commented 5 years ago

I suppose I am interested as to why you'd rather maintain both a list of approved client certs AND a CRL to override entries in that list, than simply a list of approved client certs: if the key is compromised, remove it from the list of approved certs.

u1735067 commented 5 years ago

Hum, what list of approved client certs, I feel I missed something? If I understand, in the configuration (https://caddyserver.com/docs/tls) you give a list of approved CA, but theses CA may approve multiple certificates, so you might want to revoke one of them. The other solution would be to have 1 certificate per CA?

francislavoie commented 5 years ago

@Alex131089 this is being asked more in the context of Caddy v2. See the discussion relating to this PR https://github.com/caddyserver/caddy/pull/2731

mholt commented 5 years ago

@Alex131089 It's a Caddy 2 feature :wink: -- we've greatly improved TLS controls in v2.

u1735067 commented 5 years ago

Oh ok :)

client_authentication { trusted_ca_certs, trusted_leaf_certs }

My point of view, if it's welcomed: I feel like it's reinventing the wheel for this feature? (Also, base64 certificate in the configuration file?) But a more pragmatic reason against is, it will be more probable/usual I have to allow a new cert (if I don't forget to edit the Caddy configuration after I generate it ..), than I have to revocate it (should be exceptions), so the less I have to touch the configuration the better.

u1735067 commented 5 years ago

My usage for this feature (client cert) btw: allow access to sensible/untrusted portals/apps (proxmox, irc webclient, ..) only from trusted personal devices, while still exposing them publicly.

mholt commented 5 years ago

My point of view, if it's welcomed: I feel like it's reinventing the wheel for this feature?

I dunno about that; hesitating to implement a feature that might not be needed isn't a re-invention, just questioning whether a CRL is really the right answer here.

(Also, base64 certificate in the configuration file?)

Would you prefer something else? We could change it if the reason is compelling enough...

so the less I have to touch the configuration the better.

I suppose this is true, but with a CRL you have to touch two things: add a CRL to your config, and add the cert to the CRL.

Tell ya what, I'll accept a good pull request that implements this feature; but it's not a priority for me to implement right now.

u1735067 commented 5 years ago

My point of view, if it's welcomed: I feel like it's reinventing the wheel for this feature?

I dunno about that; hesitating to implement a feature that might not be needed isn't a re-invention, just questioning whether a CRL is really the right answer here.

I mean using whitelist instead of CRL, but why not, yes ; more people should probably say what they expect.

(Also, base64 certificate in the configuration file?)

Would you prefer something else? We could change it if the reason is compelling enough...

Certificates are usually loaded from files, when discovering this, I'd be surprised you have to put it directly in the configuration file (involving a possible format conversion and not as simple as copy/pasting a file).

Nginx/Apache are using file directives:

so the less I have to touch the configuration the better.

I suppose this is true, but with a CRL you have to touch two things: add a CRL to your config, and add the cert to the CRL.

Once the CRL file is in the config, when revoking, you "just" have to generate the new CRL file (appending the new revocation) and replace the old CRL file with this one. If Caddy cannot detect the change in the file (date/size), you'd also have to reload Caddy. But you're not touching the configuration (which would be very error-prone, especially with big base64 strings).

Tell ya what, I'll accept a good pull request that implements this feature; but it's not a priority for me to implement right now.

I get it, as it's not very important to me I'm probably not playing with this but thank you for considering it though :)

mholt commented 5 years ago

Certificates are usually loaded from files

True; I've had a hard time deciding whether to stick with files or with file contents in the Caddy 2 config because the Caddy 2 config is designed to be generated on-the-fly from tooling that may or may not want to use files to load parts of the config, like certificates. Although in some cases we allow both (like loading server certificates in the TLS app), just under different object key names: https://github.com/caddyserver/caddy/wiki/v2:-Documentation#tlscertificates

Anyway, my offer still stands, if there's a PR to implement a CRL, I'll be happy to review it!

mholt commented 5 years ago

Thanks to this discussion, I just realized the v2 docs were out of date for client auth, but I updated them today: https://github.com/caddyserver/caddy/wiki/v2:-Documentation#httpserverstls_connection_policies

shynome commented 5 years ago

need this feature +1. I like the caddy simple config but I have to go back nginx because nginx support and this is required for security when use tls client certificate. At last, thank you for so great software

allardmuis commented 5 years ago

@mholt I'd really like to have this feature as well. I'll try to describe my usecase and why other solutions are not working for me.

I have a system where I have to grand access to my api to other servers. Which other parties are allowed is decided by a government institute. Anyone can apply, and if successfully audited, gets a certificate signed by their CA. So there are many certificates out there, I don't know which ones, new ones can be added every day. So I allow any client-certificate signed by the CA, without knowing in advance which specific certificates there are. This is easy with Caddy: i just set the CA in 'clients' option.

If a certificate is compromised, or someone fails the yearly audit, the CA revokes the certificates of that party. The certificate is put on the CRL maintained by the CA. I have to check that, otherwise I keep granting access to systems that should not have it anymore. Even better would be to use OSCP and request to the CA if a client-certificate that is used is still valid.

TLDR; I think CRLs are the only way if you don't hand out the client-certificates yourself, but trust everything signed by an external CA.

Btw: Reading CRLs from file would be much nicer than having them in the config file itself: a CRL might contain hundreds of certificates so the config would get bloated quite a bit.

Gr33nbl00d commented 5 years ago

I need this feature too, we have exactly the same use case like @allardmuis As this is quite urgent for us we need to find some solution soon. Could be that im forced to do the changes myself and open a pull requests. But as i have nearly no expirience with golang this will be last solution i would like to go.

@mholt you might have a look here as example: https://github.com/cloudflare/cfssl/blob/master/revoke/revoke.go

mholt commented 5 years ago

@Gr33nbl00d

I need this feature too, we have exactly the same use case like @allardmuis As this is quite urgent for us we need to find some solution soon. Could be that im forced to do the changes myself and open a pull requests. But as i have nearly no expirience with golang this will be last solution i would like to go.

Absolutely understand -- I can get authorized to work on this feature right away if your company can fund its development. If you fill out this form, we can get the ball rolling today: https://www.ardanlabs.com/my/contact-us?dd=caddy

Thanks for the link, too, that might come in handy!

Gr33nbl00d commented 4 years ago

@mholt I mentioned it to our product manager today ;) Because of some other problems this is postponed to begin of next year. So we life first without CRL till begin of next year. Than we will see ;)

wille-io commented 4 years ago

I'd also love to see this feature in caddy. Because I would rather have a list of 2 or 3 compromised certificates in a revocation list than having to put hundreds of certificates in a whitelist.

mholt commented 4 years ago

As I said above, we can prioritize this feature if your company can pay for its development; or anyone else is welcome to propose and implement it voluntarily. It's on the list (as the issue is still open) but on the backlog as I work through more pressing priorities.

wille-io commented 4 years ago

Sorry, didn't want to exert pressure or anything, just wanted to state my point of view. I wish I knew Go, I can't even find where the variable "ClientCAs" is used, so I could see where to put the counter part for it and implement it myself for Caddy.

mholt commented 4 years ago

@wille-io

I can't even find where the variable "ClientCAs" is used,

It's used by the Go standard library's crypto/tls package. You can find it used in there: https://golang.org/search?q=ClientCAs

Gr33nbl00d commented 3 years ago

Hi again, nearly 2 years later and we are now in heavy need to implement this feature. I am currently trying out how to solve things. I was thinking about using OCSP instead of CRL but: But it seems that you only support OCSP for server certificates not for client certificates. It seems that OCSP and CRL checking needs to be manually implemented via verifyPeerCertificates function.

Is that correct? If so i can now decide to implement CRL or OCSP support for client certificates or maybe even both ;)

mholt commented 3 years ago

The Go standard library will first perform its own verification of the certificate, so if it specifies an OCSP server I imagine it would take care of that for us, yeah?

We can still make a CRL configurable. You want to have a go at it?

Gr33nbl00d commented 3 years ago

i just debugged the tls handshake in crypto/tls/handshake_server.go and it does not look like if there is any OCSP check regarding the client certificate.

It looks like you have to implement checking of client certificates yourself by implementing it in verifyPeerCertificate method.

I found some other go project "snowflakedb" who seemed to have similar issue:

https://github.com/snowflakedb/gosnowflake/issues/5

They have implemented it basically here: https://github.com/snowflakedb/gosnowflake/blob/master/ocsp.go

A very basic solution for ocsp checks with the existing ocsp functions can be found here: https://stackoverflow.com/questions/46626963/golang-sending-ocsp-request-returns

Ofcourse there is caching and a lot of important stuff missing in the last example.

Gr33nbl00d commented 3 years ago

Also interesting is that issue: github.com/golang/go/issues/36736 Ok im out for today ;) Need some sleep and think about further steps tomorrow ;)

Gr33nbl00d commented 3 years ago

Regarding your question if i give it a go. Yes i will start with this. We need that feature very soon. Currently i am just checking the possibilities. Im on vacation the next 3 weeks so i have only the next 2 days to collect some info to get a decisions from management if i should go for crl or ocsp or for both. Ocsp might require enrollment of new client certs on our customers as they are using crls atm and im not sure if this is possible. But i think ocsp is the better way. So maybe we need both in the end ;)

mholt commented 3 years ago

Hm, indeed, OCSP verification hasn't been implemented yet, but it has been proposed, and that is what I was thinking of: https://github.com/golang/go/issues/40017

These are rather complex (as Filippo rightly points out) and I hesitate to implement that ourselves into Caddy.

I do think OCSP is probably better, but both have their uses in some cases I believe. If we do implement it ourselves, I'd rather switch to using the Go standard library when they do finally implement it. So whoever contributes this to Caddy will need to be willing to replace it with the standard lib's implementation if/when it becomes available.

Gr33nbl00d commented 3 years ago

For sure every line which can be deleted is a good line of code. However i digged a bit deeper into the parsing of crl. One thing i think was also a reason why they did not want to implement crl is that the asn1 implementation of go is more "dom" based. That means reading a whole asn1 structure in memory. And that's always a bad idea if you need to parse indefinitely growing structures like crl. I am about to implement a stream based asn1 parser right now as there is none out there. So you can work with every size of crl. Its a bit childish of the go developers to try to simply ignore that fact. Yeah crl are something that might go away in 10 years but still a lot of good working software depends on it. Also companys depend on it. A fact you should not ignore. To implement both things is not an easy task luckily its not so hard for me ;) i was even thinking to make this available as library so others can benefit but for the first shot i will keep it in caddy. As this problem is open for so long i dont expect that a solution will arrive soon.

mholt commented 3 years ago

@Gr33nbl00d

Its a bit childish of the go developers to try to simply ignore that fact. ... A fact you should not ignore.

I don't know much about this particular function, but I would not assume childishness on the part of the Go team. That's a naive assertion/claim. I do not think they are "ignoring" anything, especially given their track record there. Is there an issue tracking a stream-based ASN1 parser in the Go libraries that you've been following which has gone ignored?

To implement both things is not an easy task luckily its not so hard for me ;) i was even thinking to make this available as library so others can benefit but for the first shot i will keep it in caddy.

That's good then.

Pinging @FiloSottile -- since you're more expert on this than I am; maybe you'd have any tips or recommendations before going down the "implement an ASN1 parser." (If we do end up using something like this in Caddy, it will need to be solid.)

FiloSottile commented 3 years ago

Not sure what is being discussed here. Is it implementing CRL logic, an OCSP verifier, or a new ASN.1 parser?

Checking CRLs should be relatively easy, although you'd have to make decisions about the frequency at which to fetch them, and take care to extract the location of the CRLs properly. This is the kind of things you want to ask Sleevi about.

An OCSP verifier is an extremely nuanced component. See golang/go#40017.

I do not understand the purpose of a new ASN.1 parser. golang.org/x/crypto/cryptobyte or even encoding/asn1 are perfectly fine for CRL or OCSP support. It would be weird if Caddy grew and had to maintain a whole ASN.1 parser, and then adapt to all X.509 quirks, just to parse CRLs.

mholt commented 3 years ago

Yeah, I do not want a separate/new parser in Caddy (or as a dependency, either).

Gr33nbl00d commented 3 years ago

@FiloSottile @mholt your meantioned parsers are all memory based parsers not stream based parsers. This is not a good idea if you need to parse data with undefined size. What will happen if you get a 8GB revocation list? Thats why i am implementing a stream based parser does not mean i parse everything manual ofcourse i use encoding/asn1 and crypto stuff. I simply do not parse the asn1 sequences with all revocation entrys at once but i parse them one by one. Just have a litle bit of trust that i know what i am doing ;) You can review my code an than you can complain if you like. This is ofcourse only needed for CRL parsing. OCSP is in way easier to implement.

Gr33nbl00d commented 3 years ago

Not sure what is being discussed here. Is it implementing CRL logic, an OCSP verifier, or a new ASN.1 parser?

Checking CRLs should be relatively easy, although you'd have to make decisions about the frequency at which to fetch them, and take care to extract the location of the CRLs properly. This is the kind of things you want to ask Sleevi about.

An OCSP verifier is an extremely nuanced component. See golang/go#40017.

I do not understand the purpose of a new ASN.1 parser. golang.org/x/crypto/cryptobyte or even encoding/asn1 are perfectly fine for CRL or OCSP support. It would be weird if Caddy grew and had to maintain a whole ASN.1 parser, and then adapt to all X.509 quirks, just to parse CRLs.

Just google for Adam Langley and revocation lists there is a huge discussion. He simply thinks that revocation checks do not make sense CRL based. But a lot of security experts see this different. And the industry still relies a lot on CRL. Even OCSP is often backed by CRL. Adam Langley has a quite ignorant meaning on that Topic. I work for a big company and our customers want CRL no matter what Adam Langley says

Gr33nbl00d commented 3 years ago

I can also implement it as a seperate module if you make it possible for modules to implement the verifyPeerCertificate or in general client certificate checking. This way the whole stuff could be outside of caddy.

francislavoie commented 3 years ago

I can also implement it as a seperate module if you make it possible for modules to implement the verifyPeerCertificate or in general client certificate checking. This way the whole stuff could be outside of caddy.

Feel free to open a PR for this. It'll be easier to discuss with a proof-of-concept implementation.

Gr33nbl00d commented 3 years ago

Ok i will. Like i wrote earlier im atm on vacation. I already finished parsing of crl for v1 and v2 two weeks ago. But due vacation i will continue on mid September :)

Gr33nbl00d commented 3 years ago

So today i finished signature validation of CRL. I supported all kind of ECDSA and RSA signatures based on OID definition in CRL. But i still have some points where i dont know how it is best solved.

My idea is to support multiple list of CRL locations in the configuration directly, but also support dynamic adding of CRLs via CRL Distribution Points (CDP) Extension in certificates. The question here is if dynamic CRL locations are added via CDP entry in the certificate i somehow need to check if we can trust the certificate from the server handing out the CRL. For that i need some kind of list of trusted root certificates which would authorize the ca certs from the CDP points. Do you think its enough to maintain a list of certificate files in the configuration which can be trusted? This is like a "truststore" Could look like this trusted_root_certificates {root1.pem root2.pem] or is there already some kind of solution for trusted root certs in caddy.

Also i think its a good idea to allow overriding of CDP locations as sometimes host names/IPs change even if the certificate is already handed out and still valid. For that i would like to implement something like cdp_location_overrides in configuration where you could override a location of a CRL defined inside the certificate so you can adopt the config if the IP/Host changes without changing all certificates handed out. It would be nice to here some feedback of users needing that feature to get a better understanding of the use cases. Otherwise i will implement it like described.

When finished i will work on the extraction into a seperate module by adding some possibilty to implement verifyClientCertificate inside a module

francislavoie commented 3 years ago

Could look like this trusted_root_certificates {root1.pem root2.pem] or is there already some kind of solution for trusted root certs in caddy.

Well, there's acme_ca_root but that's just for the ACME CA, not for all usecases. There's also tls_trusted_ca_certs, but that's just for the reverse_proxy handler, since it's a client to the upstream app.

I figure CRL stuff would be configured somewhere in the tls app and the trusted certs config would look largely similar to the aforementioned config options, but just for the CRL lookups.

Gr33nbl00d commented 3 years ago

Could look like this trusted_root_certificates {root1.pem root2.pem] or is there already some kind of solution for trusted root certs in caddy.

Well, there's acme_ca_root but that's just for the ACME CA, not for all usecases. There's also tls_trusted_ca_certs, but that's just for the reverse_proxy handler, since it's a client to the upstream app.

I figure CRL stuff would be configured somewhere in the tls app and the trusted certs config would look largely similar to the aforementioned config options, but just for the CRL lookups.

I will try to do it in a similar or equal way than the other solutions are done

Gr33nbl00d commented 3 years ago

For those who are interested why i had to do some extra work on the asn1 parsing: I did some final tests and measurements regarding CRL size and memory usage as well as performance tests. For testing i used a real large CRL from the department of defense (Email CA-59) (the largest i could find)

https://crl.gds.disa.mil/getcrlzip?DOD+EMAIL+CA-59

This CRL is about 48 MB

By parsing it with standard go way in a main method (Unmarshalling the whole list in one big chunk)

crlBytes, err := ioutil.ReadFile("DODEMAILCA_59.crl")
crl, err := x509.ParseCRL(crlBytes)

Result is following: After first line memory usage is about 50 MB thats simply the whole file into memory. After parsing (second line) memory usage grow to 423MB

My stream based version which uses partial unmarshalling does not exceed 8MB memory usage while parsing. So i am quite happy with the result.

Parsing times are quite equal 3.9 seconds versus 4.2 seconds with the stream based approach.

Gr33nbl00d commented 3 years ago

I am big steps further i did a first successfull CRL check today, but there is still a lot todo. There are some decisions to make and some questions i have you might can answer. Let me know what you think

For information i will first try to do a MVP release (phase 1) (minimum viable product) which cover most use cases. I will then add the other more rare extradorinary use cases in a second step (phase 2)

Question i could not find an answer to yet

The verifyPeerCertificate method has an array of certificate chains (array of array of certificates) verifiedChains [][]*x509.Certificate I was wondering under which circumstances there can be multiple chains. I would have expected only one chain. Can someone answer this question?

Design proposal and phases

The CRLDistributionPoints can contain multiple schemes to download the crl (HTTP,LDAP...) Phase 1: Only support HTTP not LDAP,FTP... If no HTTP distribution point is found fail the connection Phase 2: Consider to add LDAP or other schemes (Allow to configure a preference (prefer HTTP or prefer LDAP)
What happens if the CRL signature can not be validated? Phase 1: Make the connection fail Phase 2: Make it configurable to disable CRL signature checking
What happens if the CRL defined in CRLDistributionPoints can not be downloaded at all or in a specific time Phase 1: Make the connection fail Phase 2: Make it configurable to fail or ignore (might allow some 'grace period' epecially for the first connection attempt)
What happens if the CRL is outdated possible because no update could be downloaded due server downtime Phase 1: Ignore this circumstance Phase 2: Make it configurable to fail or ignore (might allow some 'grace period')
The CRLDistributionPoints inside the certificate can be outdated because of server movement url changes Phase 2: Allow overriding CRLDP locations via config
According to rfc5280 section 4.2.1.13. The CRL issuer does not need to be the certificate issuer. In most cases the CRL issuer is the certificate issuer. In that case we can be sure that we have the certificate necessary for CRL signture validation (inside the chain). In the other case we need to maintain a list of trusted CRL issuer certificates in some kind of truststore Phase 1: Assume CRL issuer is the cert issuer or at least part of the certificate chain. If the certificate can not be found in the chain fail the connection Phase 2: Allow to configure a list of trusted CRL issuer certificates

mholt commented 3 years ago

I was wondering under which circumstances there can be multiple chains. I would have expected only one chain. Can someone answer this question?

A cert bundle is just a pile of certificates, and there may be varying ways a chain of certs can be formed, depending on the algorithm. I'm not 100% sure what Go does without reading into it.

Gr33nbl00d commented 3 years ago

Thank you for your answer. Luckily i found the reason myself in a quite long article. I think i now know what to do with these multiple chains. https://utcc.utoronto.ca/~cks/space/blog/tech/TLSHowMultipleChains

Gr33nbl00d commented 3 years ago

I am nearly done with the pure implementation. Tests are still missing and some refactorings and cleanups still outstanding. So i am now at the point to decide if this should be part of caddy or extracted to a module. I will commit changes for review in next days If this should be a module than it would mean to split "tls configuration" Because the configuration needed for CRL/OCSP client cert checking would be in a different section in the config. If you consider after a review it might make sense to add it to caddy directly the config would be part of "client_authentication" section inside tls section

I would like to hear some review of my proposed config:

   "client_authentication": {
    "trusted_ca_certs_pem_files": [
      "./certificates/ca.crt",
    ],
    "mode": "require_and_verify",
    "revocation_check": {
      "mode": "prefer_ocsp,prefer_crl", 
      "crl_config": {
        "crl_work_dir": "./crls",  
        "crl_storage": "memory,disk", 
        "crl_signature_validation": "none,verify_log,verify", 
        "trusted_signature_cert_files": [
          "./certificates/crlca.crt"                                                        
        ],
        "crl_url_locations": [
          "http://server/my.crl"                                                          
        ],
        "crl_file_locations": [
          "./crls/my.crl"                                                                       
        ],
        "crl_update_interval": "10m",                                                
        "cdp_support": {                                                                    
          "cdp_fetch": "fetch_actively,fetch_background",                 
          "cdp_crl_required": true                                                      
        }
      }
    }
  }

mode Either preferr OCSP or CRL (currently ocsp support is outstanding) crl_work_dir Directory to store temporary CRL files and repositorys_ crl_storage Either store CRL entries in memory (map) or on disk (leveldb) crl_signature_validation Mode to verify signatures

verify_log log only if a signature validation failed
verify do not accept CRL if signature validation failed

trusted_signature_cert_files Certificates of trusted CRL issuers (if not in verified client cert chain) crl_url_locations Preconfigured urls containing crls crl_file_locations Preconfigured files cotaining crls crl_update_interval Interval to check for new crls on files/urls cdp_support Enable CDP certificate extension support (only http no ldap)

cdp_fetch Mode how to handle CDP CRL locations
fetch_actively: If CRL defined in CDP is not yet loaded fetch actively, TLS handshake will then wait for retrival or allow access depending on cdp_crl_required
fetch_background: If CRL defined in CDP is not yet loaded fetch in background. Client will be informed to try later or allow access depending on cdp_crl_required

cdp_crl_required If client cert has defined a CDP either deny connection/wait for retrival or allow access depending on cdp_fetch

francislavoie commented 3 years ago

Commenting on the config, comma separated string values is pretty awkward, I recommend using string slices for that instead. For example, "mode": "prefer_ocsp,prefer_crl" should probably be "mode": ["prefer_ocsp", "prefer_crl"] instead.

Since it's a pretty niche need to revocation, I think I'd rather this be an external module. I don't think @mholt or I would want to inherit maintenance of this functionality either, if it's merged into this repo. Doesn't feel good to merge code that we don't completely understand and feel we can fix bugs in, especially for security sensitive code.

What I suggest is that a "generic" API for this is designed such that we can add an extension point to the tls module to implement revocation strategies, or something like that. I.e. a new module namespace to cover this: https://caddyserver.com/docs/extending-caddy/namespaces. I'm not totally sure "when" revocation checks are triggered, so I'm not sure how that should look, but hopefully you have some ideas.

Gr33nbl00d commented 3 years ago

This was maybe a bit misleading. The comma seperation is only to show possible options. Only one option is possible of those commma seperated ones.

I can understand. It would also be much easier as the changes would be smaller for caddy itself if it is extracted into a module. So the code for you to review would be minimal. Also its easier for me to release updates.

I could make it quite generic and call it: caddytls.ClientCertValidator

It would be quite equal to the original verifyPeerCertificate method

type ClientCertValidator interface {     
    verifyClientCert(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error  
}

So the config would be still in the same place even if it is a module?

francislavoie commented 3 years ago

Okay, interesting. Maybe we could refactor client auth modes to be implemented as a module namespace, each mode being a module that implements that interface.

The existing modes could remain, and you could add your own like require_and_verify_with_revocation or something like that.

The shape of the config might need to change, but I think that's probably fine as long as the same behaviour remains.

Gr33nbl00d commented 3 years ago

I am still diving into the plugin concept so i hope i get it right. But what you mean is that would cleanup the connpolicy.go a bit so the 2 different verifyPeerCertificate would implement the interface ? I try to dive a bit deeper into this tomorrow :)

francislavoie commented 3 years ago

The relevant docs are here https://caddyserver.com/docs/extending-caddy#host-modules

Gr33nbl00d commented 3 years ago

Hi again :) One day later and i finally think i now have a bit better understanding. Ok one easy way to solve my problems is by adding this to the client_authentication

{
  "client_authentication": {
    "mode": "require_and_verify",
    "trusted_ca_certs_pem_files": [
      "./certificates/ca.crt"
    ],
    "client_cert_validations": {
      "revocation_list": {
      }
    }
  }
}

In the ClientAuthentication i would add following fields:

    RawClientCertValidations RawClientCertValidations `json:"client_cert_validations,omitempty" caddy:"namespace=tls.clientcertvalidations"`
    ClientCertValidations []ClientCertValidator `json:"-"`

This way additional modules providing a way to verify client certs can be added. Nothing else need to be changed. However if you look at the config it makes things a bit inconsistent. As the mode also defines "validations"

I think the real problem is inside go crypto funny that this was also done by adam langley ;) "mode" has some consistency problem it defines actually two things, request mode and validation mode cert_request_mode=no_request,request cert_validation_mode=no_validation,validation

If it was not this way it could have been implemented like this:

{
  "client_authentication": {
    "mode": "request",  --> defines if to request or not to request a client cert
    "client_cert_validations": {
      "require": {},                  ---> If configured checks if a cert is given and if not fails TLS handshake
      "validate_signature": {  ---> If client cert is present validate cert
        "trusted_ca_certs_pem_files": [
          "./certificates/ca.crt"
        ]
      },
      "revocation_list": {     ---> If client cert is present check crl
      }
    }
  }
}

But as this is a problem inside Go crypto itself not valueing SRP (Single Responsibility Principle) there is no easy way without completly changing the handshake algorithm. Only way would be that you configure only the "request mode" in go tls config this means only NoCLientCert or RequestClientCert. And do the validation (require cert/validate cert) yourself in different implementations of verifyPeerCertificate implementing ClientCertValidator

If you are ok i will prepare a pull request implementing it like described in first example. It is still possible to do changes like described below afterwards.

caddyserver / caddy