Opened 5 years ago

Closed 4 years ago

#12498 closed task (implemented)

Implement ed25519 identity keys (prop 220)

Reported by: asn Owned by: nickm
Priority: High Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: 026-triaged-1, 027-triaged-1-in, SponsorU
Cc: Actual Points:
Parent ID: #15054 Points: large
Reviewer: Sponsor:

Description


Child Tickets

TicketStatusOwnerSummaryComponent
#12499closednickmImplement cross certification of identity keys with onion keys (prop 228)Core Tor/Tor

Change History (30)

comment:1 Changed 5 years ago by nickm

It appears I started some work here in a branch called 'ed25519'. But now I'm starting a new branch, 'ed25519_ref10', thinking that we should start by using the ref10 implementation by default, and only doing something more complex if this turns out to matter in our profiles. (see comment https://trac.torproject.org/projects/tor/ticket/8897#comment:15 for the dangers of premature optimization here.)

comment:2 Changed 5 years ago by nickm

Branch 'ed25519_ref10' is starting to come together. It's got the ed25519 operations I believe we need. Before putting it up for review, I want to implement encrypted storage for ed25519 secret keys, and do more testing, though.

comment:3 Changed 5 years ago by nickm

ed25519_ref10 is merged. The work-in-progress branch here is now 12498_ed25519_keys.

comment:4 Changed 5 years ago by nickm

I've also started tracking the changes this branch is making to 220 as I go and implement it in a torspec branch called "220_evolution"

comment:5 Changed 5 years ago by nickm

The branches now included an implementation of proposal 228.

comment:6 Changed 5 years ago by nickm

The branch on Tor is now 12498_ed25519_keys_v2 : It's been through a 'rebase -i --autosquash master' so it can pull in the testing changes from #11243 and fix the conflicts with that branch. Still a work in progress.

comment:7 Changed 5 years ago by nickm

The branch on Tor is now 12498_ed25519_keys_v3 : It's been through a 'rebase -i --autosquash master' so it can synchronize with #13286. Still in progress.

comment:8 Changed 5 years ago by nickm

Remaining tasks:

  • Implement replaced identity voting.
  • Check test coverage; tests for all non-covered lines.
  • Change handling of certificate validity times in descriptors so that a descriptor is either valid or not.
  • Make sure that we regenerate new signing keys as needed.
  • Only try to load master key in init_ed_keys() when we're making a signing key cert.
  • Validate extrainfo items better:
    • when validating, reject the extrainfo if the ri has an ed key and the ei doesn't.
    • when validating, reject the extrainfo if the keys mismatch.
    • check sha256 digest for match too.
  • Link handshake implementation
    • Send ed key during link handshake in certs cells.
    • Generate ed->link certification.
    • check ed->link certification as well as rsa certification if offered.
    • set ed identity on connections.
    • offer an auth method that uses ed keys.
    • implement auth method that uses ed keys.
    • use it as possible.
    • set ed identity on connections

For another ticket:

  • Support offline master identity keys.
  • Support new link identifiers in EXTEND cells
  • Write a proposal for eventually removing RSA1024 identities
  • Write a proposal for deprecating TAP
  • User interface for identifying nodes by ed25519 key
  • Family support for ed25519 keys (or some other family thing)
  • Other stuff in prop220
  • Controller interface
  • Bridge identity support with ed25519
Last edited 5 years ago by nickm (previous) (diff)

comment:9 Changed 5 years ago by nickm

Had to rebase again to pick up digest256map. Now it's 12498_ed25519_keys_v4

comment:10 Changed 5 years ago by nickm

Owner: set to nickm
Priority: normalmajor
Status: newassigned
Type: defecttask

comment:11 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final

comment:12 Changed 5 years ago by nickm

And now that 0.2.6 is close to done, the branch is rebased once more, as 12498_ed25519_keys_v5

comment:13 Changed 5 years ago by nickm

So, I should re-summarize status:

This is NOT a complete implementation of proposal 220.

This branch DOES contain a complete implementation of proposal 228, and an initial implementation of proposal 220: the parts about managing ed25519 keys, advertising them, signing with them, pinning them, and voting on them.

Still to implement in this ticket:

  • Change handling of certificate validity times in descriptors so that a descriptor is either valid or not.
  • Make sure that we regenerate new signing keys as needed.
  • Only try to load master key in init_ed_keys() when we're making a signing key cert.

Still to implement in other tickets:

  • Link handshake.
  • Circuit extension handshake
  • Support offline master identity keys.
  • Support new link identifiers in EXTEND cells
  • Write a proposal for eventually removing RSA1024 identities
  • Write a proposal for deprecating TAP
  • User interface for identifying nodes by ed25519 key
  • Family support for ed25519 keys (or some other family thing)
  • Other stuff in prop220
  • Controller interface
  • Bridge identity support with ed25519

comment:14 Changed 5 years ago by nickm

Parent ID: #15054

comment:15 Changed 5 years ago by nickm

Status: assignedneeds_review

Okay, I'm fairly sure that I've opened tickets for all the above things, or fixed them in the branch.

comment:16 Changed 5 years ago by nickm

(And now it passes unit tests.)

comment:17 Changed 5 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:18 Changed 5 years ago by isabela

Keywords: SponsorU added
Points: large
Version: Tor: 0.2.7

comment:19 Changed 4 years ago by nickm

12498_ed25519_keys_v5 now works with chutney; I've added a few little fixups.

comment:20 Changed 4 years ago by andrea

Partial code review!

Begin code review for nickm's 12498_ed25519_keys_v5 branch:

f253aef14faf7640f94e9e76947b6a4461c3c1a4:

 - This just renames a struct and some associated functions.  Looks fine.

a9720b90f860323781d37dbba6ce04f312ec3632:

 - Whitespace changes for make check-spaces compliance after previous
   commit.  This looks fine.

cf9d780b570fa3ebf02e555c45f62d8b1bc38bcf:

 - Substantive new additions here are src/or/routerkeys.{c,h},
   src/or/torcert.{c,h} and src/trunnel/ed25519_cert.{c,h}, and
   some unit tests.

 - In routerkeys.c:

   - ed_key_init_from_file() looks okay
   - ed_key_new() looks okay
   - load_ed_keys() look okay modulo the two XXXX comments
   - get_master_identity_key(), get_master_signing_keypair(),
     get_master_signing_key_cert(), get_current_link_keypair(),
     get_current_auth_keypair(), get_current_ink_key_cert() and
     get_current_auth_key_cert() all are very simple and look good.
   - routerkeys_free_all() looks okay.

 - There are only function declarations and a few straightforward #defines
   in routerkeys.h

 - In torcert.c:

   - tor_cert_sign_impl() leaks memory (encoded is never freed), but otherwise
     appears correct
   - tor_cert_create() is a straightforward call-through to
     tor_cert_sign_impl()
   - tor_cert_free() looks fine
   - tor_cert_parse() looks good
   - tor_cert_get_checkable_sig() looks good
   - tor_cert_checksig() looks good

 - In torcert.h there just function declarations and the tor_cert_t type
   for torcert.c; all looks fine.

 - src/trunnel/ed25519_cert.{c,h} are generated files

567e42e894c2d06f3934bc90f7f75c9154481023:

 - This commit implements ed25519 signatures on router descriptors

 - Adds the crypto_digest_smartlist_prefix() utility function in
   src/commom/crypto.c; looks correct but comment doesn't describe
   the new prepend arg.

 - The new base64_decode_nopad()/base64_encode_nopad() functions look good.

 - ed25519_pubkey_eq() in src/common/crypto_ed25519.c looks okay

 - Why are ed25519_signature_from/to_base64() declared in crypto_ed25519.h,
   but defined in crypto_format.c?

 - New tor_cert_dup() utility function in torcert.c looks fine

 - The ed25519_signature_from/to_base64() functions look okay

 - Changes to router.{c,h} to emit ed25519 public keys and signatures look correct

 - Ed25519 parse/verify code in routerparse.c looks correct

c461287653541a05deab32ff9dd719cc4dd25352:

 - This commit cross-certifies identity keys with onion keys

 - A bunch of formatting/constness changes in src/common/crypto.{c,h}; these
   look fine.

 - Code to emit cross-cert lines in router.c looks okay

 - Code to generate/verify cross-cert signatures in routerkeys.{c,h} looks
   okay

 - Parse/cross-verify code in routerparse.c looks okay

f7931c11cb37c4e1f6d85800ae113b43df44d9f6:

 - Key-pinning mechanism; I presume 'associated Ed25519 key' in commit
   message should be 'associated RSA key'

 - The new keypin.c code all looks fine to me

1e3a98f88d5e19239d00356d50f6b598a681d70c:

 - This uses the keypin.c code from the previous commit to enforce
   matching RSA-1024 identities with Ed25519 identities in dirserv.c;
   all looks good.

 - As a question of sysadminning the dirauths, one probably wants a way
   to keep backups of the keypin journal, and copying it out from under
   a running Tor process might lead to a corrupt copy with partially
   written lines.  Should we consider making any provision for backups
   of the keypin journal without stopping the dirauth's Tor process?

bf43f2d853928d5e8e3a314dc506b73b6965fb49:

 - This is fixing a bug in a somewhat fragile bit of parsing code introduced
   in 567e42e894c2d06f3934bc90f7f75c9154481023, and introducing a new
   smartlist_pos() function in the process.  Looks good to me.

41cbaf0f267b0d1831aa3cf42e9d279cb171bc6a:

 - We're switching microdescriptors in votes over to containing ed25519 lines
   instead of rsa1024 lines if we have a recent enough consensus method; are
   we sure instead of rather than in addition to is the right choice here?

 - The microdesc.c and routerparse.c changes look okay to me

72d0d2c9c44cb6df47b35c07f94898f952a52fbc:

 - Holy giant chunk of generated code, Batman

 - Are we sure checking generated files into the repository like this is
   the right thing vs. generating them at build time?

 - src/trunnel/link_handshake.trunnel is the only thing in here that isn't
   generated, and it looks fine.

199b84cc67326b8cfbb13387a51f696339e5e0aa:

 - A big chunk of unit tests for link handshake stuff, together with some
   trivial changes for mockability in tortls.{c,h}, channeltls.{c,h} and
   connection_or.{c,h}.  It all looks okay to me modulo the memory leaks fixed
   in df1562811712d382ad26dda81cf117e3a98ccbb6.

822104f99fb4715afa147b45aae94584db281069:

 - More of the above: new unit tests plus trivial changes for mockability; it
   all looks good.

df1562811712d382ad26dda81cf117e3a98ccbb6:

 - Fixes some test suite memory leaks introduced in
   199b84cc67326b8cfbb13387a51f696339e5e0aa

b52da5b56ea9ca45959400299f0d8ac494a8a1d3:

 - This one patches channel_tls.c and connection_or.c to use the generated
   parsing code in 72d0d2c9c44cb6df47b35c07f94898f952a52fbc, validated by
   the unit tests introduced in 199b84cc67326b8cfbb13387a51f696339e5e0aa
   and 822104f99fb4715afa147b45aae94584db281069.  It all looks okay to me.

4d2c3ba3c86a8754842ea11f7371981d2264395a:

 - This patch adds support for generating ed25519 signatures on router
   descriptors to makedesc.py for use by the unit tests.  Looks fine to me
   except for a few things covered in the subsequent commit.

c98b6874cf00491ff40b1470619aaab9059e9e33:

 - This adds support for ed25519 signatures on extra-info documents,
   and a few bug fixes on the preceding and new unit tests.

 - In router.c, this makes the following changes:

   - Modifies router_rebuild_descriptor() to set the new signing_key_cert
     field of extrainfo_t, and pass the keypair in a new arg of
     extrainfo_dump_to_string() to perform the actual signing, and copies
     around extrainfo hashes in appropriate places.  This all looks
     correct.

   - Modifies router_dump_router_to_string() to keep a copy of an extrainfo
     line which can include a signature rather than just a digest.  This
     looks fine.

   - Modifies extrainfo_dump_to_string() to take a signing keypair as a new
     argument, and when this and a signing cert are available, make a
     signature.  This looks okay.

 - In routerlist.c, this frees extrainfo->signing_key_cert in extrainfo_free();
   this looks fine.

 - In routerparse.c, this makes the following changes:

   - Adds identity-ed25519 and router-sig-ed25519 to the extrainfo token
     table.

   - Parses the extrainfo sha256 digest in router_parse_entry_from_string()

   - Adds new code to parse and verify ed25519 signatures in
     extrainfo_parse_entry_from_string()

   - All these changes look fine.

3b1e0e2374225ab483ccd632741ffe1f618a7b87:

 - This adds consistency checks between a routerinfo_t and its associated
   extrainfo_t for the SHA256 and for use of the same signing key certificate
   on both in routerinfo_incompatible_with_extrainfo() of routerlist.c, and
   tor_cert_eq()/tor_cert_opt_eq() utility functions in torcert.c.  All this
   looks good.

54eb95a777dd585112e3f0af4d32e7f6dbacb88d:

 - This patch adds code to dirserv.c and routerparse.c to emit/parse
   ed25519 identities in votes, and also some commented-out (enabled in
   the next patch) code to dirvote.c that will be concerned with
   mapping between different identities.

 - dirserv.c/routerparse.c changes look good

 - The dirvote.c code here is best considered in light of subsequent changes
   in 4198cba16944d7f9172e850de12285e3995f7e1b.

4198cba16944d7f9172e850de12285e3995f7e1b:

 - This is a refactor of dirvote.c that introduces a notion of routerstatus
   collation, with a sorted list of all identities (currently RSA keys) and
   a mechanism to query for a list of all vote_routerstatus_t objects for a
   particular identity.  The approach seems sound and the implementation
   appears correct.

 - In dirvote.c, this patch removes the commented-out code introduced in
   54eb95a777dd585112e3f0af4d32e7f6dbacb88d and replaces it with the new
   dircollator_t abstraction, and then refactors the loop in
   networkstatus_compute_consensus to use dircollator_t to find all votes
   for a particular router.  This looks good to me.

f7eee5d22d73052b1da661e1ce85a4c69e5b51d1:

 - This adds a new dircollator_collate_by_ed25519() collation method, and
   uses it when we have a supportive consensus_method.

 - A new index on (rsa identity, ed25519 identity) pairs is added for
   the vote_routerstatus_t objects, and a new
   dircollator_collate_by_ed25519() function implements the new collation
   algorithm.  This all looks good to me.

d89b55a047206f636d7a3fd0cb058b72a53d02bd:

 - TODO

931901b09f97136a9456bfdcc14f5a13849e5fa7:

 - TODO

46c53edbe0ca7ff3e93fad16a960b28e56ada5bb:

 - TODO

2b87b52c88008bff97b58e69b8567ab57fdb379e:

 - TODO

b3ed7ffa5e8f633b7bd586e669571b5a83cfcef9:

 - TODO

e8708077fc9390aa4e8c6465e5b1e1c4d17a2255:

 - TODO

91bd035e21395edc11c692457bfd2c9034e09cde:

 - TODO

d99a84307a7dd2248536b751c65dea8cc51222cc:

 - TODO

660fff9e5b6cde9c43c87335c1e2661455b90317:

 - TODO

9641ea395b93ba444e9ab508ff4697ac34d0fed3:

 - TODO

ba911b29410c6b8f874beedaec072a10e2038da9:

 - TODO

66772a26d8d4c662b41b7522075771c8697006b9:

 - TODO

09fa94086aa1793a0f24fc81f4c9b49f66ba6c9f:

 - TODO

End code review

comment:21 Changed 4 years ago 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.

comment:22 in reply to:  20 Changed 4 years ago by nickm

Replying to andrea:

Partial code review!

cf9d780b570fa3ebf02e555c45f62d8b1bc38bcf:

  • tor_cert_sign_impl() leaks memory (encoded is never freed), but otherwise appears correct

Fixed in 52c4106305d87a9be5e9437c1b529a70b4b82c46

1e3a98f88d5e19239d00356d50f6b598a681d70c:

  • As a question of sysadminning the dirauths, one probably wants a way to keep backups of the keypin journal, and copying it out from under a running Tor process might lead to a corrupt copy with partially written lines. Should we consider making any provision for backups of the keypin journal without stopping the dirauth's Tor process?

I thought about this, and the best solution I could come up with was to treat unreadable lines as bogus, and to prepend a newline on startup. Perhaps we should open a ticket to find a better way?

41cbaf0f267b0d1831aa3cf42e9d279cb171bc6a:

  • We're switching microdescriptors in votes over to containing ed25519 lines instead of rsa1024 lines if we have a recent enough consensus method; are we sure instead of rather than in addition to is the right choice here?

Pretty sure. The RSA1024 lines are redundant with the RSA identities in the consensus; they are only there now to make sure that two different descriptors from different routers will always produce different microdescriptors. (See bug #11743 and commit 4a621a50f53ebeac62d30f427c2db0c627f80a31 for background.)

72d0d2c9c44cb6df47b35c07f94898f952a52fbc:

  • Are we sure checking generated files into the repository like this is the right thing vs. generating them at build time?

No, but I think it will be easier to switch post-facto.

I've gone for the current approach since I want to freeze us to the code generated by a particular version of Trunnel with a particular version of Tor by default, and because I don't expect every developer to have to install trunnel. But see #16199 and #16202 for an alternative.

I think that covers all the questions and suggestions from your review so far, but please let me know if I missed some?

comment:23 in reply to:  21 Changed 4 years ago by nickm

Replying to 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.

comment:24 Changed 4 years ago by dgoulet

Continuing review from yesterday:

  • 4d2c3ba3c86a8754842ea11f7371981d2264395a

lgtm

  • c98b6874cf00491ff40b1470619aaab9059e9e33

lgtm

  • 3b1e0e2374225ab483ccd632741ffe1f618a7b87

lgtm

  • 54eb95a777dd585112e3f0af4d32e7f6dbacb88d

lgtm

  • 4198cba16944d7f9172e850de12285e3995f7e1b
  • d89b55a047206f636d7a3fd0cb058b72a53d02bd
  • 931901b09f97136a9456bfdcc14f5a13849e5fa7
  • 46c53edbe0ca7ff3e93fad16a960b28e56ada5bb
  • 2b87b52c88008bff97b58e69b8567ab57fdb379e

Fixes and patch lgtm.

  • b3ed7ffa5e8f633b7bd586e669571b5a83cfcef9
  • e8708077fc9390aa4e8c6465e5b1e1c4d17a2255
  • 91bd035e21395edc11c692457bfd2c9034e09cde

lgtm

  • d99a84307a7dd2248536b751c65dea8cc51222cc
  • 660fff9e5b6cde9c43c87335c1e2661455b90317
    • I don't see any manual page entry for these option. On purpose maybe?
      +  V(SigningKeyLifetime,              INTERVAL, "30 days"),
      +  V(TestingLinkKeyLifetime,          INTERVAL, "2 days"),
      +  V(TestingAuthKeyLifetime,          INTERVAL, "2 days"),
      +  V(TestingLinkKeySlop,              INTERVAL, "3 hours"),
      +  V(TestingAuthKeySlop,              INTERVAL, "3 hours"),
      +  V(TestingSigningKeySlop,           INTERVAL, "1 day"),
      
    • config.c: Typo in REJECT message, missing "Testing":
      +  if (options->TestingLinkKeyLifetime < options->TestingAuthKeySlop*2)
      +    REJECT("LinkKeyLifetime is too short.");
      +  if (options->TestingAuthKeyLifetime < options->TestingLinkKeySlop*2)
      +    REJECT("AuthKeyLifetime is too short.");
      

lgtm

  • 9641ea395b93ba444e9ab508ff4697ac34d0fed3
  • 66772a26d8d4c662b41b7522075771c8697006b9
    • router.c: My guess here is that is a forgotten artefact? :)
       assert_identity_keys_ok(void)
       {
      +  if (1)
      +    return;
      

lgtm

  • ba911b29410c6b8f874beedaec072a10e2038da9

trivial fix. lgtm

  • 09fa94086aa1793a0f24fc81f4c9b49f66ba6c9f

lgtm

Final note, I reviewed the code correctness and what I could find in the proposals but keep in mind I'm not in a position to fully review the design nor comment on it. This is really a tor subsystem I am quite unfamiliar with :S.

comment:25 Changed 4 years ago by andrea

By request, the same code review with just the stuff I was bothered about:

Code review for nickm's 12498_ed25519_keys_v5 branch - just the complaints
version:

cf9d780b570fa3ebf02e555c45f62d8b1bc38bcf:

 - In routerkeys.c:

   - load_ed_keys() look okay modulo the two XXXX comments

 - In torcert.c:

   - tor_cert_sign_impl() leaks memory (encoded is never freed), but otherwise
     appears correct

567e42e894c2d06f3934bc90f7f75c9154481023:

 - Adds the crypto_digest_smartlist_prefix() utility function in
   src/commom/crypto.c; looks correct but comment doesn't describe
   the new prepend arg.

 - Why are ed25519_signature_from/to_base64() declared in crypto_ed25519.h,
   but defined in crypto_format.c?

f7931c11cb37c4e1f6d85800ae113b43df44d9f6:

 - Key-pinning mechanism; I presume 'associated Ed25519 key' in commit
   message should be 'associated RSA key'

1e3a98f88d5e19239d00356d50f6b598a681d70c:

 - As a question of sysadminning the dirauths, one probably wants a way
   to keep backups of the keypin journal, and copying it out from under
   a running Tor process might lead to a corrupt copy with partially
   written lines.  Should we consider making any provision for backups
   of the keypin journal without stopping the dirauth's Tor process?

41cbaf0f267b0d1831aa3cf42e9d279cb171bc6a:

 - We're switching microdescriptors in votes over to containing ed25519 lines
   instead of rsa1024 lines if we have a recent enough consensus method; are
   we sure instead of rather than in addition to is the right choice here?

72d0d2c9c44cb6df47b35c07f94898f952a52fbc:

 - Are we sure checking generated files into the repository like this is
   the right thing vs. generating them at build time?

End code review

comment:26 Changed 4 years ago by asn

Hello, here is a small code review of the current 12498_ed25519_keys_v5 branch up to b52da5b. I might get to the rest of the branch but not sure if it's going to happen RSN.

  • I think there is a memleak when parsing identity lines on microdescriptors. The code seems to allow multiple such identity lines, but then for each one we do
md->ed25519_identity_pkey = tor_memdup(&k, sizeof(k));

without first cleaning the previously memdup'ed memory. Why would multiple id ed25519 lines be allowed?

  • I'm not sure what this 'tag' thing is on ed_key_init_from_file(). I don't see it explained anywhere.
  • ed25519_cert_encoded_len() can return -1 but it's retval is never checked. I think that's OK because all its input is generated by us, but might as well mention it here.
  • Comment typo "not just and" at:
    /** Helper for tor_cert_create(): signs any 32 bytes, not just and ed25519
     * key.
     */
    static tor_cert_t *
    tor_cert_sign_impl(const ed25519_keypair_t *signing_key,
    

comment:27 in reply to:  26 Changed 4 years ago by nickm

Replying to asn:

Hello, here is a small code review of the current 12498_ed25519_keys_v5 branch up to b52da5b. I might get to the rest of the branch but not sure if it's going to happen RSN.

  • I think there is a memleak when parsing identity lines on microdescriptors. The code seems to allow multiple such identity lines, but then for each one we do
md->ed25519_identity_pkey = tor_memdup(&k, sizeof(k));

without first cleaning the previously memdup'ed memory. Why would multiple id ed25519 lines be allowed?

Should be fixed in e58e44cbe3822508e185e3c13af4013df97ead71

  • I'm not sure what this 'tag' thing is on ed_key_init_from_file(). I don't see it explained anywhere.

It's documented for crypto_read/write_tagged_contents_to/from_file

  • ed25519_cert_encoded_len() can return -1 but it's retval is never checked. I think that's OK because all its input is generated by us, but might as well mention it here.

8bff91adbdb5e9af9e1152cecc30a34c6af8a5e2 should fix this.

  • Comment typo "not just and" at:

19ed5341751e09c2ecbe0757641376e096e0d0d3 should fix this.

comment:28 in reply to:  24 Changed 4 years ago by nickm

Replying to dgoulet:

  • 660fff9e5b6cde9c43c87335c1e2661455b90317
    • I don't see any manual page entry for these option. On purpose maybe?
      +  V(SigningKeyLifetime,              INTERVAL, "30 days"),
      +  V(TestingLinkKeyLifetime,          INTERVAL, "2 days"),
      +  V(TestingAuthKeyLifetime,          INTERVAL, "2 days"),
      +  V(TestingLinkKeySlop,              INTERVAL, "3 hours"),
      +  V(TestingAuthKeySlop,              INTERVAL, "3 hours"),
      +  V(TestingSigningKeySlop,           INTERVAL, "1 day"),
      

Whoops; should be fixed in 96ada96

  • config.c: Typo in REJECT message, missing "Testing":

3163a7b312451896bd3c3d401dd0df8f8ff2d854 should fix.

  • 9641ea395b93ba444e9ab508ff4697ac34d0fed3
  • 66772a26d8d4c662b41b7522075771c8697006b9
    • router.c: My guess here is that is a forgotten artefact? :)
       assert_identity_keys_ok(void)
       {
      +  if (1)
      +    return;
      

lgtm

  • ba911b29410c6b8f874beedaec072a10e2038da9

trivial fix. lgtm

  • 09fa94086aa1793a0f24fc81f4c9b49f66ba6c9f

lgtm

Final note, I reviewed the code correctness and what I could find in the proposals but keep in mind I'm not in a position to fully review the design nor comment on it. This is really a tor subsystem I am quite unfamiliar with :S.

comment:29 Changed 4 years ago by nickm

Now squashed in 12498_ed25519_keys_v6

comment:30 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged!

Note: See TracTickets for help on using tickets.