Skip to content

Commit f4606bb

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

File tree

7 files changed

+369
-64
lines changed

7 files changed

+369
-64
lines changed

‎src/error.rs‎

Lines changed: 32 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.
@@ -127,6 +134,12 @@ pub enum Error {
127134
/// Trailing data was found while parsing DER-encoded input for the named type.
128135
TrailingData(DerTypeId),
129136

137+
/// The certificate is not valid for the name it is being validated for.
138+
///
139+
/// Variant without context for use in no-`alloc` environments. See `UnexpectedCertName` for
140+
/// the variant that is used in `alloc` environments.
141+
UnexpectedCertNameSimple,
142+
130143
/// A valid issuer for the certificate could not be found.
131144
UnknownIssuer,
132145

@@ -216,7 +229,7 @@ impl Error {
216229
match &self {
217230
// Errors related to certificate validity
218231
Self::CertNotValidYet | Self::CertExpired => 290,
219-
Self::CertNotValidForName => 280,
232+
Self::CertNotValidForName(_) => 280,
220233
Self::CertRevoked | Self::UnknownRevocationStatus | Self::CrlExpired => 270,
221234
Self::InvalidCrlSignatureForPublicKey | Self::InvalidSignatureForPublicKey => 260,
222235
Self::SignatureAlgorithmMismatch => 250,
@@ -225,6 +238,7 @@ impl Error {
225238
Self::PathLenConstraintViolated => 220,
226239
Self::CaUsedAsEndEntity | Self::EndEntityUsedAsCa => 210,
227240
Self::IssuerNotCrlSigner => 200,
241+
Self::UnexpectedCertNameSimple => 280,
228242

229243
// Errors related to supported features used in an invalid way.
230244
Self::InvalidCertValidity => 190,
@@ -300,6 +314,22 @@ impl fmt::Display for Error {
300314
#[cfg(feature = "std")]
301315
impl ::std::error::Error for Error {}
302316

317+
/// Additional context for the `CertNotValidForName` error variant.
318+
///
319+
/// The contents of this type depend on whether the `alloc` feature is enabled.
320+
#[derive(Clone, Debug, PartialEq, Eq)]
321+
pub struct InvalidNameContext {
322+
/// Expected server name.
323+
#[cfg(feature = "alloc")]
324+
pub expected: ServerName<'static>,
325+
/// The names presented in the end entity certificate.
326+
///
327+
/// Unlike `expected`, these names might contain wildcard labels, and can contain both DNS
328+
/// and IP address names.
329+
#[cfg(feature = "alloc")]
330+
pub presented: Vec<String>,
331+
}
332+
303333
/// Trailing data was found while parsing DER-encoded input for the named type.
304334
#[allow(missing_docs)]
305335
#[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: 40 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,34 +12,56 @@
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::{error::InvalidNameContext, Cert, Error};
2125

2226
pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Result<(), Error> {
2327
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-
};
28+
let result = NameIterator::new(Some(cert.subject), cert.subject_alt_name).find_map(|result| {
29+
let name = match result {
30+
Ok(name) => name,
31+
Err(err) => return Some(Err(err)),
32+
};
3033

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

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

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

‎src/subject_name/ip_address.rs‎

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,35 +12,56 @@
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::{error::InvalidNameContext, Cert, Error};
1924

2025
pub(crate) fn verify_ip_address_names(reference: &IpAddr, cert: &Cert<'_>) -> Result<(), Error> {
2126
let ip_address = match reference {
2227
IpAddr::V4(ip) => untrusted::Input::from(ip.as_ref()),
2328
IpAddr::V6(ip) => untrusted::Input::from(ip.as_ref()),
2429
};
2530

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-
};
31+
let result = NameIterator::new(None, cert.subject_alt_name).find_map(|result| {
32+
let name = match result {
33+
Ok(name) => name,
34+
Err(err) => return Some(Err(err)),
35+
};
3236

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

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

4667
// 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({})", 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)