Extend ACME self check to check CAA records #1325
Conversation
This isn't needed since we now cache clients Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
jetstack-bot
added
dco-signoff: yes
jetstack-bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DanielMorsing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jetstack-bot
added
area/acme
| } | ||
| // TODO(dmo): figure out if missing CAA identity in directory | ||
| // means no CAA check is performed by ACME server or if any valid | ||
| // CAA would stop issuance (strongly suspect the former) |
There was a problem hiding this comment.
+1 I feel like the former is probably the case here.
@cpu do you have any insight?
There was a problem hiding this comment.
There's no absolute guarantee that if a CA doesn't include a caaIdentities field in the directory metadata that it won't check CAA. For instance, Let's Encrypt was checking CAA prior to the field being specified or included.
That said, if the CA doesn't include any caaIdentities there won't be a way for an ACME client to programmatically check whether issuance will pass a CAA check or not. I think for your purposes it would be correct to assume there's no CAA since you can't do much else besides hardcode ACME server URLs -> CAA identities as they arise (ick).
pkg/issuer/acme/dns/util/wait.go
Outdated
| var err error | ||
| for i := 0; i < 8; i++ { | ||
| //TODO(dmo): figure out if we need these servers to be configurable as well | ||
| msg, err = dnsQuery(queryDomain, dns.TypeCAA, RecursiveNameservers, true) |
There was a problem hiding this comment.
😅 I can totally see something going wrong here with split horizon DNS
| for i := 0; i < 8; i++ { | ||
| //TODO(dmo): figure out if we need these servers to be configurable as well | ||
| msg, err = dnsQuery(queryDomain, dns.TypeCAA, RecursiveNameservers, true) | ||
| if err != nil { |
There was a problem hiding this comment.
If a zone does not have a CAA record set, will it return SERVFAIL and cause issues?
There was a problem hiding this comment.
Do we use an old version of the miekg DNS library that turned certain "successful" DNS queries into errors? otherwise this error should only happen on actual communications failures
There was a problem hiding this comment.
If a zone returns SERVFAIL (or NOTIMPL) you would want to catch that in this precheck. Let's Encrypt would reject either case as an invalid CAA policy: https://letsencrypt.org/docs/caa/#servfail
|
|
||
| index := strings.Index(fqdn, ".") | ||
| if index == -1 { | ||
| panic("should never happen") |
There was a problem hiding this comment.
I'd rather us return an error here than panic - triggering a resync with backoff feels safer than ungraceful termination.
"I've never heard of a misbehaving DNS server before" - someone, somewhere
There was a problem hiding this comment.
So, this is all on our side. It turns the query
www.example.org. into
example.org. and then
org.
I think I can just remove it, but I wrote it as pure instinct because strings.Index going straight into an slice operation has bitten me in the past
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
|
|
||
| if !matchCAA(caas, issuerSet, iswildcard) { | ||
| // TODO(dmo): better error message | ||
| return fmt.Errorf("CAA record does not match issuer") |
There was a problem hiding this comment.
Including expected and actual here would probably suffice 😄
|
/retest |
|
Could you update this PR to also honour the |
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
|
/lgtm |
What this PR does / why we need it:
This PR implements a rudimentary CAA check for ACME clients, making sure that users don't run into LE rate limits with improperly set up DNS
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged):fixes #211
Special notes for your reviewer:
Release note: