Opened 3 years ago

Closed 12 months ago

Last modified 12 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 3 years ago by nickm

Status: newassigned

comment:2 Changed 3 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:3 Changed 3 years ago by isabela

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

comment:4 Changed 2 years ago by nickm

Owner: set to nickm

comment:5 Changed 2 years ago by nickm

Keywords: TorCoreTeam201507 added

comment:6 Changed 2 years ago by nickm

Keywords: TorCoreTeam201508 added; TorCoreTeam201507 removed

comment:7 Changed 2 years ago by nickm

Keywords: TorCoreTeam201509 added; TorCoreTeam201508 removed
Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:8 Changed 2 years ago by nickm

Keywords: 028-triaged added

comment:9 Changed 2 years ago by nickm

Keywords: SponsorU removed
Sponsor: SponsorU

Bulk-replace SponsorU keyword with SponsorU field.

comment:10 Changed 2 years ago by nickm

Keywords: TorCoreTeam201511 added

comment:11 Changed 2 years ago by nickm

Type: defectenhancement

comment:12 Changed 23 months ago by nickm

Keywords: TorCoreTeam201512 201511-deferred added; TorCoreTeam201511 removed

Bulk-move uncompleted items to december. :/

comment:13 Changed 22 months ago by nickm

Keywords: TorCoreTeam201601 201512-deferred added; TorCoreTeam201512 removed

Perhaps in January?

comment:14 Changed 21 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 21 months ago by nickm

Keywords: TorCoreTeam201602 added; TorCoreTeam201601 removed

comment:16 Changed 20 months ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 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 19 months ago by isabela

Sponsor: SponsorUSponsorU-must

comment:18 Changed 19 months ago by nickm

Keywords: TorCoreTeam201509 removed

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

comment:19 Changed 19 months ago by nickm

Keywords: TorCoreTeam201602 removed

comment:20 Changed 19 months ago by mikeperry

Keywords: tor-crypto-identity added

comment:21 Changed 18 months ago by nickm

Keywords: tor-ed25519-proto added

comment:22 Changed 18 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 18 months ago by nickm

Severity: 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 17 months ago by isabela

Points: large6

comment:25 Changed 17 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 17 months ago by nickm

Points: 6parent

comment:27 Changed 17 months ago by nickm

Keywords: TorCoreTeam201605 removed

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

comment:28 Changed 14 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 14 months ago by nickm

Keywords: TorCoreTeam201609 added

Implementation is done here; more testing needed, though.

comment:30 Changed 14 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 14 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 13 months ago by nickm

Status: assignedneeds_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 13 months ago by nickm

Keywords: review-group-9 added

comment:34 Changed 13 months ago by nickm

Keywords: nickm-check-done-20160905 removed

comment:35 Changed 13 months ago by dgoulet

Status: needs_reviewneeds_revision

Review can be found in gitlab. Hope this help.

comment:36 Changed 13 months ago by isis

Reviewer: isis
Status: needs_revisionneeds_review

comment:37 Changed 13 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 13 months ago by nickm

I just cherry-picked those commits onto my branch

comment:39 Changed 13 months ago by nickm

Keywords: nickm-deferred-20161005 added
Milestone: Tor: 0.2.9.x-finalTor: 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 13 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 12 months ago by nickm

Keywords: review-group-11 added

Moving these to review-group-11.

comment:42 Changed 12 months ago by nickm

Keywords: review-group-10 removed

comment:43 Changed 12 months ago by isis

Status: needs_reviewmerge_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 12 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 12 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

comment:46 Changed 12 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.