[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #12498 [Tor]: Implement ed25519 identity keys (prop 220)

#12498: Implement ed25519 identity keys (prop 220)
-------------------------+-------------------------------------------------
     Reporter:  asn      |      Owner:  nickm
         Type:  task     |     Status:  needs_review
     Priority:  major    |  Milestone:  Tor: 0.2.7.x-final
    Component:  Tor      |    Version:  Tor: 0.2.7
   Resolution:           |   Keywords:  026-triaged-1, 027-triaged-1-in,
Actual Points:           |  SponsorU
       Points:  large    |  Parent ID:  #15054
-------------------------+-------------------------------------------------

Comment (by nickm):

 Replying to [comment:21 dgoulet]:
 > I didn't read athena's review in order to NOT have a bias so if there is
 duplicate comment, just ignore them.
 >
 > * cf9d780b570fa3ebf02e555c45f62d8b1bc38bcf
 >  - torcert.c: Ever function is not fully using trunnel generated
 setters/getters. Here are some example missing:

 I'm going to call this "maybe fix later".  I'm opening #16204 to track it.
 Right now I'm not sure it's a great idea, though.

 >  - torcert.c: Should `ed25519_cert_getlen_signature()` be used instead
 of `ED25519_SIG_LEN`? I know it's the same hardcoded "won't change in a
 zillion year" len but just for the completion of using trunnel all the
 way?

 I'm okay with letting constants be constants. :)

 >  - torcert.c: In `tor_cert_free()`, should the `cert->encoded` be wiped
 also since the non encoded version is wiped before free?

 I'm not sure this is really sensitive or not, but it can't hurt. So sure.
 Commit ac3a419ef52f995c9f365113edc1456f09b595de

 >  - torcert.c: `tor_make_rsa_ed25519_crosscert()` doesn't have any
 comments. I don't mind for trivial functions but explaining the return
 value at least would be great since it's clearly the len of ???
 >  - torcert.c: In `tor_make_rsa_ed25519_crosscert()`, this code snippet,
 where `32 + 4` comes from? Could it be taken from a define value or using
 a function call with a name that indicates the semantic here? Sorry, but
 hardcoded offset with no information makes me nervous :).

 both documented in fb3a605effcbcf0a22869c6aab26515ed280435e

 > * 567e42e894c2d06f3934bc90f7f75c9154481023
 >  - crypto.c: Function `crypto_digest_smartlist_prefix()`, comment
 doesn't explain `prepend`.

 Already fixed; Andrea noticed.

 > * c461287653541a05deab32ff9dd719cc4dd25352
 >  - router.c: In `router_dump_router_to_string()`, the `ntor_cc_line` and
 `rsa_tap_cc_line` are leaking.

 05ed08e9c6599cd9f6f62cd6e45c366574426f8c should fix this.

 > * f7931c11cb37c4e1f6d85800ae113b43df44d9f6
 >  - keypin.c: In `keypin_close_journal()`, we probably want to check
 `keypin_journal_fd >= 0` instead because `0` is a valid fd and `-1` is
 not.

 Fixed in e13cf08d49377810cbb409cfb793f202a03054e1


 >  - keypin.c: I think I would go for the `O_SYNC` param here in case the
 server has backup or snapshot, you might not want partial data in there.

 I wonder about O_SYNC; ISTR that it has really bad performance properties.
 See note above to Andrea about how the format is designed to withstand
 keypin files that get a little munged.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12498#comment:23>
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