Opened 7 months ago

Closed 5 months ago

#30549 closed task (fixed)

Add script to remove expired sub-keys from a keyring file

Reported by: boklm Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201907R, tbb-rbm
Cc: Actual Points:
Parent ID: #30548 Points:
Reviewer: Sponsor:

Description

gpg has some commands that we can use to drop expired sub-keys from a keyring. However the command to do that is difficult to remember, so we could add a script for doing that in the ./tools directory.

Child Tickets

Change History (16)

comment:1 Changed 7 months ago by boklm

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201905 removed
Status: newneeds_review

comment:2 Changed 7 months ago by gk

Status: needs_reviewneeds_information

The commit message says things like "Add script to remove expired sub-keys from a keyring file" but then we have

+# Drop expired and revoked sub-keys from a keyring file

Looking at the code it seems we indeed want to take care of both expired and explicitly revoked keys. That's right?

If we apply that script how can we prevent removing expired subkeys we actually *still need* for building by accident?

comment:3 in reply to:  2 Changed 7 months ago by boklm

Status: needs_informationneeds_review

Replying to gk:

The commit message says things like "Add script to remove expired sub-keys from a keyring file" but then we have

+# Drop expired and revoked sub-keys from a keyring file

Looking at the code it seems we indeed want to take care of both expired and explicitly revoked keys. That's right?

Yes. I updated the commit message in in branch bug_30549_v2:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30549_v2&id=0b258f07310f8180810558930f79f13d2d4d7906

If we apply that script how can we prevent removing expired subkeys we actually *still need* for building by accident?

We should only use this script when we want to remove all expired sub-keys. I added a comment in the script mentioning that.

For the cases where we need to keep some of the expired-keys, but not all, I am not sure yet what is the best way to do that, as gpg does not seem to make it easy to keep only some of the expired sub-keys. Maybe using the script with faketime would work, but I didn't try.

comment:4 Changed 7 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201905R removed
Status: needs_reviewneeds_revision

The list-all-keyrings scripts looks good to me. However, it does sometimes weird things in that it only lists the binutils key and then stops + it modifies it as well and I am left with a binutils.gpg~ file. I am still hunting for steps to repro that reliably... That's with GnuPG 2.2.13 ona Debian testing/unstable box in case it matters.

Regarding the drop-expired-sub-keys script:

1) The script does not differentiate between subkeys that are expired in our tor-browser-build repo but are not expired in reality: there are folks that just extend the expiration date from time to time instead of/in addition to renewing keys.

2) The script should not touch keys that have no expired subkeys. When I currently do something like tools/keyring/drop-expired-sub-keys keyring/zlib.gpg then I get a modified zlib.gpg afterwards which I should not get.

3) I should not get any keyring/$.gpg~ files in my keyring dir after running the script

comment:5 in reply to:  4 Changed 6 months ago by boklm

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201905 removed
Status: needs_revisionneeds_review

Replying to gk:

The list-all-keyrings scripts looks good to me. However, it does sometimes weird things in that it only lists the binutils key and then stops + it modifies it as well and I am left with a binutils.gpg~ file. I am still hunting for steps to repro that reliably... That's with GnuPG 2.2.13 ona Debian testing/unstable box in case it matters.

The issue with the *.gpg~ files seems similar to #25435, which is fixed by adding the flag --no-auto-check-trustdb. I added it to list-all-keyrings and drop-expired-sub-keys in branch bug_30549_v3:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30549_v3&id=0151dd050de272f32a690d39f5ba501220844df5

I am not sure what is the issue when it only lists binutils and stops.

Regarding the drop-expired-sub-keys script:

1) The script does not differentiate between subkeys that are expired in our tor-browser-build repo but are not expired in reality: there are folks that just extend the expiration date from time to time instead of/in addition to renewing keys.

2) The script should not touch keys that have no expired subkeys. When I currently do something like tools/keyring/drop-expired-sub-keys keyring/zlib.gpg then I get a modified zlib.gpg afterwards which I should not get.

I think we should only run drop-expired-sub-keys in the cases where we know it is actually needed.

The process would be something like this:

  • Run list-all-keyrings to see if we include any expired key/sub-key.

Then for each expired key/sub-key:

  • Check if the expiration is expected, and do nothing in that case.
  • Check if the owner of that key/sub-key extended it, and in that case add the updated key/sub-key.
  • If the sub-key is not needed anymore, use drop-expired-sub-keys to remove it.

comment:6 Changed 6 months ago by cypherpunks

Keywords: TorBrowserTeam201905R added?
It's June now ;) LGTM.

comment:7 in reply to:  6 Changed 6 months ago by boklm

Keywords: TorBrowserTeam201906R added; TorBrowserTeam201905R removed

Replying to cypherpunks:

Keywords: TorBrowserTeam201905R added?
It's June now ;) LGTM.

Ah indeed, thanks.

comment:8 Changed 6 months ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201906R removed
Status: needs_reviewneeds_revision

Could you update drop-expired-sub-keys with the process you envision in commit:5? That way someone later without all the background we have knows what to do with the script(s).

comment:9 in reply to:  8 ; Changed 6 months ago by boklm

Keywords: TorBrowserTeam201906R added; TorBrowserTeam201906 removed
Status: needs_revisionneeds_review

Replying to gk:

Could you update drop-expired-sub-keys with the process you envision in commit:5? That way someone later without all the background we have knows what to do with the script(s).

In branch bug_30549_v4 I added a README file explaining the process:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30549_v4&id=059bb1f569084ac1c6e9d17cd3959c33afeb37d7

comment:10 in reply to:  9 ; Changed 6 months ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201906R removed
Status: needs_reviewneeds_revision

Replying to boklm:

Replying to gk:

Could you update drop-expired-sub-keys with the process you envision in commit:5? That way someone later without all the background we have knows what to do with the script(s).

In branch bug_30549_v4 I added a README file explaining the process:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30549_v4&id=059bb1f569084ac1c6e9d17cd3959c33afeb37d7

Thanks, this looks better: One final thing (I hope :) ):

- if a key is not needed anymore, remove it with `gpg --delete-keys`.

s/i/I/, but more generally I am not sure I understand the need for the gpg invocation here. If we find a key in /keyring which we don't need anymore then we just remove the file, no? Or did you have in mind cases where a bunch of keys were in the same file? I suspect we could need some clarification here.

comment:11 in reply to:  10 Changed 6 months ago by boklm

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201907 removed
Status: needs_revisionneeds_review

Replying to gk:

Replying to boklm:

Replying to gk:

Could you update drop-expired-sub-keys with the process you envision in commit:5? That way someone later without all the background we have knows what to do with the script(s).

In branch bug_30549_v4 I added a README file explaining the process:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30549_v4&id=059bb1f569084ac1c6e9d17cd3959c33afeb37d7

Thanks, this looks better: One final thing (I hope :) ):

- if a key is not needed anymore, remove it with `gpg --delete-keys`.

s/i/I/, but more generally I am not sure I understand the need for the gpg invocation here. If we find a key in /keyring which we don't need anymore then we just remove the file, no? Or did you have in mind cases where a bunch of keys were in the same file? I suspect we could need some clarification here.

Yes, I was thinking of the case of a keyring file containing multiple keys, including an expired one that is not needed anymore. I clarified this in bug_30549_v5:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30549_v5&id=419b0bef89047450a88292ea34bb8ef1e746bbea

comment:12 Changed 6 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Okay, let's see how this works out if we actually want/need to use it. I've merged the patch to master (419b0bef89047450a88292ea34bb8ef1e746bbea).

comment:13 Changed 5 months ago by boklm

Resolution: fixed
Status: closedreopened

When used with gpg >= 2.1, the tools/keyring/drop-expired-sub-keys script will convert the keyring format to the keybox format, which is only compatible with gpg >= 2.1.

I think we should update the script to keep the keyring in the old format, to keep compatibility with older versions of gpg.

comment:14 Changed 5 months ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201907R removed

comment:15 in reply to:  13 Changed 5 months ago by boklm

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201907 removed
Status: reopenedneeds_review

Replying to boklm:

I think we should update the script to keep the keyring in the old format, to keep compatibility with older versions of gpg.

There is a patch fixing this in branch bug_30549_v6:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30549_v6&id=cd6555af118fa06a30d54a491618b50c5d463c5d

comment:16 Changed 5 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good. Merged to master with commit 1ece76a456213a16f23b41a6ce133a44a0302ee0.

Note: See TracTickets for help on using tickets.