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

Conversation

ctz
Copy link
Contributor

@ctz ctz commented Oct 21, 2016

No description provided.

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I do agree that something like this is reasonable.

Some clarifying questions:

  1. Are you sure these are the semantics you want? My understanding from previous discussion is that you wanted to implement something similar to what Mozilla is proposing for StartCom/WoSign. IIUC, those semantics are like this: if the cert chains up to one of their roots, then compare notBefore of the end-entity certificate to the given expiration time, and if the notBefore date is the same or later, then reject the certificate. Notice, in particular, that the current time isn't involved at all in that.
  2. Do we need to store the entire validity? It seems like for real-world use cases we only need to store one date, not two, because nobody will probably need to trust a root at some point in the future, but not now.
  3. Maybe we should consider having a NEVER_EXPIRES constant that is "9999-12-31 12:59:59 UTC" or similar, instead of using Option? OTOH, if I understand correctly Option<&T> doesn't take any extra space over &T since Option is a nullable type, so maybe using Option is actually more efficient. Probably not a big deal either way.

@@ -54,6 +54,13 @@ pub fn build_chain<'a>(required_eku_if_present: KeyPurposeId,
return Err(Error::UnknownIssuer);
}

if trust_anchor.validity.is_some() {
Copy link
Owner

Choose a reason for hiding this comment

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

You can write this as (ignoring the formatting):

if let Some(validity) = trust_anchor.validity {
    let validity = untrusted::Input::from(validity.as_ref()); // Maybe `as_ref()` isn't 
    try!(validity.read_all(Error::BadDER, |v| check_validity(v, time)));
}

tests/expiry.rs Outdated

#[test]
pub fn expired_ee_after()
{
Copy link
Owner

Choose a reason for hiding this comment

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

The formatting of this file is weird, at least with respect to the braces.

At some point in the near future, I'll invest in getting rustfmt or similar to do this automatically, but in the meantime let's use the formatting that is used in the rest of the source. (I see that I accepted some other tests with this formatting. I guess we should fix them too in a separate commit.)

src/webpki.rs Outdated
pub name_constraints: Option<&'a [u8]>
pub name_constraints: Option<&'a [u8]>,

/// If supplied, the anchor is only considered when the current time
Copy link
Owner

Choose a reason for hiding this comment

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

"current time" is misleading. Maybe "validation time" is better.

@ctz
Copy link
Contributor Author

ctz commented Oct 27, 2016

Are you sure these are the semantics you want?

Yes. This is a separate thing from the wosign/startcom rules. I have that on a separate branch, but it's not done yet.

I'll sort out the rest of the things and drop a note when this PR is updated.

Cheers,
Joe

@ctz ctz force-pushed the jbp-root-expiry branch 3 times, most recently from cd79343 to 48211fa Compare October 29, 2016 22:40
@ctz
Copy link
Contributor Author

ctz commented Oct 29, 2016

Ready for your attention again.

I've replaced the raw validity with a single not_after integer, reusing the existing validity parsing bits in verify_cert.

try!(cert.validity.read_all(Error::BadDER,
|value| check_validity(value, time)));
try!(parse_validity(&cert.validity)
.and_then(|bounds| check_validity(bounds, time)));
Copy link
Owner

Choose a reason for hiding this comment

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

I think validity is a better name than bounds here.

@@ -185,6 +188,15 @@ fn check_validity(input: &mut untrusted::Reader, time: time::Timespec)
Ok(())
}

pub fn parse_validity(validity: &untrusted::Input)
-> Result<(time::Timespec, time::Timespec), Error> {
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of using a tuple, let's create a struct Validity { notBefore: Timespec, notAfter: Timespec }. I'm trying to get away from using tuples like this in ring and webpki, in general.

@@ -16,6 +16,7 @@

use {Error, TrustAnchor};
use cert::{certificate_serial_number, Cert, EndEntityOrCA, parse_cert_internal};
use verify_cert::parse_validity;
Copy link
Owner

Choose a reason for hiding this comment

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

Based on the existing code, it seems like parse_validty should go into cert instead of verify_cert, like certificate_serial_number.

pub name_constraints: Option<&'a [u8]>,

/// The root's expiry in seconds since the UNIX 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 :(

@ctz
Copy link
Contributor Author

ctz commented Nov 6, 2016

Thanks for the comments. I've pushed the fixes.

@briansmith
Copy link
Owner

Do you still think this is useful? My recollection is fuzzy now, but I seem to remember thinking we wouldn't need this, depending on how the StartCom (et al.) stuff turned out.

If we still need this, I think we should land it on top of the Time refactoring (PR #44).

@ctz
Copy link
Contributor Author

ctz commented Aug 20, 2017

My primary use-case for this is preventing programs that compile-in webpki-roots from supporting those roots past their real notAfter dates.

There was a separate unrelated thing about startcom, which was saying per-TrustAnchor "don't validate certs after a certain notBefore date". That's definitely not required any more.

@briansmith
Copy link
Owner

@ctz Back when you first submitted this PR I was working on a project where I needed to cram as much stuff as I could into a tiny amount of space, and I kept putting off reviewing this PR until I was sure that this wouldn't create a problem for that project. Now that project is long-over and it would be no problem to take this PR, if you are interested in rebasing it.

The purpose of this is to avoid programs which compile-in
root certificates from trusting them indefinitely.
Base automatically changed from master to main January 14, 2021 01:31
@briansmith
Copy link
Owner

Note: I renamed the "master" branch to "main". Sorry for the inconvenience. This PR has had its base branch updated to "main" but you'll need to deal with the change in your local repo yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants