If two auth certs are both old but were generated nearby in time, we keep both

changed milestone to %Tor: 0.2.6.x-final

added 026-triaged-1 andrea-review component::core tor/tor milestone::Tor: 0.2.6.x-final nickm-patch priority::medium resolution::implemented status::closed type::defect labels

I think maybe this line wanted to be

-            (cert_published + OLD_CERT_LIFETIME < newest_published)) {
+            (newest_published + OLD_CERT_LIFETIME < now)) {

so we wait 7 days before deleting any unexpired certs, in case we see them again (and earlier in the function we ensure that we never delete the newest cert for a given authority).

Does that sound plausible?

(I found the bug by setting up a test network and then updating a signing key and then moving my date forward a month. So it is a confirmed real bug.)

I think what we want here is for the logic to be "discard superseded certificates if any certificate newer than them has existed for at least X days?"

Trac:
Keywords: N/A deleted, 026-triaged-1 added

See 8157b8b7 and #854 (moved) for information on how this logic first appeared.

See branch "bug11454_11457" for a rewrite of the cert expiration code.

Trac:
Status: new to needs_review

Add the nickm-patch keyword to a bunch of needs_review tickets.

Trac:
Keywords: N/A deleted, nickm-patch added

Relatively quick review. The rewrite seems to solve this and #11457 (moved).

Thoughts:

if the first cert in the list is very expired and all subsequent certs are from the future, we don't remove it until we reach the future.
I think similar scenario to #11457 (moved), where one cert is created then soon after another is created, after two days all tors will discard the original cert. if the authority then starts reusing the original, everyone will re-request it every hour? This is much less bad than #11457 (moved), but it's a side-effect of discarding unexpired, superseded certs.
should we remember the signing key digest of the certs we download, and not discard superseded certs which we redownload often?
I wonder what other weird edge cases exist.

Minor consmetic changes

diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index 7112282..83d1c69 100644
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -498,7 +498,7 @@ trusted_dirs_remove_old_certs(void)
          * Remove it. */
         should_remove = 1;
       } else if (next_cert_published + SUPERSEDED_CERT_LIFETIME < now) {
-        /* Certificate has been superseded for OLD_CERT_LIFETIME.
+        /* Certificate has been superseded for SUPERSEDED_CERT_LIFETIME.
          * Remove it.
          */
         should_remove = 1;
@@ -512,7 +512,7 @@ trusted_dirs_remove_old_certs(void)
 
   } DIGESTMAP_FOREACH_END;
 #undef DEAD_CERT_LIFETIME
-#undef OLD_CERT_LIFETIME
+#undef SUPERSEDED_CERT_LIFETIME
 
   trusted_dirs_flush_certs_to_disk();
 }