change results of validation to always return records

This commit is contained in:
Benjamin Fry
2023-11-25 22:11:18 -08:00
parent 1d2a1df0ee
commit ec806ae445
3 changed files with 40 additions and 35 deletions

View File

@@ -225,8 +225,8 @@ pub enum ProofErrorKind {
/// The error type for dnssec errors that get returned in the crate /// The error type for dnssec errors that get returned in the crate
#[derive(Debug, Clone, Error)] #[derive(Debug, Clone, Error)]
pub struct ProofError { pub struct ProofError {
proof: Proof, pub proof: Proof,
kind: ProofErrorKind, pub kind: ProofErrorKind,
} }
impl ProofError { impl ProofError {

View File

@@ -193,11 +193,15 @@ where
message_response, message_response,
options, options,
) )
.map(Result::<DnsResponse, ProtoError>::Ok)
}) })
.and_then(move |verified_message| { .and_then(move |verified_message| {
// Query should be unowned at this point // Query should be unowned at this point
let query = Arc::clone(&query2); let query = Arc::clone(&query2);
// TODO: I've noticed upstream resolvers don't always return NSEC responses
// this causes bottom up evaluation to fail
// at this point all of the message is verified. // at this point all of the message is verified.
// This is where NSEC (and possibly NSEC3) validation occurs // This is where NSEC (and possibly NSEC3) validation occurs
// As of now, only NSEC is supported. // As of now, only NSEC is supported.
@@ -249,7 +253,7 @@ async fn verify_response<H>(
query: Arc<Query>, query: Arc<Query>,
mut message: DnsResponse, mut message: DnsResponse,
options: DnsRequestOptions, options: DnsRequestOptions,
) -> Result<DnsResponse, ProtoError> ) -> DnsResponse
where where
H: DnsHandle + Sync + Unpin, H: DnsHandle + Sync + Unpin,
{ {
@@ -257,15 +261,15 @@ where
let nameservers = message.take_name_servers(); let nameservers = message.take_name_servers();
let additionals = message.take_additionals(); let additionals = message.take_additionals();
let answers = verify_rrsets(handle.clone(), &query, answers, options).await?; let answers = verify_rrsets(handle.clone(), &query, answers, options).await;
let nameservers = verify_rrsets(handle.clone(), &query, nameservers, options).await?; let nameservers = verify_rrsets(handle.clone(), &query, nameservers, options).await;
let additionals = verify_rrsets(handle.clone(), &query, additionals, options).await?; let additionals = verify_rrsets(handle.clone(), &query, additionals, options).await;
message.insert_answers(answers); message.insert_answers(answers);
message.insert_name_servers(nameservers); message.insert_name_servers(nameservers);
message.insert_additionals(additionals); message.insert_additionals(additionals);
Ok(message) message
} }
/// This pulls all answers returned in a Message response and returns a future which will /// This pulls all answers returned in a Message response and returns a future which will
@@ -276,7 +280,7 @@ async fn verify_rrsets<H>(
query: &Query, query: &Query,
records: Vec<Record>, records: Vec<Record>,
options: DnsRequestOptions, options: DnsRequestOptions,
) -> Result<Vec<Record>, ProtoError> ) -> Vec<Record>
where where
H: DnsHandle + Sync + Unpin, H: DnsHandle + Sync + Unpin,
{ {
@@ -302,7 +306,7 @@ where
// there were no records to verify // there were no records to verify
if rrset_types.is_empty() { if rrset_types.is_empty() {
return Ok(records); return records;
} }
// collect all the rrsets to verify // collect all the rrsets to verify
@@ -333,14 +337,24 @@ where
// TODO: support non-IN classes? // TODO: support non-IN classes?
debug!( debug!(
"verifying: {}, record_type: {:?}, rrsigs: {}", "verifying: {name} record_type: {record_type}, rrsigs: {rrsig_len}",
rrset.name, rrsig_len = rrsigs.len()
record_type,
rrsigs.len()
); );
// verify this rrset // verify this rrset
let proof = verify_rrset(handle.clone_with_context(), rrset, rrsigs, options).await?; let proof = verify_rrset(handle.clone_with_context(), rrset, rrsigs, options).await;
let proof = match proof {
Ok(proof) => {
debug!("verified: {name} record_type: {record_type}",);
proof
}
Err(ProofError { proof, kind }) => {
debug!("failed to verify: {name} record_type: {record_type}: {kind}",);
proof
}
};
rrset_proofs.insert((name, record_type), proof); rrset_proofs.insert((name, record_type), proof);
} }
@@ -352,7 +366,7 @@ where
.map(|proof| record.set_proof(*proof)); .map(|proof| record.set_proof(*proof));
} }
Ok(records) records
} }
// TODO: is this method useful/necessary? // TODO: is this method useful/necessary?
@@ -373,7 +387,7 @@ async fn verify_rrset<H>(
rrset: Rrset<'_>, rrset: Rrset<'_>,
rrsigs: Vec<&RRSIG>, rrsigs: Vec<&RRSIG>,
options: DnsRequestOptions, options: DnsRequestOptions,
) -> Result<Proof, ProtoError> ) -> Result<Proof, ProofError>
where where
H: DnsHandle + Sync + Unpin, H: DnsHandle + Sync + Unpin,
{ {
@@ -382,13 +396,9 @@ where
match rrset.record_type { match rrset.record_type {
// validation of DNSKEY records require different logic as they search for DS record coverage as well // validation of DNSKEY records require different logic as they search for DS record coverage as well
RecordType::DNSKEY => { RecordType::DNSKEY => {
verify_dnskey_rrset(handle.clone_with_context(), rrset, options) verify_dnskey_rrset(handle.clone_with_context(), rrset, options).await
.map_err(|e| ProtoError::from(format!("failed to validate DNSKEYs: {e}")))
.await
} }
_ => verify_default_rrset(&handle.clone_with_context(), rrset, rrsigs, options) _ => verify_default_rrset(&handle.clone_with_context(), rrset, rrsigs, options).await,
.await
.map_err(|e| ProtoError::from(format!("failed to validate RRSIGs: {e}"))),
} }
} }

View File

@@ -612,6 +612,10 @@ pub mod testing {
); );
} }
} }
for record in response.as_lookup().record_iter() {
assert!(record.proof().is_secure())
}
} }
/// Test IP lookup from domains that exist but unsigned with DNSSEC validation. /// Test IP lookup from domains that exist but unsigned with DNSSEC validation.
@@ -622,7 +626,7 @@ pub mod testing {
mut exec: E, mut exec: E,
handle: R, handle: R,
) { ) {
use crate::error::*; use crate::{error::*, lookup};
use proto::rr::RecordType; use proto::rr::RecordType;
let resolver = AsyncResolver::new( let resolver = AsyncResolver::new(
ResolverConfig::default(), ResolverConfig::default(),
@@ -637,18 +641,9 @@ pub mod testing {
// needs to be a domain that exists, but is not signed (eventually this will be) // needs to be a domain that exists, but is not signed (eventually this will be)
let response = exec.block_on(resolver.lookup_ip("hickory-dns.org.")); let response = exec.block_on(resolver.lookup_ip("hickory-dns.org."));
assert!(response.is_err()); let lookup_ip = response.unwrap();
let error = response.unwrap_err(); for record in lookup_ip.as_lookup().record_iter() {
assert!(record.proof().is_bogus())
let error_str = format!("{error}");
let name = Name::from_str("hickory-dns.org.").unwrap();
let expected_str = format!(
"proto error: failed to validate RRSIGs: Bogus: ds record should exist: {name}"
);
assert_eq!(error_str, expected_str);
if let ResolveErrorKind::Proto(_) = *error.kind() {
} else {
panic!("wrong error")
} }
} }