Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow parsing of CAA Resource Record (Closes #292) #360

Merged
merged 9 commits into from Sep 26, 2020

Conversation

lxdicted
Copy link
Contributor

CAA (Certification Authority Authorization) was introduced in RFC 6844.
This has been obsoleted by RFC 8659. This commit added the possibility
to query CAA resource records with adig(1) and adds a parser for CAA
records, that can be used in conjunction with ares_query(3).

CAA (Certification Authority Authorization) was introduced in RFC 6844.
This has been obsoleted by RFC 8659. This commit added the possibility
to query CAA resource records with adig and adds a parser for CAA
records, that can be used in conjunction with ares_query(3).
@coveralls
Copy link

coveralls commented Sep 12, 2020

Coverage Status

Coverage decreased (-0.1%) to 88.618% when pulling 840a0f7 on lxdicted:feature/caa into 14933d3 on c-ares:master.

@bradh352
Copy link
Member

Can you add a test. case for this? Also, please look at the warnings generated during build. I'm not quite sure why travis failed to build, but AppVeyor looks to me like something to do with the whitespace in Makefile.inc so it shouldn't be a big deal.

@bradh352
Copy link
Member

bradh352 commented Sep 15, 2020

overall, it looks pretty good. The only things I see are you're missing a manpage for ares_parse_caa_reply(), and you should have some tests structured through DNSPacket rather than outputting the entire binary stream so its more comprehensible if something goes wrong in the future for someone debugging.

@bradh352
Copy link
Member

in docs/Makefile.inc need to add the html and pdf targets that are auto-generated from your .3 file

@bagder
Copy link
Member

bagder commented Sep 17, 2020

(I just submitted #362 to get rid of the html and pdf targets...)

@lxdicted
Copy link
Contributor Author

@bradh352 : I cannot see the benefit of adding additional test code (DNSPacket), it only generates the binary stream, that is present already. Is this a hard requirement to get this merged?

@bradh352
Copy link
Member

It's really for future debugability when things change in the future. It's much harder to understand the entire protocol stream when you really only care about a small part of it.

That said, I'm not sure if we should hold merging up for that or not. @bagder thoughts?

@bradh352
Copy link
Member

@bagder just looking for someone else to weigh in on the test case not using DNSPacket, if that's acceptable or too much of a future burden. Thanks!

@bagder
Copy link
Member

bagder commented Sep 26, 2020

I'm fine with that, as we can always add more and fine-tune the tests going forward if we need to. (Said by someone who's flying a bit off from the c-ares project right now.)

@bradh352 bradh352 merged commit f5b4c08 into c-ares:master Sep 26, 2020
3 checks passed

caa_curr->critical = (int)*strptr++;
caa_curr->plength = (int)*strptr++;
if (caa_curr->plength <= 0 || (int)caa_curr->plength >= rr_len - 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OSS-Fuzz is reporting a use-of-uninitialized-value here, which I suspect is because the two lines above don't do any length checks to confirm that strptris still within range [abuf, abuf+alen).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed in 4d6975b

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that did the trick – thanks.

@augjoh
Copy link

augjoh commented Oct 23, 2020

@bagder : Integration of this functionality in nodejs seems to be blocked (nodejs/node#35466). When do you plan to release the next version of c-ares?

@bradh352
Copy link
Member

I was thinking about that recently. I think there's just 1 bug I'd like to resolve before release, and that would be #317

sergepetrenko pushed a commit to tarantool/c-ares that referenced this pull request Jul 29, 2022
CAA (Certification Authority Authorization) was introduced in RFC 6844.
This has been obsoleted by RFC 8659. This commit added the possibility
to query CAA resource records with adig and adds a parser for CAA
records, that can be used in conjunction with ares_query(3).

Closes Bug: c-ares#292
Fix By: Daniela Sonnenschein (@lxdicted)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants