Opened 5 years ago

Closed 4 years ago

#13642 closed enhancement (implemented)

Implement offline encrypted master keys for Ed25519 identities

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: tor-relay, prop-220, 027-triaged-1-in, SponsorU
Cc: sysrqb, isis Actual Points:
Parent ID: #15054 Points: small-remaning
Reviewer: Sponsor:


The ed25519 identity key proposal is written so that master identity keys are used only to certify medium-term signing keys, and medium-term signing keys are used to sign everything else.

To implement this on top of my branch for #12498, I'd suggest the following interface:

  • Have a 'tor --encrypt-master-key' command that you can run while a Tor server is _not_ running. It should prompt for a password, generate a new master key, and encrypt it with the crypto_pwbox functionality. It should have an option that says where to store the master key. It should refuse to run if the master key is already present. It should have an option to change the passphrase.
  • Have a 'tor --new-signing-key' command that generates a new signing key and certificate for our master key. It should take a number of days that the signing key should be value, with a default around 30 days. It shouldn't require that the Tor server not be running. It should have an option that says where to store the signing keys and certificates.
  • Every command that takes a password should:
    • Use the standard safety features for reading passwords securely from the command line. (There should be a wrapper function for doing this across different platforms in src/common.)
    • Have an option that specifies an fd on which a password will be provided.
    • Have documented error codes that can be used for shell scripts.
    • Call tor_mlockall() before doing anything.
    • Have a --no-passphrase option that uses an empty string for the passphrase.
  • Have a running Tor server check for a new signing key periodically, and on sighup.
  • Have a running Tor server warn the user periodically when the signing key certificate is going to expire soon.

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by isis

Cc: isis added

comment:2 Changed 4 years ago by nickm

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

comment:3 Changed 4 years ago by nickm

Keywords: tor-relay prop-220 added
Parent ID: #15054

comment:4 Changed 4 years ago by nickm

Status: newneeds_review

My branch 13642_offline_master_v1 (based on 12498_ed25519_keys_v5) has an implementation of this. The CLI might want some work.

comment:5 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:6 Changed 4 years ago by isabela

Keywords: SponsorU added
Points: small-remaning
Priority: normalmajor
Version: Tor: 0.2.7

comment:7 Changed 4 years ago by nickm

Now see 13642_offline_master_v2. (Rebased to master now that #12498 is merged.)

comment:8 Changed 4 years ago by dgoulet

Status: needs_reviewneeds_revision

Code review:

  • 4a79fad1d095221c80b00b6378a3dd246e358c50
    • In (nitpick) seems to have weird indentation issues:
               getifaddrs \
      +    getpass \
               getrlimit \
    • In common/compat.c: tor_getpass takes a ssize_t buflen, I think it should be a size_t because 1) that shouldn't be negative and 2) readpassphrase() takes a size_t as bufsiz.
    • In common/compat.c: tor_getpass(), I think readpassphrase() shouldn't echo the passphrase which can be fixed by using RPP_ECHO_OFF.
    • In common/compat.c: I agree with you that the alternatives are not pretty :)
  • d8628ee3d04e4ffe89430b12a0c2dae01d0ea5b4
    • In routerkeys.c: function read_encrypted_secret_key(), the while(1) readpassphrase, when we call crypto_unpwbox(), we don't handle UNPWBOX_BAD_SECRET so this means we are fine with an infinite loop or until the user ctrl+c or enters the right passphrase?
    • In routerkeys.c: function write_encrypted_secret_key(), this looks like a typo, should be pwbuf1 in the second memwipe I would imagine (and 0 instead of 1 maybe?)
      +  memwipe(pwbuf0, 0, sizeof(pwbuf0));
      +  memwipe(pwbuf0, 1, sizeof(pwbuf1));

The rest lgtm!

comment:9 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

Made those changes; thanks; looks better now?

comment:10 Changed 4 years ago by dgoulet


lgtm;! ACK

comment:11 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed


Note: See TracTickets for help on using tickets.