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

Introduce a new base module #474

Open
wants to merge 95 commits into
base: main
Choose a base branch
from
Open

Introduce a new base module #474

wants to merge 95 commits into from

Conversation

bal-e
Copy link
Contributor

@bal-e bal-e commented Dec 26, 2024

This introduces a new_base module as a potential candidate for replacing base. While it has a similar structure (although much is left to be filled out), it has three distinguishing features:

  1. It builds the foundation for a derive-based interface for easily and efficiently parsing (and possibly building) DNS messages.

  2. It introduces RevName, a more efficient type for domain names (where labels are stored in reverse order).

There's a lot more work to be done:

  • Define some record data types (probably those from RFC1035).
  • Define the OPT record data, and some EDNS option types.
  • Define some common message structures (including a generic fallback).
  • Update existing parts of the codebase to show the PR's benefits.
    • The client transports (... while remaining compatible?).
    • The stub resolver.
  • Try defining some derive macros for the parsing/building traits.

Note: this is a new version of #469 with a less problematic branch name.

bal-e added 30 commits December 6, 2024 16:46
It implements 'Hash' for the provided integer types.
@bal-e bal-e marked this pull request as ready for review January 20, 2025 16:54
@bal-e bal-e requested a review from a team January 20, 2025 16:54
fn parse_from_message(
message: &'a Message,
start: usize,
) -> Result<Self, ParseError> {
Copy link
Member

Choose a reason for hiding this comment

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

Why not call split_from_message and check that rest is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While that would be a valid implementation, I am hoping to define new proc macros for SplitFromMessage and ParseFromMessage soon (in the same fashion as SplitBytes and ParseBytes). I chose to define this method with a bit more boilerplate so that I can refer to it as a template when writing the proc macro.

#[derive(Clone, BuildBytes, ParseBytes, SplitBytes)]
pub struct Question<N> {
/// The domain name being requested.
pub qname: N,
Copy link
Member

Choose a reason for hiding this comment

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

I think for names there needs to be some underlying trait that specifies that N can parse and build compressed DNS names. Otherwise, for example, compressed and uncompressed names can get confused or other confusion may arise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is strongly expected that any domain name type (at least those in this crate) will implement ParseFromMessage, SplitFromMessage and BuildIntoMessage using name (de)compression. It is currently possible to use an incorrect type here, but I believe the risks of such a confusion are quite low. In the future, if we introduce new name types which do not follow these conventions, we can introduce e.g. a Name trait.

#[cfg(all(feature = "std", feature = "siphasher"))]
use crate::new_base::wire::{AsBytes, TruncationError};

//----------- CookieRequest --------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

I find this very confusing. There are two types of cookies, short cookies that only contain the client's cookie and longer cookies that contain both the client and the server parts. There are two roles, the client and the server.

It is unclear to me what is what and who does what.

Copy link
Contributor Author

@bal-e bal-e Jan 22, 2025

Choose a reason for hiding this comment

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

A CookieRequest is a "client coookie", which requests a Cookie ("server cookie") from the server. I tried these names out as I found them more intuitive than ClientCookie and ServerCookie, but I'm happy to switch to the conventional names.

Copy link
Member

Choose a reason for hiding this comment

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

Clients send two types of cookies, the small one and the big. And receive only the big one. Servers receive small and big and send only big.

Another issue might be that if a client receives a server cookie, we cannot assume anything about what the server put there. The server might just send some random bytes.

#[repr(transparent)]
pub struct Txt {
/// The text strings, as concatenated [`CharStr`]s.
content: [u8],
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong. A txt record is a collection of strings. Boundaries need to be preserved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CharStrs are the DNS <character-string>s and always start with a length octet, so boundaries are preserved.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Maybe an extra comment with what you just wrote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A PR that includes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants