revise dns_certify with new ACME / let's encrypt in mind #219
Conversation
|
once this is in and tested successfully, I'd like to go ahead and draft a release unless there are objections |
certify/dns_certify.ml
Outdated
| | Ok cert -> | ||
| if is_certificate tlsa then | ||
| (cert :: certs, cacerts) | ||
| else (* must be is_ca_certificate *) |
There was a problem hiding this comment.
does this comment suggest we should check for is_ca_certificate ?
There was a problem hiding this comment.
that's checked some lines above.. this code will only ever be executed if either is_certificate or is_ca_certificate is true.. checking it here again means I'd need to handle the impossible case of neither being true, which I'd like to avoid (but the code may be refactored for good to avoid these cases)
There was a problem hiding this comment.
I think it'd be good if the comment could explain that (I was a bit unsure if must be meant "we can logically derive that it is because we just checked" or "it really really should be, but we don't know if it is")
certify/dns_certify.ml
Outdated
| (cert :: certs, cacerts) | ||
| else (* must be is_ca_certificate *) | ||
| (certs, cert :: cacerts) | ||
| | _ -> acc |
There was a problem hiding this comment.
here we ignore the parsing Error _ ?
There was a problem hiding this comment.
yes, this is a shortcoming of the code (but the same behaviour as previously). maybe best to emit a log message..
…ate, is_ca_certificate, is_name since ACME v2 the certificate chain is part of the provisioning: - no need to hardcode the certificates in Dns_certificy_mirage anymore - use the CA certificates from dns (TLSA records again)
Co-Authored-By: C For C's Sake <cfcs@users.noreply.github.com>
…ns-client, dns-server, dns-tsig and dns-cli (4.4.0) CHANGES: * dns-stub, a new opam package, is a stub resolver mirage/ocaml-dns#209 @hannesm, review by @cfcs * embed IP address of recursive resolver only once mirage/ocaml-dns#214 @hannesm, fixes mirage/ocaml-dns#210, review by @cfcs * Dns_trie.lookup returns NotAuthoritative if no SOA is present mirage/ocaml-dns#217 @hannesm, review by @cfcs * Secondary server is looked up in trie properly (may be in another zone, which primary is not authoritative for the other zone) mirage/ocaml-dns#217 @hannesm, review by @cfcs * new function Dns.Dnskey.pp_name_key mirage/ocaml-dns#218 @hannesm, review by @cfcs * dns-certify uses new ACME protocol (where the intermediate certificate is part of the issuance process) mirage/ocaml-dns#219 @hannesm, review by @cfcs * dns-certify/dns-tsig/dns-cli: use mirage-crypto mirage/ocaml-dns#219 @hannesm, review by @cfcs
the main change is that let's encrypt now very conveniently provides the chain of trust as well as the end-entity certificate. this means that we no longer need to carry the hard-coded certificate around. but this implies that we need to store the CA chain somewhere (again, as TLSA records :)
I'm still not entirely happy with the dns_certify API, but I think this is moving into the right direction.