Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new TS3 and TS5 CA pubkeys, introduce timestamp #81

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

tsusanka
Copy link
Contributor

Adding here instead of trezor-suite as of #79.

@komret so is it enough to add here and let's just wait for trezor/trezor-suite#12982? I do not think we need to rush. cc @MiroslavProchazka

@tsusanka tsusanka requested a review from komret June 26, 2024 12:36
@tsusanka
Copy link
Contributor Author

Oh wait, I didn't update the timestamp. It ain't here. That's an oversight I guess and it should be part of the authenticity.json? I know @matejcik mentioned that he will not use that in trezorctl, so he didn't need it, but we should move it here as well to have the single source of truth?

@matejcik
Copy link
Contributor

i didn't realize that there is supposed to be a timestamp. yeah, sure, please add it.

@komret
Copy link
Contributor

komret commented Jun 26, 2024

We should update Suite manually until trezor/trezor-suite#12982 is done.

@tsusanka
Copy link
Contributor Author

tsusanka commented Jul 4, 2024

@komret do we want the timestamp field in the debug keys as well? Meaning authenticity-dev.json. I guess yes for consistency?

@tsusanka
Copy link
Contributor Author

tsusanka commented Jul 4, 2024

Btw now we will introduce timestamp per each model. Currently we have it one timestamp for all if I am not mistaken. I think it is better like this though? @komret

@tsusanka
Copy link
Contributor Author

tsusanka commented Jul 5, 2024

For simplicity I have force-pushed and

  1. Introduced timestamp in a separate commit
  2. Add TS5 keys, so both TS3 and TS5 keys are updated

@tsusanka tsusanka changed the title Add new TS3 CA pubkeys and update timestamp Add new TS3 and TS5 CA pubkeys, introduce timestamp Jul 5, 2024
@komret
Copy link
Contributor

komret commented Jul 9, 2024

The timestamp field is used to check whether the config file holding the rootPubKeys and caPubKeys is up to date. If a device uses a caPubKey whose validity starts later than the timestamp, then we cannot really check the certificate because we have an outdated config. In such cases, the event is reported to Sentry for us to check manually if the caPubKey is legit, while the check is shown as successful to the user. This may happen when the user has a new device and an old version of Suite.

This means that Suite doesn't care about when the keys were added to the data repo nor does it have to differentiate between devices. You're saying that trezorctl doesn't need it either, so unless there is some other reason to have it here, we can remove it from this repo and generate it automatically in Suite when the data is synced. cc @karliatto

@matejcik
Copy link
Contributor

matejcik commented Jul 9, 2024

so unless there is some other reason to have it here, we can remove it from this repo and generate it automatically in Suite when the data is synced

When this file becomes signed, and Suite can download it on-line, you could in theory still run into a situation where your device is newer than the copy on data.trezor.io.
In practice, that would mean that we can package, sell, and ship the device faster than we update the file, which should in fact never happen.

Another use of the timestamp is downgrade attack prevention. Let's say we blacklist a batch and remove the bad key from the list. An attacker could substitute an older version of the (correctly signed) file, which did not blacklist the key yet. By requiring the timestamp to be no more than, say, one month old, this makes the attack more difficult.
(but then we run into problems with discontinued models, where the timestamp gets stuck on an old date...)

i'd say generating the date in Suite is the right thing to do, but keeping it also here seems vaguely useful

Copy link
Contributor

@komret komret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in case Suite ever downloads this data directly from the data repo, the timestamps make sense. However, now when we copy the keys to Suite either manually or automatically, we don't care about the timestamp and can just use one value for all devices corresponding to the day when we sync it.

Approving.

@komret komret merged commit 7386d26 into master Jul 9, 2024
1 check passed
@peter-sanderson peter-sanderson deleted the tsusanka/new-ts3-keys branch July 10, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants