Skip to content

Commit

Permalink
Review feedback: Implement response OPT stripping if the request lack…
Browse files Browse the repository at this point in the history 
…ed an OPT record.
  • Loading branch information
@ximon18
ximon18 committed Apr 4, 2024
1 parent abdf4f3 commit b9a5b06
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 36 deletions.
76 changes: 41 additions & 35 deletions src/net/server/middleware/processors/edns.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use core::ops::ControlFlow;

use octseq::Octets;
use tracing::{debug, enabled, trace, warn, Level};
use tracing::{debug, enabled, error, trace, warn, Level};

use crate::base::iana::OptRcode;
use crate::base::message_builder::AdditionalBuilder;
Expand All @@ -13,8 +13,8 @@ use crate::base::StreamTarget;
use crate::net::server::message::{Request, TransportSpecificContext};
use crate::net::server::middleware::processor::MiddlewareProcessor;
use crate::net::server::middleware::processors::mandatory::MINIMUM_RESPONSE_BYTE_LEN;
use crate::net::server::util::add_edns_options;
use crate::net::server::util::start_reply;
use crate::net::server::util::{add_edns_options, remove_edns_opt_record};

/// EDNS version 0.
///
Expand Down Expand Up @@ -52,6 +52,7 @@ impl EdnsMiddlewareProcessor {
impl EdnsMiddlewareProcessor {
/// Create a DNS error response to the given request with the given RCODE.
fn error_response<RequestOctets, Target>(
&self,
request: &Request<RequestOctets>,
rcode: OptRcode,
) -> AdditionalBuilder<StreamTarget<Target>>
Expand All @@ -72,6 +73,7 @@ impl EdnsMiddlewareProcessor {
);
}

self.postprocess(request, &mut additional);
additional
}
}
Expand Down Expand Up @@ -100,10 +102,9 @@ where
if iter.next().is_some() {
// More than one OPT RR received.
debug!("RFC 6891 6.1.1 violation: request contains more than one OPT RR.");
return ControlFlow::Break(Self::error_response(
request,
OptRcode::FormErr,
));
return ControlFlow::Break(
self.error_response(request, OptRcode::FormErr),
);
}

if let Ok(opt) = opt {
Expand All @@ -116,10 +117,9 @@ where
// RCODE=BADVERS."
if opt_rec.version() > EDNS_VERSION_ZERO {
debug!("RFC 6891 6.1.3 violation: request EDNS version {} > 0", opt_rec.version());
return ControlFlow::Break(Self::error_response(
request,
OptRcode::BadVers,
));
return ControlFlow::Break(
self.error_response(request, OptRcode::BadVers),
);
}

match request.transport_ctx() {
Expand All @@ -137,7 +137,7 @@ where
if opt_rec.opt().tcp_keepalive().is_some() {
debug!("RFC 7828 3.2.1 violation: edns-tcp-keepalive option received via UDP");
return ControlFlow::Break(
Self::error_response(
self.error_response(
request,
OptRcode::FormErr,
),
Expand Down Expand Up @@ -215,7 +215,7 @@ where
if keep_alive.timeout().is_some() {
debug!("RFC 7828 3.2.1 violation: edns-tcp-keepalive option received via TCP contains timeout");
return ControlFlow::Break(
Self::error_response(
self.error_response(
request,
OptRcode::FormErr,
),
Expand All @@ -236,6 +236,35 @@ where
request: &Request<RequestOctets>,
response: &mut AdditionalBuilder<StreamTarget<Target>>,
) {
// https://www.rfc-editor.org/rfc/rfc6891.html#section-6.1.1
// 6.1.1: Basic Elements
// ...
// "If an OPT record is present in a received request, compliant
// responders MUST include an OPT record in their respective
// responses."
//
// We don't do anything about this scenario at present.

// https://www.rfc-editor.org/rfc/rfc6891.html#section-7
// 7: Transport considerations
// ...
// "Lack of presence of an OPT record in a request MUST be taken as an
// indication that the requestor does not implement any part of this
// specification and that the responder MUST NOT include an OPT
// record in its response."
//
// So strip off any OPT record present if the query lacked an OPT
// record.
if request.message().opt().is_none() {
if let Err(err) = remove_edns_opt_record(response) {
error!(
"Error while stripping OPT record from response: {err}"
);
*response = self.error_response(request, OptRcode::ServFail);
return;
}
}

// https://datatracker.ietf.org/doc/html/rfc7828#section-3.3.2
// 3.3.2. Sending Responses
// "A DNS server that receives a query sent using TCP transport that
Expand Down Expand Up @@ -280,29 +309,6 @@ where
}
}
}

// https://www.rfc-editor.org/rfc/rfc6891.html#section-6.1.1
// 6.1.1: Basic Elements
// ...
// "If an OPT record is present in a received request, compliant
// responders MUST include an OPT record in their respective
// responses."
//
// We don't do anything about this scenario at present.

// https://www.rfc-editor.org/rfc/rfc6891.html#section-7
// 7: Transport considerations
// ...
// "Lack of presence of an OPT record in a request MUST be taken as an
// indication that the requestor does not implement any part of this
// specification and that the responder MUST NOT include an OPT
// record in its response."
//
// So strip off any OPT record present if the query lacked an OPT
// record.

// TODO: How can we strip off the OPT record in the response if no OPT
// record is present in the request?
}
}

Expand Down
49 changes: 48 additions & 1 deletion src/net/server/util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,9 @@ where
// delay response building until the complete set of differences to a base
// response are known. Or a completely different builder approach that can
// edit a partially built message.
if response.counts().arcount() > 0 {
if response.counts().arcount() > 0
&& response.as_message().opt().is_some()
{
// Make a copy of the response.
let copied_response = response.as_slice().to_vec();
let Ok(copied_response) = Message::from_octets(&copied_response)
Expand Down Expand Up @@ -271,3 +273,48 @@ where
// No existing OPT record in the additional section so build a new one.
response.opt(op)
}

/// Removes any OPT records present in the response.
pub fn remove_edns_opt_record<Target>(
response: &mut AdditionalBuilder<StreamTarget<Target>>,
) -> Result<(), PushError>
where
Target: Composer,
{
// TODO: This function has the same less than ideal properties as the
// add_edns_options() function above that it is similar to, ideally we can
// avoid the need to copy the response.
if response.counts().arcount() > 0
&& response.as_message().opt().is_some()
{
// Make a copy of the response.
let copied_response = response.as_slice().to_vec();
let Ok(copied_response) = Message::from_octets(&copied_response)
else {
warn!("Internal error: Unable to create message from octets while adding EDNS option");
return Ok(());
};

if copied_response.opt().is_some() {
// Discard the current records in the additional section of the
// response.
response.rewind();

// Copy the non-OPT records from the copied response to the
// current response.
if let Ok(current_additional) = copied_response.additional() {
for rr in current_additional.flatten() {
if rr.rtype() != Rtype::Opt {
if let Ok(Some(rr)) = rr
.into_record::<AllRecordData<_, ParsedDname<_>>>()
{
response.push(rr)?;
}
}
}
}
}
}

Ok(())
}

0 comments on commit b9a5b06

Please sign in to comment.