Skip to content

Support DNS messages with RFC 8490 DSO-Data - #1672

Closed
Kentzo wants to merge 3 commits into
miekg:masterfrom
Kentzo:dsorr
Closed

Support DNS messages with RFC 8490 DSO-Data#1672
Kentzo wants to merge 3 commits into
miekg:masterfrom
Kentzo:dsorr

Conversation

@Kentzo

@Kentzo Kentzo commented Aug 13, 2025

Copy link
Copy Markdown
Contributor

In this PR I used custom, non-RR, type to directly represent DSO data in Msg.

Msg.Pack doesn't stand in user's way if they want to pack DSO message with RRs. On the other hand, Msg.Unpack won't attempt DSO unpacking unless true number of RRs is zero.

@Kentzo
Kentzo requested review from miekg and tmthrgd as code owners August 13, 2025 23:36
@Kentzo
Kentzo force-pushed the dsorr branch 2 times, most recently from b31f384 to 49d0bf3 Compare August 14, 2025 00:08
Comment thread msg.go
if isDSOCompressible(dns) {
return true
}
fallthrough

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is done in spirit that if user wants to pack an invalid DSO message (with both DSO-Data and RR-data) we should not disallow compression.

@Kentzo
Kentzo force-pushed the dsorr branch 5 times, most recently from 58f1095 to 586be66 Compare August 14, 2025 02:31
Comment thread dso.go

// RFC 8490, Section 7.3: Encryption Padding TLV
type DSOEncryptionPadding struct {
Padding []byte

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This seems wasteful when messages originate from the user as it going to be just slices of N '\0' bytes. I suppose this could be replaced with some "sparse" structure.

@Kentzo

Kentzo commented Aug 14, 2025

Copy link
Copy Markdown
Contributor Author

fwiw, of 1212 added lines, there are 126 are "newlines" and 222 are comments.

@miekg

miekg commented Aug 16, 2025

Copy link
Copy Markdown
Owner

isDSOCompressible(dns)

DSO has compression??

(still 1200+ lines ...if inclined to review, I'm spending less then an hour on this lib per week, so we're talking early next year)

@Kentzo

Kentzo commented Aug 16, 2025

Copy link
Copy Markdown
Contributor Author

Correct. See RFC 8765, Push TLV.

still 1200+ lines ...if inclined to review, I'm spending less then an hour on this lib per week, so we're talking early next year

no worries and no rush. Im experimenting in my fork and happy to assist.

@miekg

miekg commented Aug 16, 2025 via email

Copy link
Copy Markdown
Owner

@Kentzo

Kentzo commented Aug 16, 2025

Copy link
Copy Markdown
Contributor Author

Msg.isCompressible as written relies on *Count to determine whether Msg os compressible in principle. This check is useless for DSO messages, because counts are 0.

Although, since this is a performance optimization (to avoid allocation of compression tables needlessly), it could suffice to consider any OpcodeStateful message as compressible for the sake of simplicity.

@miekg

miekg commented Aug 16, 2025 via email

Copy link
Copy Markdown
Owner

@Kentzo

Kentzo commented Aug 16, 2025

Copy link
Copy Markdown
Contributor Author

*please see edit to the message above.

still I don't think such a niche DNS feature should warrant such a code
increase in this lib.

The readme says support of rfc 8490, hence my interest to actually help implementing it. I understand if you don’t want, but then I’m confused by the claim.

We can have a discussion on this - should have happened first

I wanted to have full working implementation (via CoreDNS plugin) and intended to present my work only then. I decided to publish this (and previous related one) PR after learning of your work on dnsv2 just to give you an idea what might be needed for proper DSO support, as claimed in readme.

By no means the PR is final and I’m happy to rearrange as necessary.

However, since you have little to no time to dig into RFCs and read other implementations, perhaps it’s more efficient to do back-and-forth via PRs? Easier to discuss code at hand than abstract ideas.

@miekg

miekg commented Aug 18, 2025

Copy link
Copy Markdown
Owner

there are many, many unfleshed things that are done in this PR and I'm not going add this anytime soon and probably never (in this repo).

In codeberg's version there are/were some issues that start from the very beginning on how this might be possible or not.

Even super basic things like: everything guard against question != 1, how do you propose the cleanly fix that and do we need this (and maybe more - dont think its my task to enumerate these)

@miekg miekg closed this Aug 18, 2025
@Kentzo

Kentzo commented Aug 18, 2025

Copy link
Copy Markdown
Contributor Author

Whether this lib needs guards / validation checks is up to your consideration, of course. User’s code will need them, because the spec often requires to “forcibly disconnect” and it seems prudent to drop a poor actor as soon as possible. But I have no objections to omit them from this pkg entirely.

I will wait until you figure out a way to cleanly fold edns0, svcb and dso into pseudo in dnsv2. Don’t want to further clash with your process.

@Kentzo
Kentzo deleted the dsorr branch August 23, 2025 01:41
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.

2 participants