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

MessageResponseBuilder::error_msg does not support all RCODEs #1203

Closed
jonasbb opened this issue Sep 15, 2020 · 3 comments · May be fixed by #1265
Closed

MessageResponseBuilder::error_msg does not support all RCODEs #1203

jonasbb opened this issue Sep 15, 2020 · 3 comments · May be fixed by #1265

Comments

@jonasbb
Copy link
Contributor

jonasbb commented Sep 15, 2020

Describe the bug
MessageResponseBuilder::error_msg allows to construct a response message with a given response code. The implementation fails to set the EDNS options if the response code requires EDNS.

To Reproduce
Steps to reproduce the behavior:

A minimal implementation of RequestHandler for the server crate:

impl trust_dns_server::server::RequestHandler for Handler {
    type ResponseFuture = future::Ready<()>;

    fn handle_request<R: ResponseHandler>(
        &self,
        request: trust_dns_server::server::Request,
        response_handle: R,
    ) -> Self::ResponseFuture {
        let request_message = request.message;
        if let Some(req_edns) = request_message.edns() {
            let response = MessageResponseBuilder::new(Some(request_message.raw_queries()));
            let our_version = 0;
            if req_edns.version() > our_version {
                response_handle
                    .send_response(response.error_msg(
                        request_message.id(),
                        request_message.op_code(),
                        ResponseCode::BADVERS,
                    ))
                    .unwrap();
                return future::ready(());
            }
        } else {
            todo!()
        }
    }
}

This code should produce a DNS response with the status code BADVERS but it does not create a EDNS record, thus it will end up as NOERROR. The lower 4 bit of BADVERS are all 0 which maps to the NOERROR value.

Expected behavior
Silently changing the response code is very bad. I can see two possible solutions here:

  1. Silently add a EDNS record to the message and set the high response code there.
    This would be the most developer friendly solution. If a EDNS record already exists it simply sets the high bits. This could lead to a situation where the server response with a EDNS record even if the client didn't specify EDNS support. But these "extended" response codes only make sense in the presence of EDNS.
  2. Return an error when setting a response code which requires EDNS. This would require to change the function signature to return a Result<_, _>.

One problem with both solutions is that MessageResponse::set_edns allows to overwrite the EDNS record of the response after setting the correct response code bits. This again could silently alter the intended response code.

Version:
Crate: server
Version: 0.20.0-alpha.1 and main

Additional context

The same problem is also present in trust_dns_server::authority::Catalog (0.20.0-alpha.1 and main).
https://github.com/bluejekyll/trust-dns/blob/48f625ad0de450329fcf628db779b21ae6eddfda/crates/server/src/authority/catalog.rs#L94-L124
Here the problem is not related to the use of error_msg, but the code implements the behavior manually and forgets to set the high response code in the EDNS record.

The documentation for Header::set_response_code hints that it does not support all response codes. The documentation could be improved with a link to EDNS and a small example how to properly set any response code.

@bluejekyll
Copy link
Member

This can definitely be improved. It looks like you've identified some ways that you want to improve the situation. Would you be interested in submitting a PR for this?

jonasbb added a commit to jonasbb/trust-dns that referenced this issue Sep 16, 2020
@jonasbb
Copy link
Contributor Author

jonasbb commented Sep 16, 2020

Yes, I will take a look at that. The solution should presumably also be applied to Message in proto, since that has an almost identical error_msg function.

@bluejekyll
Copy link
Member

The solution should presumably also be applied to Message in proto, since that has an almost identical error_msg function.

Oh, it's too bad those are duplicating the logic. Would be nice, if it's not difficult, to combine them.

jonasbb added a commit to jonasbb/trust-dns that referenced this issue Oct 2, 2020
jonasbb added a commit to jonasbb/trust-dns that referenced this issue Oct 9, 2020
jonasbb added a commit to jonasbb/trust-dns that referenced this issue Oct 26, 2020
Some ResponseCodes have high bits which require EDNS to encode them.
This commit updates Message::set_response_code and
MessageResponseBuiler::error_msg to silently create a EDNS section if
required and sets the high bits there.

This also adds a warning to Header::set_response_code that this function
cannot set the high bits.

Closes hickory-dns#1203
Closes hickory-dns#1207
jonasbb added a commit to jonasbb/trust-dns that referenced this issue Mar 11, 2022
Some ResponseCodes have high bits which require EDNS to encode them.
This commit updates Message::set_response_code and
MessageResponseBuiler::error_msg to silently create a EDNS section if
required and sets the high bits there.

This also adds a warning to Header::set_response_code that this function
cannot set the high bits.

Closes hickory-dns#1203
Closes hickory-dns#1207
jonasbb added a commit to jonasbb/trust-dns that referenced this issue Mar 11, 2022
Some ResponseCodes have high bits which require EDNS to encode them.
This commit updates Message::set_response_code and
MessageResponseBuiler::error_msg to silently create a EDNS section if
required and sets the high bits there.

This also adds a warning to Header::set_response_code that this function
cannot set the high bits.

Closes hickory-dns#1203
Closes hickory-dns#1207
bluejekyll pushed a commit that referenced this issue Apr 10, 2022
@jonasbb jonasbb closed this as not planned Won't fix, can't repro, duplicate, stale Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants