Skip to content

Commit d384bac

Browse files
djcdjc
committed
Add context in name validation errors
1 parent fd892e7 commit d384bac

File tree

7 files changed

+366
-64
lines changed

7 files changed

+366
-64
lines changed

‎src/error.rs‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,16 @@
1212
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
1313
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
1414

15+
#[cfg(feature = "alloc")]
16+
use alloc::string::String;
17+
#[cfg(feature = "alloc")]
18+
use alloc::vec::Vec;
1519
use core::fmt;
1620
use core::ops::ControlFlow;
1721

22+
#[cfg(feature = "alloc")]
23+
use pki_types::ServerName;
24+
1825
/// An error that occurs during certificate validation or name validation.
1926
#[derive(Clone, Debug, PartialEq, Eq)]
2027
#[non_exhaustive]
@@ -33,7 +40,7 @@ pub enum Error {
3340
CertExpired,
3441

3542
/// The certificate is not valid for the name it is being validated for.
36-
CertNotValidForName,
43+
CertNotValidForName(InvalidNameContext),
3744

3845
/// The certificate is not valid yet; i.e. the time it is being validated
3946
/// for is earlier than the certificate's notBefore time.
@@ -216,7 +223,7 @@ impl Error {
216223
match &self {
217224
// Errors related to certificate validity
218225
Self::CertNotValidYet | Self::CertExpired => 290,
219-
Self::CertNotValidForName => 280,
226+
Self::CertNotValidForName(_) => 280,
220227
Self::CertRevoked | Self::UnknownRevocationStatus | Self::CrlExpired => 270,
221228
Self::InvalidCrlSignatureForPublicKey | Self::InvalidSignatureForPublicKey => 260,
222229
Self::SignatureAlgorithmMismatch => 250,
@@ -300,6 +307,22 @@ impl fmt::Display for Error {
300307
#[cfg(feature = "std")]
301308
impl ::std::error::Error for Error {}
302309

310+
/// Additional context for the `CertNotValidForName` error variant.
311+
///
312+
/// The contents of this type depend on whether the `alloc` feature is enabled.
313+
#[derive(Clone, Debug, PartialEq, Eq)]
314+
pub struct InvalidNameContext {
315+
/// Expected server name.
316+
#[cfg(feature = "alloc")]
317+
pub expected: ServerName<'static>,
318+
/// The names presented in the end entity certificate.
319+
///
320+
/// These are the subject names as present in the leaf certificate and may contain DNS names
321+
/// with or without a wildcard label as well as IP address names.
322+
#[cfg(feature = "alloc")]
323+
pub presented: Vec<String>,
324+
}
325+
303326
/// Trailing data was found while parsing DER-encoded input for the named type.
304327
#[allow(missing_docs)]
305328
#[non_exhaustive]

‎src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ pub use {
7979
UnknownStatusPolicy,
8080
},
8181
end_entity::EndEntityCert,
82-
error::{DerTypeId, Error},
82+
error::{DerTypeId, Error, InvalidNameContext},
8383
rpk_entity::RawPublicKeyEntity,
8484
signed_data::alg_id,
8585
trust_anchor::anchor_from_trusted_cert,

‎src/subject_name/dns_name.rs‎

Lines changed: 41 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,34 +12,57 @@
1212
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
1313
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
1414

15+
#[cfg(feature = "alloc")]
16+
use alloc::format;
1517
use core::fmt::Write;
1618

19+
#[cfg(feature = "alloc")]
20+
use pki_types::ServerName;
1721
use pki_types::{DnsName, InvalidDnsNameError};
1822

1923
use super::verify::{GeneralName, NameIterator};
20-
use crate::{Cert, Error};
24+
use crate::cert::Cert;
25+
use crate::error::{Error, InvalidNameContext};
2126

2227
pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Result<(), Error> {
2328
let dns_name = untrusted::Input::from(reference.as_ref().as_bytes());
24-
NameIterator::new(Some(cert.subject), cert.subject_alt_name)
25-
.find_map(|result| {
26-
let name = match result {
27-
Ok(name) => name,
28-
Err(err) => return Some(Err(err)),
29-
};
29+
let result = NameIterator::new(Some(cert.subject), cert.subject_alt_name).find_map(|result| {
30+
let name = match result {
31+
Ok(name) => name,
32+
Err(err) => return Some(Err(err)),
33+
};
3034

31-
let presented_id = match name {
32-
GeneralName::DnsName(presented) => presented,
33-
_ => return None,
34-
};
35+
let presented_id = match name {
36+
GeneralName::DnsName(presented) => presented,
37+
_ => return None,
38+
};
3539

36-
match presented_id_matches_reference_id(presented_id, IdRole::Reference, dns_name) {
37-
Ok(true) => Some(Ok(())),
38-
Ok(false) | Err(Error::MalformedDnsIdentifier) => None,
39-
Err(e) => Some(Err(e)),
40-
}
41-
})
42-
.unwrap_or(Err(Error::CertNotValidForName))
40+
match presented_id_matches_reference_id(presented_id, IdRole::Reference, dns_name) {
41+
Ok(true) => Some(Ok(())),
42+
Ok(false) | Err(Error::MalformedDnsIdentifier) => None,
43+
Err(e) => Some(Err(e)),
44+
}
45+
});
46+
47+
match result {
48+
Some(result) => return result,
49+
#[cfg(feature = "alloc")]
50+
None => {}
51+
#[cfg(not(feature = "alloc"))]
52+
None => Err(Error::CertNotValidForName(InvalidNameContext {})),
53+
}
54+
55+
// Try to yield a more useful error. To avoid allocating on the happy path,
56+
// we reconstruct the same `NameIterator` and replay it.
57+
#[cfg(feature = "alloc")]
58+
{
59+
Err(Error::CertNotValidForName(InvalidNameContext {
60+
expected: ServerName::DnsName(reference.to_owned()),
61+
presented: NameIterator::new(Some(cert.subject), cert.subject_alt_name)
62+
.filter_map(|result| Some(format!("{:?}", result.ok()?)))
63+
.collect(),
64+
}))
65+
}
4366
}
4467

4568
/// A reference to a DNS Name presented by a server that may include a wildcard.

‎src/subject_name/ip_address.rs‎

Lines changed: 39 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,35 +12,57 @@
1212
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
1313
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
1414

15+
#[cfg(feature = "alloc")]
16+
use alloc::format;
17+
1518
use pki_types::IpAddr;
19+
#[cfg(feature = "alloc")]
20+
use pki_types::ServerName;
1621

1722
use super::verify::{GeneralName, NameIterator};
18-
use crate::{Cert, Error};
23+
use crate::cert::Cert;
24+
use crate::error::{Error, InvalidNameContext};
1925

2026
pub(crate) fn verify_ip_address_names(reference: &IpAddr, cert: &Cert<'_>) -> Result<(), Error> {
2127
let ip_address = match reference {
2228
IpAddr::V4(ip) => untrusted::Input::from(ip.as_ref()),
2329
IpAddr::V6(ip) => untrusted::Input::from(ip.as_ref()),
2430
};
2531

26-
NameIterator::new(None, cert.subject_alt_name)
27-
.find_map(|result| {
28-
let name = match result {
29-
Ok(name) => name,
30-
Err(err) => return Some(Err(err)),
31-
};
32+
let result = NameIterator::new(None, cert.subject_alt_name).find_map(|result| {
33+
let name = match result {
34+
Ok(name) => name,
35+
Err(err) => return Some(Err(err)),
36+
};
3237

33-
let presented_id = match name {
34-
GeneralName::IpAddress(presented) => presented,
35-
_ => return None,
36-
};
38+
let presented_id = match name {
39+
GeneralName::IpAddress(presented) => presented,
40+
_ => return None,
41+
};
3742

38-
match presented_id_matches_reference_id(presented_id, ip_address) {
39-
true => Some(Ok(())),
40-
false => None,
41-
}
42-
})
43-
.unwrap_or(Err(Error::CertNotValidForName))
43+
match presented_id_matches_reference_id(presented_id, ip_address) {
44+
true => Some(Ok(())),
45+
false => None,
46+
}
47+
});
48+
49+
match result {
50+
Some(result) => return result,
51+
#[cfg(feature = "alloc")]
52+
None => {}
53+
#[cfg(not(feature = "alloc"))]
54+
None => Err(Error::CertNotValidForName(InvalidNameContext {})),
55+
}
56+
57+
#[cfg(feature = "alloc")]
58+
{
59+
Err(Error::CertNotValidForName(InvalidNameContext {
60+
expected: ServerName::from(*reference),
61+
presented: NameIterator::new(None, cert.subject_alt_name)
62+
.filter_map(|result| Some(format!("{:?}", result.ok()?)))
63+
.collect(),
64+
}))
65+
}
4466
}
4567

4668
// https://tools.ietf.org/html/rfc5280#section-4.2.1.6 says:

‎src/subject_name/verify.rs‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@
1212
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
1313
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
1414

15+
#[cfg(feature = "alloc")]
16+
use alloc::string::String;
17+
#[cfg(feature = "alloc")]
18+
use core::fmt;
19+
1520
use super::dns_name::{self, IdRole};
1621
use super::ip_address;
1722
use crate::der::{self, FromDer};
@@ -295,3 +300,148 @@ impl<'a> FromDer<'a> for GeneralName<'a> {
295300

296301
const TYPE_ID: DerTypeId = DerTypeId::GeneralName;
297302
}
303+
304+
#[cfg(feature = "alloc")]
305+
impl fmt::Debug for GeneralName<'_> {
306+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
307+
match self {
308+
GeneralName::DnsName(name) => write!(
309+
f,
310+
"DnsName(\"{}\")",
311+
String::from_utf8_lossy(name.as_slice_less_safe())
312+
),
313+
GeneralName::DirectoryName => write!(f, "DirectoryName"),
314+
GeneralName::IpAddress(ip) => {
315+
write!(f, "IpAddress({:?})", IpAddrSlice(ip.as_slice_less_safe()))
316+
}
317+
GeneralName::UniformResourceIdentifier(uri) => write!(
318+
f,
319+
"UniformResourceIdentifier(\"{}\")",
320+
String::from_utf8_lossy(uri.as_slice_less_safe())
321+
),
322+
GeneralName::Unsupported(tag) => write!(f, "Unsupported({:02x})", tag),
323+
}
324+
}
325+
}
326+
327+
#[cfg(feature = "alloc")]
328+
struct IpAddrSlice<'a>(&'a [u8]);
329+
330+
#[cfg(feature = "alloc")]
331+
impl fmt::Debug for IpAddrSlice<'_> {
332+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
333+
match self.0.len() {
334+
4 => {
335+
let mut first = true;
336+
for byte in self.0 {
337+
match first {
338+
true => first = false,
339+
false => f.write_str(".")?,
340+
}
341+
342+
write!(f, "{}", byte)?;
343+
}
344+
345+
Ok(())
346+
}
347+
16 => {
348+
let (mut first, mut skipping) = (true, false);
349+
for group in self.0.chunks_exact(2) {
350+
match (first, group == [0, 0], skipping) {
351+
(true, _, _) => first = false,
352+
(false, false, false) => f.write_str(":")?,
353+
(false, true, _) => {
354+
skipping = true;
355+
continue;
356+
}
357+
(false, false, true) => {
358+
skipping = false;
359+
f.write_str("::")?;
360+
}
361+
}
362+
363+
if group[0] != 0 {
364+
write!(f, "{:x}", group[0])?;
365+
}
366+
367+
match group[0] {
368+
0 => write!(f, "{:x}", group[1])?,
369+
_ => write!(f, "{:02x}", group[1])?,
370+
}
371+
}
372+
Ok(())
373+
}
374+
_ => {
375+
f.write_str("[invalid: ")?;
376+
let mut first = true;
377+
for byte in self.0 {
378+
match first {
379+
true => first = false,
380+
false => f.write_str(", ")?,
381+
}
382+
write!(f, "{:02x}", byte)?;
383+
}
384+
f.write_str("]")
385+
}
386+
}
387+
}
388+
}
389+
390+
#[cfg(all(test, feature = "alloc"))]
391+
mod tests {
392+
use super::*;
393+
394+
#[test]
395+
fn debug_names() {
396+
assert_eq!(
397+
format!(
398+
"{:?}",
399+
GeneralName::DnsName(untrusted::Input::from(b"example.com"))
400+
),
401+
"DnsName(\"example.com\")"
402+
);
403+
404+
assert_eq!(format!("{:?}", GeneralName::DirectoryName), "DirectoryName");
405+
406+
assert_eq!(
407+
format!(
408+
"{:?}",
409+
GeneralName::IpAddress(untrusted::Input::from(&[192, 0, 2, 1][..]))
410+
),
411+
"IpAddress(192.0.2.1)"
412+
);
413+
414+
assert_eq!(
415+
format!(
416+
"{:?}",
417+
GeneralName::IpAddress(untrusted::Input::from(
418+
&[0x20, 0x01, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x0d, 0xb8][..]
419+
))
420+
),
421+
"IpAddress(2001::db8)"
422+
);
423+
424+
assert_eq!(
425+
format!(
426+
"{:?}",
427+
GeneralName::IpAddress(untrusted::Input::from(&[1, 2, 3, 4, 5, 6][..]))
428+
),
429+
"IpAddress([invalid: 01, 02, 03, 04, 05, 06])"
430+
);
431+
432+
assert_eq!(
433+
format!(
434+
"{:?}",
435+
GeneralName::UniformResourceIdentifier(untrusted::Input::from(
436+
b"https://example.com"
437+
))
438+
),
439+
"UniformResourceIdentifier(\"https://example.com\")"
440+
);
441+
442+
assert_eq!(
443+
format!("{:?}", GeneralName::Unsupported(0x42)),
444+
"Unsupported(66)"
445+
);
446+
}
447+
}

0 commit comments

Comments
 (0)