Opened 5 years ago

Closed 4 years ago

#11454 closed defect (implemented)

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

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: 026-triaged-1, nickm-patch, andrea-review
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In trusted_dirs_remove_old_certs() we check if

            (cert_published + OLD_CERT_LIFETIME < newest_published)) {

when deciding whether to discard an old cert from our cache.

We don't check it at all with respect to current time.

So if an authority generates a signing key in January, and then generates ten more signing keys within a week, and now it's April, we'll still keep all of them until they expire or until a new signing key shows up that's more than 7 days newer than them.

This cannot be the right logic.

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by arma

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?

comment:2 Changed 5 years ago by arma

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

comment:3 Changed 5 years ago by nickm

Keywords: 026-triaged-1 added

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?"

comment:4 Changed 4 years ago by nickm

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

comment:5 Changed 4 years ago by nickm

Status: newneeds_review

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

comment:6 Changed 4 years ago by nickm

Keywords: nickm-patch added

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

comment:7 Changed 4 years ago by sysrqb

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

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

comment:8 Changed 4 years ago by nickm

Keywords: andrea-review added

comment:9 Changed 4 years ago by andrea

This branch looks good to me; merge away.

comment:10 Changed 4 years ago by nickm

Okay, will target for 0.2.6.4-??? or 0.2.7.1-alpha, since 0.2.6.3-alpha comes out today.

comment:11 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged to master. woooo

Note: See TracTickets for help on using tickets.