[tor-bugs] #12498 [Tor]: Implement ed25519 identity keys (prop 220)
Tor Bug Tracker & Wiki
blackhole at torproject.org
Mon May 25 04:45:16 UTC 2015
#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 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
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12498#comment:20>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list