Opened 2 years ago

Closed 3 months ago

#17639 closed enhancement (fixed)

provide an option to display the expiry date of a given ed25519 signing key

Reported by: cypherpunks Owned by: isis
Priority: High Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.2-alpha
Severity: Normal Keywords: tor-ed25519-proto, review-group-21
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: nickm Sponsor: SponsorM-can

Description

Currently there is no way to see when the signing key is about to expire.

Adding such a feature would be great.

Child Tickets

Change History (31)

comment:1 Changed 21 months ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:2 Changed 21 months ago by cypherpunks

ref: #18228

comment:3 Changed 20 months ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

Throw most 0.2.8 "NEW" tickets into 0.2.9. I expect that many of them will subsequently get triaged out.

comment:4 Changed 19 months ago by isabela

Sponsor: SponsorU-can

comment:5 Changed 19 months ago by nickm

Points: small

comment:6 Changed 19 months ago by nickm

Priority: MediumHigh

comment:7 Changed 18 months ago by nickm

Keywords: tor-ed25519-proto added

comment:8 Changed 17 months ago by isabela

Points: small1

comment:9 Changed 17 months ago by andrea

Owner: set to andrea
Status: newassigned

Taking ownership for 0.2.9 triage

comment:10 Changed 14 months ago by nickm

Keywords: nickm-deferred-20160905 added
Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final

Deferring some andrea-assigned items from 0.2.9. Andrea, please move any of these back if you disagree; this is just a first approximation.

comment:11 Changed 10 months ago by dgoulet

Keywords: triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

Triaged out on December 2016 from 030 to 031.

comment:12 Changed 8 months ago by nickm

Sponsor: SponsorU-can

Clear sponsorS and sponsorU from open tickets in 0.3.1

comment:13 Changed 8 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: unspecified
Owner: andrea deleted

comment:14 Changed 5 months ago by cypherpunks

Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Version: Tor: 0.2.7.4-rcTor: 0.3.0.7

comment:15 Changed 5 months ago by arma

Usually, when moving a ticket to an upcoming milestone, it works best to move the ticket forward in some way too. :)

comment:16 Changed 5 months ago by nickm

Keywords: triage-out-030-201612 removed

comment:17 Changed 5 months ago by nickm

Keywords: nickm-deferred-20160905 removed

comment:18 Changed 5 months ago by nickm

Status: assignednew

Change the status of all assigned/accepted Tor tickets with owner="" to "new".

comment:19 Changed 3 months ago by isis

Owner: set to isis
Status: newaccepted

Hi! I took a stab at this in my bug17639 branch. I'm not totally sold on that command line flag or the implementation, so if someone can think of something more user friendly or intuitive, I'd be happy to hear better ideas!

Last edited 3 months ago by isis (previous) (diff)

comment:20 Changed 3 months ago by isis

Status: acceptedneeds_review
Version: Tor: 0.3.0.7Tor: 0.2.7.2-alpha

comment:21 Changed 3 months ago by nickm

Keywords: review-group-21 added

comment:22 Changed 3 months ago by nickm

Reviewer: nickm

comment:23 Changed 3 months ago by nickm

Status: needs_reviewneeds_revision

This looks comparatively solid to me! A few things to consider as possibilities, though maybe they're not needed:

  • Maybe this should printf() something to stdout, instead of using the log facility, and run at --quiet by default?
  • Maybe the output format should be machine-readable?
  • Maybe it should dump information about the installed authority auth key as well
  • I wonder what it should do about hidden service keys?
  • Technically speaking, keys don't expire: certificates do. The user needs to replace both of them, not just one.
  • The buffer in log_ed_key_expiration() can probably just be stack-allocated.
  • Documentation on the new option should go into the manpage


Please fix whatever from above you agree with. :)

comment:24 in reply to:  23 Changed 3 months ago by isis

Replying to nickm:

This looks comparatively solid to me! A few things to consider as possibilities, though maybe they're not needed:

  • Maybe this should printf() something to stdout, instead of using the log facility, and run at --quiet by default?


Yes, this makes sense. TBH, I didn't know if I was allowed/encouraged to do printf(), since it seems like there's a lot of ways "interfaces" over stdin/stdout can be bad/broken/wrong, particularly when they are relied upon by other scripts/programs (cf. gnupg). I think it does make sense though to work even with --quiet, and in this case the user is asking a question like "hey parse this thing and tell me what it says", not intending any operation or for the binary to run as a daemon or anything more complicated, so it makes sense here.

  • Maybe the output format should be machine-readable?


Yeah! But what should this look like?

Literally just spit out the expiry in ISO8601 format? (With or without the underscore in between the date and the time?) Or some more easily machine parseable (but less human readable) format, like seconds-since-epoch?

Should it be in the local timezone, or in UTC? (Probably UTC, right? if we expect scripts to be able to process it?)

  • Maybe it should dump information about the installed authority auth key as well
  • I wonder what it should do about hidden service keys?


Yes, I can add these. I thought about the first one before, but I didn't know how to make the interface, plus had worries about the patch being large/invasive. I think the "proper" way to do it would be to add a suboptions parser so that the user could be like "--key-expiration auth" or "--key-expiration sign"; this way, it would more easily extendable to further changes in supported cert types moving forward.

I'm not sure what to do about onion service keys/certs at all. I imagine there are users with multiple hidden services. Frankly, I didn't even know OS keys/certs could expire. Is that just a v2 thing? Is there some canonical way to refer to an onion service such that I could provide some option like "--key-expiration specifier-for-my-onion-service"? Should I optionally take an onion service's address and learn the keys from the configured onion service directory?

  • Technically speaking, keys don't expire: certificates do. The user needs to replace both of them, not just one.


Right. How should we communicate this to users? I'm not a UX person at all, but I can vaguely naïvely foresee confusion of like "but I was asking about my keys". Should I change to the cmdline flag to --cert-expiration?

  • The buffer in log_ed_key_expiration() can probably just be stack-allocated.


Yep, done in cc2af48569.

  • Documentation on the new option should go into the manpage


Yeah… it would probably be the nice thing to do to tell operators how to use it. :) I will add this once we agree on what the commandline flags and output should be like.

Please fix whatever from above you agree with. :)

comment:25 Changed 3 months ago by isis

Status: needs_revisionneeds_information

comment:26 Changed 3 months ago by nickm

Status: needs_informationneeds_revision

I vote for something like

signing-cert-expiry: 2017-07-25 08:30:15 UTC

for the output format, and document that you need to grep for the signing-cert-expiry.

I guess that's probably enough, plus the manpage. What do you think?

comment:27 in reply to:  26 Changed 3 months ago by isis

Replying to nickm:

I vote for something like

signing-cert-expiry: 2017-07-25 08:30:15 UTC

for the output format, and document that you need to grep for the signing-cert-expiry.

I guess that's probably enough, plus the manpage. What do you think?


Sounds good! This fits in well with the char *description for certs, so we should be able to expand this output format easily in the future for other/new types of certs.

comment:28 Changed 3 months ago by isis

Sponsor: SponsorM-can
Status: needs_revisionneeds_review

Replying to isis:

Replying to nickm:

This looks comparatively solid to me! A few things to consider as possibilities, though maybe they're not needed:

  • Maybe this should printf() something to stdout, instead of using the log facility, and run at --quiet by default?


Yes, this makes sense. TBH, I didn't know if I was allowed/encouraged to do printf(), since it seems like there's a lot of ways "interfaces" over stdin/stdout can be bad/broken/wrong, particularly when they are relied upon by other scripts/programs (cf. gnupg). I think it does make sense though to work even with --quiet, and in this case the user is asking a question like "hey parse this thing and tell me what it says", not intending any operation or for the binary to run as a daemon or anything more complicated, so it makes sense here.

  • Maybe the output format should be machine-readable?


Yeah! But what should this look like?

Literally just spit out the expiry in ISO8601 format? (With or without the underscore in between the date and the time?) Or some more easily machine parseable (but less human readable) format, like seconds-since-epoch?

Should it be in the local timezone, or in UTC? (Probably UTC, right? if we expect scripts to be able to process it?)


The above two are done in commit 6ba245f916e.

  • Maybe it should dump information about the installed authority auth key as well
  • I wonder what it should do about hidden service keys?


Yes, I can add these. I thought about the first one before, but I didn't know how to make the interface, plus had worries about the patch being large/invasive. I think the "proper" way to do it would be to add a suboptions parser so that the user could be like "--key-expiration auth" or "--key-expiration sign"; this way, it would more easily extendable to further changes in supported cert types moving forward.


This is done in commit 7c2329c3eb, or the suboptions parsing is, at least. I didn't add parsers for auth keys or onion service keys yet, because I don't really understand which key is the auth key ("dir-identity-key"?) and the following questions about OS keys:

I'm not sure what to do about onion service keys/certs at all. I imagine there are users with multiple hidden services. Frankly, I didn't even know OS keys/certs could expire. Is that just a v2 thing? Is there some canonical way to refer to an onion service such that I could provide some option like "--key-expiration specifier-for-my-onion-service"? Should I optionally take an onion service's address and learn the keys from the configured onion service directory?

  • Technically speaking, keys don't expire: certificates do. The user needs to replace both of them, not just one.


Right. How should we communicate this to users? I'm not a UX person at all, but I can vaguely naïvely foresee confusion of like "but I was asking about my keys". Should I change to the cmdline flag to --cert-expiration?


For this, I kept the command line flag as "--key-expiration" but all the function/object names and documentation in the code uses "key certificate" or just "certificate" everywhere. (Commit d01793e2ee.)

  • The buffer in log_ed_key_expiration() can probably just be stack-allocated.


Yep, done in cc2af48569.

  • Documentation on the new option should go into the manpage


Yeah… it would probably be the nice thing to do to tell operators how to use it. :) I will add this once we agree on what the commandline flags and output should be like.


Done in commit 9e6aee99.

Please fix whatever from above you agree with. :)


I also added tests and fixed some problems the tests caught in commit 9b82470a27.

I made oniongit PR for review, if we want that! https://oniongit.eu/network/tor/merge_requests/3

comment:29 Changed 3 months ago by nickm

Status: needs_reviewneeds_revision

I tried this out, but when I ran make check, the new test failed. Otherwise it looks okay to me.

comment:30 in reply to:  29 Changed 3 months ago by isis

Replying to nickm:

I tried this out, but when I ran make check, the new test failed. Otherwise it looks okay to me.


Oops, there was one line of code sitting in my tree that didn't get committed. It's added in commit 9b3b63c3d and should fix the test (by printing to stderr rather than stdout).

The rebased/squashed branch is bug17639_r1. Tests pass.

Last edited 3 months ago by isis (previous) (diff)

comment:31 Changed 3 months ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Yeah, that's better! Merged and pushed.

Note: See TracTickets for help on using tickets.