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

Authority can depend on client source ipnet #1994

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

Conversation

dxfgg17
Copy link

@dxfgg17 dxfgg17 commented Jul 26, 2023

I thought it could be useful to answer differently to clients depending on their source ip network.

There is no breaking change (existing functions still work the same way) but I added the two following functions in the catalog.rs file:

pub fn upsert_for_ipnet(
    &mut self,
    ipnet: IpNet,
    name: LowerName,
    authority: Box<dyn AuthorityObject>,
)

pub fn remove_for_ipnet(
    &mut self,
    ipnet: &IpNet,
    name: &LowerName,
) -> Option<Box<dyn AuthorityObject>>

Now, authorities are stored like this:

authorities: vec![(None, HashMap::new())],

There is a small performance overhead when adding an AuthorityObject because the vector has to be sorted to have at the first position the smallest IpNet and last position the biggest one (which is None, corresponding to all networks and to the current behaviour, all networks).

If this is not a wanted behavior or functionnalty feel free to delete this, I won't be offended it was just something I needed :)

@bluejekyll
Copy link
Member

Thanks for the PR and the suggestion. I think this is a good addition, I need to spend some time reviewing this, but as a feature I think it's a good idea. BIND calls this "views" for what it's worth, and it would be good for us to think through what the config will look like for this as well.

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

I left a lot of questions, I still want to think through the implications of this, but just seeking to have a conversation about some of this for now.

crates/server/src/authority/catalog.rs Outdated Show resolved Hide resolved
@@ -346,7 +444,8 @@ impl Catalog {
response_handle: R,
) -> ResponseInfo {
let request_info = request.request_info();
let authority = self.find(request_info.query.name());
let src_ip = request.src().ip();
Copy link
Member

Choose a reason for hiding this comment

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

A question I have here is if we should consider the EDNS client_addr as well (this isn't supported in Trust-DNS yet, but might be in the future) So I just want to think through what that would look like. It seems like we could trigger similar logic here based on that IP as well.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I'm sure this would be possibe but I really don't know anything about EDNS protocol. Is everything ready so it can be implemented now, even if not supported yet by trust dns?

/// * `ipnet` - [`IpNet`] this authority is for, e.g. 10.5.0.0/16
/// * `name` - zone name, e.g. example.com.
/// * `authority` - the zone data
pub fn upsert_for_ipnet(
Copy link
Member

Choose a reason for hiding this comment

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

I want to think about this change here and the additional functions. It seems we could maybe fix this a little differently by maybe passing an object that has these parameters rather than creating alternative function names?

We have the request object, but I think that has a bunch of optionals that makes it unwieldy... so maybe we need a lookup object that has non-optional data in it that could be used for the authority lookup?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know what is best for the crate. My first intention was to avoid any breaking change so the pull request would have a chance to be accepted. I still believe this is a niche feature and doesn't worth changing the function signature...

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

2 participants