Opened 2 years ago

Last modified 29 hours ago

#22029 needs_review enhancement

Allow ed25519 keys to be banned in the approved-routers file

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 034-triage-20180328, 034-removed-20180328
Cc: neel Actual Points:
Parent ID: Points: 1
Reviewer: asn Sponsor:

Description

Once we start pinning ed25519 keys, we should probably also allow directory authorities to ban them in approved-routers. This will allow us to migrate away from RSA keys at some point.

(Where else do we use RSA fingerprints?)

Child Tickets

Change History (25)

comment:1 Changed 2 years ago by dgoulet

Oh fine idea!

Quick question here. Can a relay have N rsa keys (for N > 1) for 1 ed25519 key and still keep it's uptime/weight?

I'm asking here because we currently block by RSA fingerprint but what if I can rotate that everyday (or when blocked) but still keep my consensus weight because my ed25519 is still recognized by dirauths?

comment:2 in reply to:  1 Changed 23 months ago by teor

Replying to dgoulet:

Oh fine idea!

Quick question here. Can a relay have N rsa keys (for N > 1) for 1 ed25519 key and still keep it's uptime/weight?

Yes, but not for long.

The directory authorities keep a key pinning journal, but don't enforce it.

When we turn on key pinning, authorities won't vote for relays that change one key and keep the other the same.

I'm asking here because we currently block by RSA fingerprint but what if I can rotate that everyday (or when blocked) but still keep my consensus weight because my ed25519 is still recognized by dirauths?

The bandwidth script uses RSA fingerprints, so changing your RSA removes all your bandwidth.

In the far future, when we remove RSA keys, we will want to have a file that bans both RSA and ed25519 keys, to make the transition easier.

comment:3 Changed 18 months ago by dgoulet

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

comment:4 Changed 14 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Deferring various "new"-status enhancement tickets to 0.3.4

comment:5 Changed 12 months ago by nickm

Keywords: 034-triage-20180328 added

comment:6 Changed 12 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:7 Changed 11 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:8 Changed 5 months ago by neel

Owner: set to neel
Status: newassigned

comment:9 Changed 5 months ago by neel

Cc: neel added

comment:10 Changed 6 weeks ago by neel

The function dirserv_load_fingerprint_file() reads the file approved-routers. I have a few questions:

  1. Should the ed25519 key in the approved-routers file be a base16-encoded key (similar to what we do right now with RSA fingerprints)?
  2. Would it be okay that if a ed25519 key was given, I check keypin hashtable to get the relay's corresponding RSA key and then add it to the list? I propose will be done with a new function that searches the ed25519 keypin hash table for each entry until a matching ed25519 key is given, and then return a corresponding RSA key.

I am concerned with Point 2 however because of the O(n2) running time from needing to go through the list of all Tor relays. Another concern is that mapping ed25519 to RSA could mean we prolong the life of the RSA code.

Would it be better to overhaul the relay data structures to be ed25519-first and then do this?

comment:11 in reply to:  10 Changed 6 weeks ago by nickm

Replying to neel:

The function dirserv_load_fingerprint_file() reads the file approved-routers. I have a few questions:

  1. Should the ed25519 key in the approved-routers file be a base16-encoded key (similar to what we do right now with RSA fingerprints)?

I think it should base64-encoded; that's how ed25519 keys are printed everywhere else in Tor.

  1. Would it be okay that if a ed25519 key was given, I check keypin hashtable to get the relay's corresponding RSA key and then add it to the list? I propose will be done with a new function that searches the ed25519 keypin hash table for each entry until a matching ed25519 key is given, and then return a corresponding RSA key.

I think it should just store the ed25519 keys, and look up by the ed25519 key.

I am concerned with Point 2 however because of the O(n2) running time from needing to go through the list of all Tor relays. Another concern is that mapping ed25519 to RSA could mean we prolong the life of the RSA code.

Would it be better to overhaul the relay data structures to be ed25519-first and then do this?

I think that might be a good idea, but instead of being ed25519-only, we should make it handle both ed25519 _and_ RSA keys.

comment:12 Changed 6 weeks ago by neel

Thank you for your responses, nickm!

I do have a few more questions, however:

I think it should just store the ed25519 keys, and look up by the ed25519 key.

Does that mean something like authdir_config_t should be done for ed25519 (even if the digests are stored as a union), and then introduce a function like dirserv_get_status_impl() that takes in ed25519 keys. And then I should make dirserv_router_get_status() look at ed25519 keys as well?

comment:13 Changed 6 weeks ago by nickm

I would suggest adding a third element to authdir_config_t, a digest256map_t, mapping ed25519 identities to router_status_t elements. Would that work?

comment:14 Changed 6 weeks ago by neel

I would suggest adding a third element to authdir_config_t, a digest256map_t, mapping ed25519 identities to router_status_t elements. Would that work?

Yes. I thought about doing the same thing as well (even before you responded).

comment:15 Changed 6 weeks ago by neel

My patch is ready and the PR is here: https://github.com/torproject/tor/pull/682

comment:16 Changed 6 weeks ago by neel

Status: assignedneeds_review

comment:17 Changed 6 weeks ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.1.x-final

comment:18 Changed 5 weeks ago by dgoulet

Reviewer: teor

comment:19 Changed 3 weeks ago by teor

Reviewer: teor

I don't have time to do this review next week: I need to focus on #23588 (large, IPv6, needs tests) and #29280 (chutney).

comment:20 Changed 2 weeks ago by asn

Reviewer: asn

comment:21 Changed 2 weeks ago by asn

Status: needs_reviewmerge_ready

Code looks reasonable in principle. Added some tech-debt related concerns to the PR. Let me know if you want to fix these, thanks!

comment:22 Changed 2 weeks ago by asn

Status: merge_readyneeds_revision

Oops, meant to toggle to needs_revision.

Last edited 2 weeks ago by asn (previous) (diff)

comment:23 Changed 2 weeks ago by neel

Status: needs_revisionneeds_review

I have simplified the if loop and pushed it.

I do not plan to write a test for add_ed25519_to_dir().

comment:24 in reply to:  23 Changed 5 days ago by asn

Status: needs_reviewneeds_revision

Replying to neel:

I have simplified the if loop and pushed it.

I do not plan to write a test for add_ed25519_to_dir().

Hello neel,

sounds good wrt not writing tests.

However, I still feel that the parts of dirserv_load_fingerprint_file() that got changed are still much measier than before. In particular:

  • It's not clear to me what happens if is_rsa and is_ed25519 are false. Is this case handled? Should it be handled?
  • It's not clear to me why the if statement needs to be if ((a||b)||c). Is this equivalent to (a||b||c)? Why the extra parentheses or why so many stuff in the same if? It's not clear to me what it's trying to do. Like, will it send a log_notice everytime is_rsa is true? That does not seem like the previous behavior. Please it would be great if this could be simplified and also a comment added (perhaps even in its own function).

My suggestion would be to move the nickname checking above the key-type checking and then do stuff based on the key-type more cleanly and more commented.

Sorry for the nitpicks. Thanks! :)

comment:25 Changed 29 hours ago by neel

Status: needs_revisionneeds_review

That's fine.

I have updated my code and pushed it.

Note: See TracTickets for help on using tickets.