[GH-ISSUE #1203] `MessageResponseBuilder::error_msg` does not support all RCODEs #627

New issue

Closed

opened 2026-03-15 23:32:05 +03:00 by kerem · 3 comments

kerem commented

2026-03-15 23:32:05 +03:00

Owner

Originally created by @jonasbb on GitHub (Sep 15, 2020).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1203

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:

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.
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).
github.com/bluejekyll/trust-dns@48f625ad0d/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.

Originally created by @jonasbb on GitHub (Sep 15, 2020). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1203 **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: ```rust 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`](https://docs.rs/trust-dns-server/0.20.0-alpha.1/trust_dns_server/authority/struct.MessageResponse.html#method.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`](https://docs.rs/trust-dns-proto/0.20.0-alpha.1/trust_dns_proto/op/header/struct.Header.html#method.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.

kerem

2026-03-15 23:32:05 +03:00

closed this issue
added the
crate:server
label

kerem commented

2026-03-15 23:32:21 +03:00

Author

Owner

@bluejekyll commented on GitHub (Sep 15, 2020):

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?

@bluejekyll commented on GitHub (Sep 15, 2020): 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?

kerem commented

2026-03-15 23:32:26 +03:00

Author

Owner

@jonasbb commented on GitHub (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.

@jonasbb commented on GitHub (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.

kerem commented

2026-03-15 23:32:31 +03:00

Author

Owner

@bluejekyll commented on GitHub (Sep 16, 2020):

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.

@bluejekyll commented on GitHub (Sep 16, 2020): > 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.

kerem referenced this issue

2026-03-16 02:13:16 +03:00

[PR #627] [MERGED] Set minimum tokio-timer version to 0.2.6 #1564