Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally expire TrustAnchors #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use crate::{der, signed_data, Error};
use crate::{der, signed_data, time, Error};
use untrusted;

pub enum EndEntityOrCA<'a> {
Expand All @@ -35,6 +35,26 @@ pub struct Cert<'a> {
pub subject_alt_name: Option<untrusted::Input<'a>>,
}

pub struct Validity {
pub not_before: time::Time,
pub not_after: time::Time,
}

pub fn parse_validity(validity: untrusted::Input) -> Result<Validity, Error> {
validity.read_all(Error::BadDER, |input| {
let not_before = der::time_choice(input)?;
let not_after = der::time_choice(input)?;
if not_before < not_after {
Ok(Validity {
not_before,
not_after,
})
} else {
Err(Error::InvalidCertValidity)
}
})
}

pub fn parse_cert<'a>(
cert_der: untrusted::Input<'a>, ee_or_ca: EndEntityOrCA<'a>,
) -> Result<Cert<'a>, Error> {
Expand Down
3 changes: 3 additions & 0 deletions src/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,7 @@ impl Time {
/// `std::time::SystemTime` is available (when `#![no_std]` isn't being
/// used).
pub fn from_seconds_since_unix_epoch(secs: u64) -> Time { Time(secs) }

/// Unwrap this type to a unix timestamp.
pub(crate) fn into_seconds_since_unix_epoch(self) -> u64 { self.0 }
}
18 changes: 12 additions & 6 deletions src/trust_anchor_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

use super::der;
use crate::{
cert::{certificate_serial_number, parse_cert_internal, Cert, EndEntityOrCA},
cert::{certificate_serial_number, parse_cert_internal, parse_validity, Cert, EndEntityOrCA},
Error, TrustAnchor,
};
use untrusted;
Expand All @@ -40,7 +40,7 @@ pub fn cert_der_as_trust_anchor(cert_der: untrusted::Input) -> Result<TrustAncho
EndEntityOrCA::EndEntity,
possibly_invalid_certificate_serial_number,
) {
Ok(cert) => Ok(trust_anchor_from_cert(cert)),
Ok(cert) => trust_anchor_from_cert(cert),
Err(Error::BadDER) => parse_cert_v1(cert_der).or(Err(Error::BadDER)),
Err(err) => Err(err),
}
Expand Down Expand Up @@ -75,12 +75,15 @@ pub fn generate_code_for_trust_anchors(name: &str, trust_anchors: &[TrustAnchor]
decl + &value
}

fn trust_anchor_from_cert<'a>(cert: Cert<'a>) -> TrustAnchor<'a> {
TrustAnchor {
fn trust_anchor_from_cert<'a>(cert: Cert<'a>) -> Result<TrustAnchor<'a>, Error> {
let validity = parse_validity(cert.validity)?;
Ok(TrustAnchor {
subject: cert.subject.as_slice_less_safe(),
spki: cert.spki.as_slice_less_safe(),
name_constraints: cert.name_constraints.map(|nc| nc.as_slice_less_safe()),
}
not_before: validity.not_before.into_seconds_since_unix_epoch(),
not_after: validity.not_after.into_seconds_since_unix_epoch(),
})
}

/// Parses a v1 certificate directly into a TrustAnchor.
Expand All @@ -94,14 +97,17 @@ fn parse_cert_v1<'a>(cert_der: untrusted::Input<'a>) -> Result<TrustAnchor<'a>,

skip(tbs, der::Tag::Sequence)?; // signature.
skip(tbs, der::Tag::Sequence)?; // issuer.
skip(tbs, der::Tag::Sequence)?; // validity.
let validity = der::expect_tag_and_get_value(tbs, der::Tag::Sequence)?;
let subject = der::expect_tag_and_get_value(tbs, der::Tag::Sequence)?;
let spki = der::expect_tag_and_get_value(tbs, der::Tag::Sequence)?;
let validity = parse_validity(validity)?;

Ok(TrustAnchor {
subject: subject.as_slice_less_safe(),
spki: spki.as_slice_less_safe(),
name_constraints: None,
not_before: validity.not_before.into_seconds_since_unix_epoch(),
not_after: validity.not_after.into_seconds_since_unix_epoch(),
})
});

Expand Down
24 changes: 15 additions & 9 deletions src/verify_cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ pub fn build_chain(
return Err(Error::UnknownIssuer);
}

let not_before = time::Time::from_seconds_since_unix_epoch(trust_anchor.not_before);
let not_after = time::Time::from_seconds_since_unix_epoch(trust_anchor.not_after);

if time < not_before {
return Err(Error::CertNotValidYet);
}
if not_after < time {
return Err(Error::CertExpired);
}

let name_constraints = trust_anchor.name_constraints.map(untrusted::Input::from);

untrusted::read_all_optional(name_constraints, Error::BadDER, |value| {
Expand Down Expand Up @@ -159,8 +169,7 @@ fn check_issuer_independent_properties(
// See the comment in `remember_extension` for why we don't check the
// KeyUsage extension.

cert.validity
.read_all(Error::BadDER, |value| check_validity(value, time))?;
cert::parse_validity(cert.validity).and_then(|validity| check_validity(&validity, time))?;
untrusted::read_all_optional(cert.basic_constraints, Error::BadDER, |value| {
check_basic_constraints(value, used_as_ca, sub_ca_count)
})?;
Expand All @@ -172,17 +181,14 @@ fn check_issuer_independent_properties(
}

// https://tools.ietf.org/html/rfc5280#section-4.1.2.5
fn check_validity(input: &mut untrusted::Reader, time: time::Time) -> Result<(), Error> {
let not_before = der::time_choice(input)?;
let not_after = der::time_choice(input)?;

if not_before > not_after {
fn check_validity(validity: &cert::Validity, time: time::Time) -> Result<(), Error> {
if validity.not_before > validity.not_after {
return Err(Error::InvalidCertValidity);
}
if time < not_before {
if time < validity.not_before {
return Err(Error::CertNotValidYet);
}
if time > not_after {
if time > validity.not_after {
return Err(Error::CertExpired);
}

Expand Down
8 changes: 8 additions & 0 deletions src/webpki.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,14 @@ pub struct TrustAnchor<'a> {
/// The value of a DER-encoded NameConstraints, containing name
/// constraints to apply to the trust anchor, if any.
pub name_constraints: Option<&'a [u8]>,

/// The root's earliest validity in seconds since the epoch. The anchor
/// is only considered when the validation time is after this value.
pub not_before: u64,

/// The root's expiry in seconds since the epoch. The anchor
/// is only considered when the validation time is before this value.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good optimization and I appreciate you taking the time to do it.

The Timespec documentation refers to "the epoch" but AFAICT it isn't necessarily the UNIX epoch, so saying "UNIX epoch" in the comment seems wrong.

We could take the optimization further by making not_after a u32 by reducing its resolution from seconds to days, right? It probably would only matter for 32-bit platforms. Maybe there's not an easy way to do get go from a TimeSpec to days since an epoch, thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dividing seconds down to days is going to be annoying over 40-60 year periods, and I can't find anything in the time crate to help :(

pub not_after: u64,
}

/// Trust anchors which may be used for authenticating servers.
Expand Down
Binary file added tests/expiry/ca.der
Binary file not shown.
Binary file added tests/expiry/ee.der
Binary file not shown.
Binary file added tests/expiry/eelong.der
Binary file not shown.
63 changes: 63 additions & 0 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,66 @@ fn read_root_with_neg_serial() {
#[cfg(feature = "std")]
#[test]
fn time_constructor() { let _ = webpki::Time::try_from(std::time::SystemTime::now()).unwrap(); }

#[cfg(feature = "trust_anchor_util")]
mod expiry {
use untrusted::Input;
use webpki::{trust_anchor_util, EndEntityCert, Error};

fn expect_expiry(when: u64, ee: &[u8], expect_err: Option<Error>) {
let ca = include_bytes!("expiry/ca.der");

let ee_input = Input::from(ee);
let inter_vec = vec![];
let anchors = [trust_anchor_util::cert_der_as_trust_anchor(Input::from(ca)).unwrap()];
let anchors = webpki::TLSServerTrustAnchors(&anchors);

let rough_time = webpki::Time::from_seconds_since_unix_epoch(when);

let cert = EndEntityCert::from(ee_input).unwrap();
let rc = cert.verify_is_valid_tls_server_cert(
super::ALL_SIGALGS,
&anchors,
&inter_vec,
rough_time,
);

assert_eq!(expect_err, rc.err());
}

#[test]
pub fn valid() {
let cert = include_bytes!("expiry/ee.der");
expect_expiry(1479496432, &cert[..], None);
}

#[test]
pub fn ee_not_valid_yet() {
let cert = include_bytes!("expiry/ee.der");
expect_expiry(1476731633, &cert[..], None);
expect_expiry(1476731632, &cert[..], Some(Error::CertNotValidYet));
}

#[test]
pub fn ee_expired() {
let cert = include_bytes!("expiry/ee.der");
expect_expiry(1479496433, &cert[..], None);
expect_expiry(1479496434, &cert[..], Some(Error::CertExpired));
}

#[test]
fn ca_not_valid_yet() {
let cert = include_bytes!("expiry/eelong.der");
expect_expiry(1476731623, &cert[..], None);
expect_expiry(1476731622, &cert[..], Some(Error::UnknownIssuer));
}

#[test]
fn ca_expired() {
// This certificate has an expiry that extends past the end
// of its CA cert.
let cert = include_bytes!("expiry/eelong.der");
expect_expiry(1508267623, &cert[..], None);
expect_expiry(1508267624, &cert[..], Some(Error::UnknownIssuer));
}
}