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

[Tor Bug Tracker & Wiki]

#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 dgoulet):

 I didn't read athena's review in order to NOT have a bias so if there is
 duplicate comment, just ignore them.

 * f253aef14faf7640f94e9e76947b6a4461c3c1a4

  lgtm

 * a9720b90f860323781d37dbba6ce04f312ec3632

  lgtm

 * cf9d780b570fa3ebf02e555c45f62d8b1bc38bcf
  - torcert.c: Ever function is not fully using trunnel generated
 setters/getters. Here are some example missing:
 {{{
 cert = tor_malloc_zero(sizeof(tor_cert_t));
 --> ed25519_cert_new()
 (and so on...)
 ed25519_cert_getlen_certified_key()
 ed25519_cert_get_exp_field()
 }}}
  - 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?
  - torcert.c: In `tor_cert_free()`, should the `cert->encoded` be wiped
 also since the non encoded version is wiped before free?
  - 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 :).
 {{{
   int siglen = crypto_pk_private_sign(rsa_key,
 (char*)rsa_ed_crosscert_getarray_sig(cc),
                                       rsa_ed_crosscert_getlen_sig(cc),
                                     (char*)res, 32 + 4);
 }}}
  - routerkeys.{c,h} lgtm!

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

  The rest lgtm!

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

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

  The rest lgtm.

 * 1e3a98f88d5e19239d00356d50f6b598a681d70c

  lgtm

 * bf43f2d853928d5e8e3a314dc506b73b6965fb49

  lgtm

 * 41cbaf0f267b0d1831aa3cf42e9d279cb171bc6a

  lgtm

 * 72d0d2c9c44cb6df47b35c07f94898f952a52fbc
  - Ok large amount of trunnel code. I didn't go over all the generated
 code.

 * 199b84cc67326b8cfbb13387a51f696339e5e0aa
 * 822104f99fb4715afa147b45aae94584db281069
 * df1562811712d382ad26dda81cf117e3a98ccbb6
  - Tests are great! :)

 * b52da5b56ea9ca45959400299f0d8ac494a8a1d3
  - channeltls.c: Trunnel is not fully used in that file.
 {{{
 + uint16_t cert_type = c->cert_type;
 --> certs_cell_cert_get_cert_type()
 + uint16_t cert_len = c->cert_len;
 --> certs_cell_cert_get_cert_len()
 + n_types = ac->n_methods;
 --> auth_challenge_cell_get_n_methods()
 ...
 }}}
  - channeltls.c: There is a bunch of memcpy() that are made using directly
 a trunnel data structure instead of setter. I'm OK with it else we have to
 do a wrapper over the trunnel ones to iterate 32 times to set the 32 bytes
 but at least for the length, the trunnel API could be used like
 `auth1_getlen_cid()`.
 {{{
 + memcpy(auth->scert,
 +        tor_x509_cert_get_cert_digests(cert)->d[DIGEST_SHA256], 32);
 + memcpy(auth->cid, client_id, 32);
 + memcpy(auth->sid, server_id, 32);
 }}}
  Same for something like: `+  crypto_rand((char*)auth->rand, 24);`, we
 should try to use trunnel as much as possible, here
 `auth1_getarray_rand()` and `auth1_getlen_rand`.


 Ok I'm stopping here since my brain can't continue for now. Will try to
 complete the rest tomorrow.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12498#comment:21>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online