On Thu, 30 Mar 2017 21:47:34 +0200 Jan Klemkow <[email protected]> wrote: > I'm not a developer (just a contributor), but I worked on httpd client > certs a year ago, too. (https://marc.info/?t=145285926100003&r=1&w=2)
Interesting. Thanks Jan, I hadn't seen your earlier diffs before (my fault -- I guess I should have searched further back in the list archives to begin with) > I got a private response from a developer, who had an own similar diff > in preparation. He told me that is better to name configuration > option "client ca <file>" instead of "ca <file>". I just went with single words for consistency: apart from "ticket lifetime", all the other tls options were single words. It's just semantics though: "client ca <file>" sounds fine to me too, happy to use that instead. What's more interesting is learning now that there were two of you working on this a year ago. I'm curious as to why that didn't proceed -- strategic decision not to go down that path, or waiting for OCSP support in libtls (which arrived more recently), or the project just had other priorities? > You could adapt my regression test to make sure the feature does not > break in future development: > https://marc.info/?l=openbsd-tech&m=146289968117988&w=2 Good idea. Can do. My only comment on tests/args-tls.pl in your diff is that it might be better to test client TLS connections both with & without client TLS certs (I haven't tried your diff, but a quick read show that it just modified an existing TLS test to use a client cert). > I had not time to test your diff, but I like it. I also need this > feature in httpd. Thanks. Your diff & mine have quite a bit in common, but there's a few points of difference worth noting: I think passing through TLS_PEER_* to fastcgi (as my diff did) is pretty much essential (even if we end up checking revocation status in httpd itself or in libtls, instead of leaving it to the fastcgi responders), as authentication usually goes hand-in-hand with authorisation, so fastcgi responders need some way (ideally issuer, DN & serial number, but issuer, DN & cert hash is at least a start) of knowing *which* certificate was used for authentication. Your diff defined tls options for both mandatory (request & require) and optional (request but do not require) client cert checking. Mine only defined a tls option for mandatory checking -- mainly because I've never yet seen a real world use case for optional client cert checking that couldn't be implemented more coherently (not to mention more safely) some other way... but perhaps I was wrong. Is the optional mode worth having? If so, why? Your diff used chunked transfers for config_{get,set}tls(), to avoid the hard limit of imsg transfers. In my local tests I had only two certs (for root & subordinate CAs) in the client CA file (and they were only throwaway certs for testing, with relatively small moduli), so I didn't run into the imsg size limit. But if the chain or the certs in it were any longer, then yes the size limit would cause problems. Having seen your code now, I think your chunked transfer approach makes good sense. Is it worth putting forward another diff with those changes now, or should I wait to hear whether the feature would be considered welcome or not first? > In response of your second mail: > I prefer the Idea to handle revocations by a CRL which is checked by > libtls in case of client certificates. Thanks. Yes, I agree that CRLs are *probably* the way to go here (OCSP being a bit problematic for client certs in most, but not all, circumstances). I can also see the benefit of handling local CRL file checking in libtls (reusable beyond just httpd), but wonder if that needs to be balanced against the flexibility of delegating revocation checks to the fastcgi responders (as some sites -- e.g. where there are no WAN links involved -- might be better served by doing online OCSP, whereas most others will likely prefer local CRL files -- so passing through both URIs from the cert, plus the serial number would allow greater choice). That's probably the matter I'd like some guidance from the devs on the most -- as it's almost more about policy than about the code. > btw: As far as I know, there is a source tree lock at the moment. > Thus, it may take some time to integrate you diff. Thanks. Yes, I've heard that and am definitely NOT trying to get this into 6.1! But I still think it's worth having these discussions now, so that by the time the 6.1 release is done & dusted we might have something to add that's actually worth committing, rather than just my first cut diff and a few initial thoughts...
