starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 11:15:54 +03:00

[GH-ISSUE #2211] should `proto::rr::resource::Record.rdata` really be an `Option`? #924

New issue

Closed

opened 2026-03-16 00:58:38 +03:00 by kerem · 6 comments

kerem commented

2026-03-16 00:58:38 +03:00

Owner

Originally created by @japaric on GitHub (May 14, 2024).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2211

take for example the concrete type Record<RRSIG>, under what circumstances would its rdata field be None? To me that seems like a malformed / invalid value. if the type of the rdata is known then that .rdata field ought to be populated

In my mind, the representation of Record should be:

pub struct Record<R: RecordData = RData> {
    name_labels: Name,
    // rr_type: RecordType, // <- not needed as it can be derived from `.rdata`
    dns_class: DNSClass,
    ttl: u32,
    rdata: R, // <- `R` instead of `Option<R>`
    #[cfg(feature = "mdns")]
    mdns_cache_flush: bool,
    #[cfg(feature = "dnssec")]
    proof: Proof,
}

The record_type getter should defer the work to self.rdata.record_type

impl Record<R: RecordData> {
    pub fn record_type(&self) -> RecordType {
        RecordData::record_type(&self.rdata)
    }
}

The set_record_type method should not exist.

The set_data method should take an R value instead of an Option<R>. its debug_assertion becomes unnecessary because set_record_type does not exist.

All this works fine in presence of the RData enum. Replacing the rdata with a different record-type data causes the output of the record_type getter to be automatically updated:

let mut record: Record<RData>;
let new_data: RData;

// pre-conditions
assert_eq!(RecordType::RRSIG, record.record_type());
assert!(matches!(new_data, RData::A(_)));

record.set_data(new_data);

// this gets updated
assert_eq!(RecordType::A, record.record_type());

For unknown / unsupported record types the rdata can be represented as a buffer of bytes as they appear on the wire. the wire-level representation of a record contains a RDLENGTH field that indicates how long the following RDATA field is. the existing RData::Unknown variant already does that.

am I missing something? is Record.rdata = None being used in practice?

Originally created by @japaric on GitHub (May 14, 2024). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2211 take for example the concrete type `Record<RRSIG>`, under what circumstances would its `rdata` field be `None`? To me that seems like a malformed / invalid value. if the type of the `rdata` is known then that `.rdata` field ought to be populated In my mind, the representation of `Record` should be: ``` rust pub struct Record<R: RecordData = RData> { name_labels: Name, // rr_type: RecordType, // <- not needed as it can be derived from `.rdata` dns_class: DNSClass, ttl: u32, rdata: R, // <- `R` instead of `Option<R>` #[cfg(feature = "mdns")] mdns_cache_flush: bool, #[cfg(feature = "dnssec")] proof: Proof, } ``` The `record_type` getter should defer the work to `self.rdata.record_type` ``` rust impl Record<R: RecordData> { pub fn record_type(&self) -> RecordType { RecordData::record_type(&self.rdata) } } ``` The `set_record_type` method should not exist. The `set_data` method should take an `R` value instead of an `Option<R>`. its `debug_assertion` becomes unnecessary because `set_record_type` does not exist. All this works fine in presence of the `RData` enum. Replacing the `rdata` with a different record-type data causes the output of the `record_type` getter to be automatically updated: ``` rust let mut record: Record<RData>; let new_data: RData; // pre-conditions assert_eq!(RecordType::RRSIG, record.record_type()); assert!(matches!(new_data, RData::A(_))); record.set_data(new_data); // this gets updated assert_eq!(RecordType::A, record.record_type()); ``` For unknown / unsupported record types the `rdata` can be represented as a buffer of bytes as they appear on the wire. the wire-level representation of a record contains a `RDLENGTH` field that indicates how long the following `RDATA` field is. the existing `RData::Unknown` variant already does that. --- am I missing something? is `Record.rdata = None` being used in practice?

kerem

2026-03-16 00:58:38 +03:00

closed this issue
added the
crate:all

cleanup
labels

kerem commented

2026-03-16 00:58:55 +03:00

Author

Owner

@djc commented on GitHub (May 14, 2024):

This sounds reasonable to me. This might be an oversight from when we migrated to the generically-typed RData setup, or it might be necessary in some contexts to take out the rdata value without having ownership of the Record? I think staging an attempt at simplifying here would be helpful. (@bluejekyll might have more context, but might also take longer to respond...)

@djc commented on GitHub (May 14, 2024): This sounds reasonable to me. This might be an oversight from when we migrated to the generically-typed `RData` setup, or it might be necessary in some contexts to take out the `rdata` value without having ownership of the `Record`? I think staging an attempt at simplifying here would be helpful. (@bluejekyll might have more context, but might also take longer to respond...)

kerem commented

2026-03-16 00:59:00 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 14, 2024):

There are some cases where the RDATA is unspecified yet the record-type is. This happens in the dns update sections of the code. That’s mostly why it ended up being Option.

I couldn’t figure out an elegant way to deal with that case.

@bluejekyll commented on GitHub (May 14, 2024): There are some cases where the RDATA is unspecified yet the record-type is. This happens in the dns update sections of the code. That’s mostly why it ended up being Option. I couldn’t figure out an elegant way to deal with that case.

kerem commented

2026-03-16 00:59:05 +03:00

Author

Owner

@japaric commented on GitHub (May 15, 2024):

There are some cases where the RDATA is unspecified yet the record-type is. This happens in the dns update sections of the code. That’s mostly why it ended up being Option.

by "unspecified" do you mean that the RDATA contents do not match what's specified in the record-type header field or may even be completely absent? can that unspecified state be serialized / deserialized into wire format (bytes)? or is it a state that's limited to e.g. the construction / build stage of a resource record?

if the state is limited to a section of the code, we could have two Record types, a specified one and an unspecified one, that share a lot of code save for the RDATA getter / setter.

struct SpecifiedRecord<RDATA> {
    header: RecordHeader,
    rdata: RDATA,
}

struct UnspecifiedRecord {
    header: RecordHeader,
    // `.record_type` is allowed to diverge from `.rdata`
    record_type: RecordType,
    rdata: Option<Unknown>, // or maybe Option<RData>
}

struct RecordHeader {
    name: Name,
    ttl: u32,
    // ..
}

if you could point out where in code the unspecified state happens, that would help me better understand how it's being used

@japaric commented on GitHub (May 15, 2024): > There are some cases where the RDATA is unspecified yet the record-type is. This happens in the dns update sections of the code. That’s mostly why it ended up being Option. by "unspecified" do you mean that the RDATA contents do not match what's specified in the record-type header field or may even be completely absent? can that unspecified state be serialized / deserialized into wire format (bytes)? or is it a state that's limited to e.g. the construction / build stage of a resource record? if the state is limited to a section of the code, we could have two `Record` types, a specified one and an unspecified one, that share a lot of code save for the RDATA getter / setter. ``` rust struct SpecifiedRecord<RDATA> { header: RecordHeader, rdata: RDATA, } struct UnspecifiedRecord { header: RecordHeader, // `.record_type` is allowed to diverge from `.rdata` record_type: RecordType, rdata: Option<Unknown>, // or maybe Option<RData> } struct RecordHeader { name: Name, ttl: u32, // .. } ``` if you could point out where in code the unspecified state happens, that would help me better understand how it's being used

kerem commented

2026-03-16 00:59:10 +03:00

Author

Owner

@japaric commented on GitHub (May 15, 2024):

if you could point out where in code the unspecified state happens

I found the relevant code in crates/proto/op/update_message.rs which implements RFC2136 (DNS update). After some initial sketching I think I'm leaning towards creating a separate UpdateRecord, which accepts an Option-al .rdata field, and a separate UpdateMessage, which better aligns with the DNS message structure described in RFC2136 (section 2); and then dedicating the existing Record, tweaked to have a non-Option-al .rdata field, and Message to QUERY-like logic (RFC1035).

Furthermore, RFC4033 states that "The DNS security extensions provide data and origin authentication for DNS data. The mechanisms outlined above are not designed to protect operations such as zone transfers and dynamic update (RFC2136, RFC3007)" so I think it makes sense to not overload the Message struct, which can include DNSSEC signatures, with DNS update functionality, which is unrelated to DNSSEC.

@japaric commented on GitHub (May 15, 2024): > if you could point out where in code the unspecified state happens I found the relevant code in `crates/proto/op/update_message.rs` which implements RFC2136 (DNS update). After some initial sketching I think I'm leaning towards creating a separate `UpdateRecord`, which accepts an `Option`-al `.rdata` field, and a separate `UpdateMessage`, which better aligns with the DNS message structure described in RFC2136 (section 2); and then dedicating the existing `Record`, tweaked to have a non-`Option`-al `.rdata` field, and `Message` to QUERY-like logic (RFC1035). Furthermore, RFC4033 states that "The DNS security extensions provide data and origin authentication for DNS data. The mechanisms outlined above are not designed to protect operations such as zone transfers and dynamic update (RFC2136, RFC3007)" so I think it makes sense to not overload the `Message` struct, which can include DNSSEC signatures, with DNS update functionality, which is unrelated to DNSSEC.

kerem commented

2026-03-16 00:59:15 +03:00

Author

Owner

@djc commented on GitHub (May 15, 2024):

Strong 👍 from me!

@djc commented on GitHub (May 15, 2024): Strong 👍 from me!

kerem commented

2026-03-16 00:59:20 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 15, 2024):

I’ve tried to go down this route in the past. I’m open to see where your investigation leads. There are a lot of areas of the code that rely on Message and Record, so it might end up being a large change.

@bluejekyll commented on GitHub (May 15, 2024): I’ve tried to go down this route in the past. I’m open to see where your investigation leads. There are a lot of areas of the code that rely on Message and Record, so it might end up being a large change.

kerem referenced this issue

2026-03-16 02:25:44 +03:00

[PR #924] [MERGED] Bump radix_trie from 0.1.5 to 0.1.6 #1793