Opened 2 years ago

Closed 6 months ago

Last modified 6 months ago

#15055 closed enhancement (implemented)

Implement ed25519 link handshake

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Normal Keywords: tor-relay, prop-220, 027-triaged-1-in, 028-triaged, 201511-deferred, 201512-deferred, tor-crypto-identity, tor-ed25519-proto, TorCoreTeam201609, nickm-deferred-20161005, review-group-11
Cc: Actual Points:
Parent ID: #15054 Points: parent
Reviewer: isis Sponsor: SponsorU-must

Description

In #12498 , we implement a new identity key type. Now it's time to use it in a proper handshake as documented in proposal 220.

Child Tickets

TicketTypeStatusOwnerSummary
#13752enhancementclosednickmExtend TLS RSA link keys to 2048-bit
#16035defectclosednickmImplement proposal 244: RFC5705 for exporting key material in tls handshake
#17779defectclosednickmMemory leak in routerkeys.c
#19020defectclosednickmRSA cross-certification of ed25519 keys differs from spec
#19155enhancementclosednickm[prop220] send CERTS cells correctly for Ed25519
#19156enhancementclosednickm[prop220] send AUTHENTICATE cells with correct ed25519 signatures
#19157enhancementclosednickm[prop220] Check all new certificate types (incl cross-cert and ed25519)
#19158enhancementclosednickm[prop220] Understand and validate ed25519-signed AUTH0002 cells
#19159enhancementclosednickm[prop220] Add ed25519-ID field to channel and/or or_connection
#19160enhancementclosednickm[prop220] Advertise support for Ed25519 link authentication
#19470defectclosednickmMatch 15055_wip commits to #15055 subtickets
#19958enhancementclosednickmImplement proposal 264 (protocol versioning)
#20027defectclosednickmEd25519 certificate parsing does badly with expirations after 2038
#20051taskclosednickmUnit tests for ed25519 link handshake code

Change History (46)

comment:1 Changed 2 years ago by nickm

  • Status changed from new to assigned

comment:2 Changed 2 years ago by nickm

  • Keywords 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:3 Changed 2 years ago by isabela

  • Keywords SponsorU added
  • Points set to large
  • Priority changed from normal to major
  • Version set to Tor: 0.2.7

comment:4 Changed 2 years ago by nickm

  • Owner set to nickm

comment:5 Changed 22 months ago by nickm

  • Keywords TorCoreTeam201507 added

comment:6 Changed 21 months ago by nickm

  • Keywords TorCoreTeam201508 added; TorCoreTeam201507 removed

comment:7 Changed 20 months ago by nickm

  • Keywords TorCoreTeam201509 added; TorCoreTeam201508 removed
  • Milestone changed from Tor: 0.2.7.x-final to Tor: 0.2.8.x-final

comment:8 Changed 19 months ago by nickm

  • Keywords 028-triaged added

comment:9 Changed 19 months ago by nickm

  • Keywords SponsorU removed
  • Sponsor set to SponsorU

Bulk-replace SponsorU keyword with SponsorU field.

comment:10 Changed 18 months ago by nickm

  • Keywords TorCoreTeam201511 added

comment:11 Changed 18 months ago by nickm

  • Type changed from defect to enhancement

comment:12 Changed 17 months ago by nickm

  • Keywords TorCoreTeam201512 201511-deferred added; TorCoreTeam201511 removed

Bulk-move uncompleted items to december. :/

comment:13 Changed 16 months ago by nickm

  • Keywords TorCoreTeam201601 201512-deferred added; TorCoreTeam201512 removed

Perhaps in January?

comment:14 Changed 15 months ago by nickm

Bulk-modify: It is February 2016, and no longer possible that anything else will get done in January 2016. Time's arrow and all that.

comment:15 Changed 15 months ago by nickm

  • Keywords TorCoreTeam201602 added; TorCoreTeam201601 removed

comment:16 Changed 14 months ago by nickm

  • Milestone changed from Tor: 0.2.8.x-final to Tor: 0.2.9.x-final

These seem like features, or like other stuff unlikely to be possible this month. Bumping them to 0.2.9

comment:17 Changed 13 months ago by isabela

  • Sponsor changed from SponsorU to SponsorU-must

comment:18 Changed 13 months ago by nickm

  • Keywords TorCoreTeam201509 removed

Removing TorCoreTeam201509 from these tickets, since we do not own a time machine.

comment:19 Changed 13 months ago by nickm

  • Keywords TorCoreTeam201602 removed

comment:20 Changed 13 months ago by mikeperry

  • Keywords tor-crypto-identity added

comment:21 Changed 13 months ago by nickm

  • Keywords tor-ed25519-proto added

comment:22 Changed 12 months ago by nickm

  • Keywords TorCoreTeam201605 added

Give myself a few items for May. I hope I can do even more than this, but let's be careful.

comment:23 Changed 12 months ago by nickm

  • Severity set to Normal

I'm now working in 15055_wip, which I am rebasing a LOT. You probably don't want to look yet.

comment:24 Changed 12 months ago by isabela

  • Points changed from large to 6

comment:25 Changed 11 months ago by nickm

Okay, going to try to wrap all this up next week. That will be a challenge. Here are the pieces of it, to break it down into little edible bites:

  • Fix #19020.
  • Servers connecting to servers send all correct authentication information when protocol permits.
  • Servers receiving connections send all certificates (when protocol permits?)
  • Servers receiving connections from servers check authentication information, make correct decisions.
  • Anyone connecting to a server checks all certs (if provided)
  • Add an ed25519_id_key item to or_connection_t / channel_t.
  • Index those by ed25519 id.
  • (non)profit.

comment:26 Changed 11 months ago by nickm

  • Points changed from 6 to parent

comment:27 Changed 11 months ago by nickm

  • Keywords TorCoreTeam201605 removed

Remove "TorCoreTeam201605" keyword. The time machine is broken.

comment:28 Changed 8 months ago by nickm

Status: I believe I will finish the core implementation today or tomorrow. The testing and debugging and reviewing will take a bit longer.

comment:29 Changed 8 months ago by nickm

  • Keywords TorCoreTeam201609 added

Implementation is done here; more testing needed, though.

comment:30 Changed 8 months ago by nickm

  • Keywords nickm-check-done-20160905 added

Batch-tagging a bunch of tickets assigned to me: I believe these are all done or mostly done or done-enough.

comment:31 Changed 8 months ago by nickm

My unit tests now test the success cases of the code. Next, to test the failing cases (all of them).

comment:32 Changed 8 months ago by nickm

  • Status changed from assigned to needs_review

Now at long last ready for review.

My branch is feature_15055 in my public repository.

There is a gitlab PR to put comments on at: https://gitlab.com/nickm_tor/tor/merge_requests/7/commits

comment:33 Changed 8 months ago by nickm

  • Keywords review-group-9 added

comment:34 Changed 8 months ago by nickm

  • Keywords nickm-check-done-20160905 removed

comment:35 Changed 7 months ago by dgoulet

  • Status changed from needs_review to needs_revision

Review can be found in gitlab. Hope this help.

comment:36 Changed 7 months ago by isis

  • Reviewer set to isis
  • Status changed from needs_revision to needs_review

comment:37 Changed 7 months ago by isis

There's some docstrings for functions in src/or/routerkeys.c, which didn't have any before and which I ran into during my review, in my feature/15055 branch. (I also marked some other functions in that module with DOCDOC, because I didn't need to figure out what they do at the moment.)

I'm about halfway done with review (on gitlab).

comment:38 Changed 7 months ago by nickm

I just cherry-picked those commits onto my branch

comment:39 Changed 7 months ago by nickm

  • Keywords nickm-deferred-20161005 added
  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.3.0.x-final

Deferring big/risky-feature things (even the ones I really love!) to 0.3.0. Please argue if I'm wrong.

comment:40 Changed 7 months ago by nickm

  • Keywords review-group-10 added; review-group-9 removed

Moving not-reviewed-by-me tickets in review-group-9, and for-0.2.9/0.2.8 tickets, to review-group-10.

comment:41 Changed 6 months ago by nickm

  • Keywords review-group-11 added

Moving these to review-group-11.

comment:42 Changed 6 months ago by nickm

  • Keywords review-group-10 removed

comment:43 Changed 6 months ago by isis

  • Status changed from needs_review to merge_ready

Okay, review complete: https://gitlab.com/nickm_tor/tor/merge_requests/7

There's some minor complaints about documenting that we expect future AUTHCHALLENGE types to always be better than previously defined types, which is probably true, but if we're depending on that maybe we should note it?

Overall, it looks good and can be merged.

comment:44 Changed 6 months ago by nickm

I've added comments there, and squashed the branch into a feature15055_v2, and merged it. Tests pass!

comment:45 Changed 6 months ago by nickm

  • Resolution set to implemented
  • Status changed from merge_ready to closed

comment:46 Changed 6 months ago by teor

The LinkAuth protover was not updated in this patch, and it should have been. I opened #20578 to address that.

Note: See TracTickets for help on using tickets.