Optionally expire TrustAnchors #24

ctz · 2016-10-21T19:11:18Z

No description provided.

briansmith

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

Some clarifying questions:

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.
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.
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.

briansmith · 2016-10-21T20:32:53Z

src/verify_cert.rs

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

+        if trust_anchor.validity.is_some() {


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))); }

briansmith · 2016-10-22T04:40:09Z

tests/expiry.rs

+
+#[test]
+pub fn expired_ee_after()
+{


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.)

briansmith · 2016-10-23T21:33:26Z

src/webpki.rs

-    pub name_constraints: Option<&'a [u8]>
+    pub name_constraints: Option<&'a [u8]>,
+
+    /// If supplied, the anchor is only considered when the current time


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

ctz · 2016-10-27T20:29:01Z

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 · 2016-10-29T22:43:20Z

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.

briansmith · 2016-11-06T07:15:02Z

src/verify_cert.rs

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


I think validity is a better name than bounds here.

briansmith · 2016-11-06T07:15:56Z

src/verify_cert.rs

@@ -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> {


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.

briansmith · 2016-11-06T07:18:28Z

src/trust_anchor_util.rs

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

 use {Error, TrustAnchor};
 use cert::{certificate_serial_number, Cert, EndEntityOrCA, parse_cert_internal};
+use verify_cert::parse_validity;


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

briansmith · 2016-11-06T07:25:30Z

src/webpki.rs

+    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.


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.

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 · 2016-11-06T11:23:29Z

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

briansmith · 2017-08-18T23:08:52Z

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 · 2017-08-20T09:30:04Z

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 · 2019-03-25T18:58:42Z

@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.

briansmith · 2021-01-14T01:42:51Z

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.

briansmith requested changes Oct 23, 2016

View reviewed changes

briansmith reviewed Oct 23, 2016

View reviewed changes

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

briansmith reviewed Nov 6, 2016

View reviewed changes

ctz force-pushed the jbp-root-expiry branch from 48211fa to 713a629 Compare November 6, 2016 11:07

ctz force-pushed the jbp-root-expiry branch from 713a629 to 5e36115 Compare November 6, 2016 12:49

Check time validity of TrustAnchors

93a55e2

The purpose of this is to avoid programs which compile-in root certificates from trusting them indefinitely.

ctz force-pushed the jbp-root-expiry branch from 5e36115 to 93a55e2 Compare May 4, 2019 17:14

Base automatically changed from master to main January 14, 2021 01:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally expire TrustAnchors #24

Optionally expire TrustAnchors #24

ctz commented Oct 21, 2016

briansmith left a comment

briansmith Oct 21, 2016

briansmith Oct 22, 2016

briansmith Oct 23, 2016

ctz commented Oct 27, 2016

ctz commented Oct 29, 2016

briansmith Nov 6, 2016

briansmith Nov 6, 2016

briansmith Nov 6, 2016

briansmith Nov 6, 2016

ctz Nov 6, 2016

ctz commented Nov 6, 2016

briansmith commented Aug 18, 2017

ctz commented Aug 20, 2017

briansmith commented Mar 25, 2019

briansmith commented Jan 14, 2021

Optionally expire TrustAnchors #24

Are you sure you want to change the base?

Optionally expire TrustAnchors #24

Conversation

ctz commented Oct 21, 2016

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctz commented Oct 27, 2016

ctz commented Oct 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctz commented Nov 6, 2016

briansmith commented Aug 18, 2017

ctz commented Aug 20, 2017

briansmith commented Mar 25, 2019

briansmith commented Jan 14, 2021