Fix CAA records whitespace splitting with additional optional properties #92
Conversation
- Currently, CAA records with whitespace inside of the value will fail
to tuple unpack, because split expects only two spaces.
- Added fix to force split to only split twice, instead of as many spaces
as are detected, since a valid CAA record shouldn't have more than two
whitespace characters separating between the flag, tag and value.
- As per RFC 6844, optional parameters are defined as the below:
```
5.2. CAA issue Property
The issue property tag is used to request that certificate issuers
perform CAA issue restriction processing for the domain and to grant
authorization to specific certificate issuers.
The CAA issue property value has the following sub-syntax (specified
in ABNF as per [RFC5234]).
issuevalue = space [domain] space [";" *(space parameter) space]
domain = label *("." label)
label = (ALPHA / DIGIT) *( *("-") (ALPHA / DIGIT))
space = *(SP / HTAB)
parameter = tag "=" value
tag = 1*(ALPHA / DIGIT)
value = *VCHAR
For consistency with other aspects of DNS administration, domain name
values are specified in letter-digit-hyphen Label (LDH-Label) form.
```
- This allows for records that look like the below:
```
CAA 0 issue "cert.example.com; cansignhttpexchanges=yes"
```
- Added property 'cansignhttpexchanges=yes' to CAA record test. - Whitespace is added as this is the example provided in RFC 6844. - This change addresses failure to tuple unpack, caused by whitespaces in optional issuer parameters, when following RFC conventions.
There was a problem hiding this comment.
Interesting. I guess this is part of the CAA spec? I see some similar bits after a quick look at https://datatracker.ietf.org/doc/html/rfc6844#section-3, but hadn't run across them in the wild before.
We may need to look into supporting these in the core record type and it's rdata processing. https://github.com/octodns/octodns/blob/ed13d2db52b4127f1035a8927a396467219b9918/octodns/record/caa.py#L16 as it would appear to have the same issue.
Just treating anything else in the last section as part of the value seems like a workable way to go about this, but I'd need to think about it for a bit as to whether it'd be better to separate the tags out into a dict, e.g.
foo:
type: CAA
values:
- flags: 0
tag: issue
value: ca.unit.tests
params:
cansignhttpexchanges: yesAny thoughts on that?
Even though route53's current implementatiomn is not using octodns core's rdata parsing it'd be best to keep them in sync so would like to hold off on this until there's a broader decision on the path.
/cc @octodns/review for thoughts?
|
Thanks for that Ross, I did think it was possible that this was happening on other providers, but didn't have the time to check. I noticed this when trying to run I would agree that having a YAML To be clear, the main widely supported optional property that I am aware of is the one I used in the test, (it's required for AMP), but a CA can add any properties they want in their implementation of issuing certificates, and if there are CAA records present on a domain that don't implement the additional property, then they can't issue according to the spec. So additional properties might be more heavily used with something like an internal self-hosted CA authority at a company. Another potential fix we could employ is something like the following line [1], which works for all records that Route53 accepts. (The validation inside of Route53 specifically does not allow for any additional spaces outside of the quotation marks, and never allows more than the It's a bit cleaner as we just let the tuple unpacking inherit all remaining values from the split, (and if you did want to add a params section, then that could be done by getting rid of the join and only keeping I hope that all makes sense, and if not, please let me know so I can fix up anything ambiguous. Thanks! |
There was a problem hiding this comment.
Good point. Guess it's similar to SPF values in TXT records where there's data encoded into the value itself. Makes sense to leave these alone and just use your splitting logic. will do the same over in octoDNS core (and potentially other modules that haven't yet moved over to using the core's rdata handling.)
|
Looks like formatting is off. |
|
Hi Ross, Thanks for your help, I've fixed it, please try the CI again. (I didn't notice the git precommit hook failing, as I run NixOS, I have to modify any scripts that use shebangs to instead use a Nix shebang.) Next time, I'll set up a Fedora dev container to make this a bit easier. Cheers, |
I would follow jmittermair's argument
This would make it easy to write (and verify) an ABNF-based validation that is close to the spec.
It would be great if we could adopt the proposed patch to the octoDNS core and then invite the route53 (or other provider's) contributors to switch to using the core's rdata capabilities. |
|
octodns/octodns#1171 has shipped to address this in octoDNS core. |
That's been on my TODO list for quite a while, but it hasn't bubbled up to the top. |
[1] https://www.rfc-editor.org/rfc/rfc6844.txt
split(..., maxsplit)parameter, to ensure that we only split the first 2 whitespace characters between the FIELD, TAG and VALUE, whilst leaving any additional whitespace untouched.Note:
(This is my first public pull request -- lint was successful and test coverage passes. Please let me know if I have missed anything).