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

Make RR::CDS (resp RR::CDNSKEY) subclass of RR::DS (resp RR::DNSKEY) #156

Closed
wants to merge 3 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Oct 6, 2022

Purpose

Instead of implementing specific methods, reuse the one from Zonemaster::LDNS::RR::DS and Zonemaster::LDNS::RR::DNSKEY since a CDS (resp CDNSKEY) record is identical to a DS (DNSKEY) record, see RFC 7344 section 3.1 and 3.2.

Context

#114

Changes

  • make Zonemaster::LDNS::RR::CDS a subclass of Zonemaster::LDNS::RR::DS
  • make Zonemaster::LDNS::RR::CDNSKEY a subclass of Zonemaster::LDNS::RR::DNSKEY

How to test this PR

Unit tests are provided.

@ghost ghost requested review from mattias-p, matsduf and vlevigneron October 6, 2022 10:57
@ghost ghost changed the title Make RR::CDS (resp CDNSKEY) subclass of RR::DS (resp RR::DNSKEY) Make RR::CDS (resp RR::CDNSKEY) subclass of RR::DS (resp RR::DNSKEY) Oct 6, 2022
@ghost
Copy link
Author

ghost commented Oct 6, 2022

It seems the keytag method can't be used for CDNSKEY because it relies on ldns_calc_keytag() that does not accept CDNSKEY type (see https://github.com/NLnetLabs/ldns/blob/1.8.3/dnssec.c#L287-L291)

uint16_t
ldns_calc_keytag(const ldns_rr *key)
{
        ...
	if (ldns_rr_get_type(key) != LDNS_RR_TYPE_DNSKEY &&
	    ldns_rr_get_type(key) != LDNS_RR_TYPE_KEY
	    ) {
		return 0;
	}
        ...
}

Applying this patch in libldns would fix the issue:

diff --git a/dnssec.c b/dnssec.c
index fbaa518a..63bdab09 100644
--- a/dnssec.c
+++ b/dnssec.c
@@ -285,7 +285,8 @@ ldns_calc_keytag(const ldns_rr *key)
        }
 
        if (ldns_rr_get_type(key) != LDNS_RR_TYPE_DNSKEY &&
-           ldns_rr_get_type(key) != LDNS_RR_TYPE_KEY
+           ldns_rr_get_type(key) != LDNS_RR_TYPE_KEY &&
+           ldns_rr_get_type(key) != LDNS_RR_TYPE_CDNSKEY
            ) {
                return 0;
        }

@matsduf
Copy link
Contributor

matsduf commented Oct 6, 2022

It seems the keytag method can't be used for CDNSKEY because it relies on ldns_calc_keytag() that does not accept CDNSKEY type (see https://github.com/NLnetLabs/ldns/blob/1.8.3/dnssec.c#L287-L291)
(...)

Applying this patch in libldns would fix the issue:
(...)

Do you see any work-around, or must LDNS be fixed first? Have you created an issue or a PR there?

@matsduf matsduf added this to the v2022.2 milestone Oct 6, 2022
@ghost
Copy link
Author

ghost commented Oct 6, 2022

I've opened NLnetLabs/ldns#187

@matsduf matsduf modified the milestones: v2022.2, v2023.1 Nov 8, 2022
@matsduf matsduf modified the milestones: v2023.1, v2023.2 May 2, 2023
@matsduf matsduf modified the milestones: v2023.2, v2024.1 Dec 4, 2023
@ghost ghost closed this by deleting the head repository Feb 9, 2024
This pull request was closed.
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.

1 participant