Re: [tor-bugs] #18320 [Core Tor/Tor]: Clear old entries from the key-pinning journal file

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#18320: Clear old entries from the key-pinning journal file
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  andrea
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_revision
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  tor-dos, TorCoreTeam201606, review-  |        Version:
  group-3                                        |     Resolution:
Parent ID:  #17293                               |  Actual Points:
 Reviewer:  dgoulet                              |         Points:  3
                                                 |        Sponsor:
                                                 |  SponsorU-can
-------------------------------------------------+-------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_revision
 * reviewer:   => dgoulet


Comment:

 General comment.

 I'm not so cheerful about the keypin code directly using HT_* instead of
 the tested and robust interface we have that are `digestmap` and
 `digest256map` _especially_ when we use RSA and ed25519 digest as keys.
 Maybe there is a reason? I don't see one for now nor see one in the
 comment. So I understand why this patch is using the HT_* interface but it
 adds quite a bit of complexity for review... Re-implementing the
 add/remove/iteration is really not ideal as we duplicate lots of things.

 Why the double linked list? Isn't a `smartlist_t` ordered and when
 removing we can make sure to keep it with `smartlist_del_keeporder()`?
 This would simplify things since implementing a double linked list in this
 file is not ideal I would say.

 Some code comment:

 DG1: `keypin_add_or_replace_entry_in_map()` can return -2 I believe but
 it's not documented.

 DG2: This `if(pruner)` is not useful there: `if (pruner)
 keypin_free_pruner(pruner);`

 DG3: In `keypin_prune_journal()`, the pruned journal is written to disk
 only if `r == 0` that is we parsed it successfully. However, on error the
 `log_info(...)` indicating that we've pruned the journal is given. Maybe
 if `r` is an error, we should `goto err`? (might be confusing with
 `strerror(errno)` though).

 DG4: `keypin_add_line_to_pruner()`, I think `len` should be `size_t` here.

 Ok, done for now.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18320#comment:20>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs