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

Chore: more verbose error messages for missing TUF private keys #362

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

vkhoroz
Copy link
Member

@vkhoroz vkhoroz commented Jan 23, 2024

No description provided.

@vkhoroz vkhoroz self-assigned this Jan 23, 2024
Copy link
Member

@doanac doanac left a comment

Choose a reason for hiding this comment

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

thanks. this is helpful

@vkhoroz
Copy link
Member Author

vkhoroz commented Jan 23, 2024

Example errors after this change:

$ fioctl keys tuf updates rotate-offline-key -r targets -k bad-tuf-root.tgz -K bad-tuf-targets.tgz -s
ERROR: Error reading old TUF targets private key:
 Found no active private key for key IDs: [fe02b71e39afcc8cc15c4e6c776dcbedc680aca2d7c6ca1e3f4e58250d4eefc3].
$ fioctl keys tuf updates rotate-offline-key -r targets -k bad-tuf-root.tgz -K good-tuf-targets.tgz -s
= New target keyid: 9eec7097149e0af6883d75975467bdccf0ab35bf1a0d4ff7f1e2b73ffb233fbf
= Removing unused key: 68b52de9c4ab797f4fffa6f559daf5c4f835e19c5049c93d22bc770ac62f97c6
= Re-signing prod targets
= Signing new TUF root
ERROR: Error reading old TUF root private key:
 Found no active private key for key IDs: [abe7f32a17acc760d06bd146e03a9aa8a8c48ba813594e133263e0e19ccbdb7f].

If you can deduct what is going wrong here, then these new errors are good enough.

@mike-sul
Copy link
Contributor

$ fioctl keys tuf updates rotate-offline-key -r targets -k bad-tuf-root.tgz -K bad-tuf-targets.tgz -s
ERROR: Error reading old TUF targets private key:
 Found no active private key for key IDs: [fe02b71e39afcc8cc15c4e6c776dcbedc680aca2d7c6ca1e3f4e58250d4eefc3].

Why the command is trying to read the "old" TUF targets private key?

@vkhoroz
Copy link
Member Author

vkhoroz commented Jan 23, 2024

Why the command is trying to read the "old" TUF targets private key?

IIRC we've discussed this today:

When the user runs rotate-offline-key they need to provide evidence that they own the key,
That evidence comes in a form of a tar.gz file containing a that private key.
Otherwise, a user could accidentally rotate someone else's key.

If the user have lost their key - there is still an ability to add a new key, or remove a lost key, or do both...
But only in a more explicit form, using add-offline-key/delete-offline-key commands.

@mike-sul
Copy link
Contributor

IIRC we've discussed this today

Yes, I know why the command "reading" the current targets role key. However, the message is still misleading for me - hence my question. The key reason is that it doesn't convey what it is trying to achieve, instead it says about mechanics to achieve it.

IMO, The message should say something like "Failed to find the current targets role key to be rotated (or for rotation) in ; expected to find keys with IDs listed in TUF root metadata: [] ".

Maybe I am just looking into it from a wrong perspective. Perhaps checking it with the customer support would be the right way to verify whether the updated error messages are clear.

@vkhoroz
Copy link
Member Author

vkhoroz commented Jan 23, 2024

@mike-sul your proposed message

Failed to find the current targets role key to be rotated in 'file'.
Expected to find keys with IDs listed in TUF root metadata: [id1 id2].

... is almost the same as a message added by this PR:

Error reading old TUF targets private key:
 Found no active private key for key IDs: [id1 id2].

The only 3 essential differences I found are:

  1. Using current instead of old. I'm fine to make this replacement if you think it is more clear.
  2. You message shows a file name. Adding this info would require much more changes, but also look uglier. IMHO a user should know what file they supplied from a command they run.
  3. Your message adds listed in TUF root metadata to describe key IDs. IMHO this is too verbose, but also not fully exact.

I think all the user needs to know is that one of the old or new private keys with IDs id1 ... idN for role root or targets is missing in a file they provided.
The messages in this PR convey that information with minimal code changes.

@mike-sul
Copy link
Contributor

mike-sul commented Jan 23, 2024

.. is almost the same as a message added by this PR:

As for me they are absolutely different :).

  1. Yes, "current" is more clear.
  2. How customer can know that it is looking for a key in some file and why? The key point is to say that the command is trying to find out which of the keys listed in the current root to rotate. At least something like in "the specified key file" would be useful.

I think all the user needs to know is that one of the old or new private keys with IDs id1 ... idN for role root or targets is missing in a file they provided.

That's great, I would put in the error message "... targets is missing in a file they provided".

@vkhoroz
Copy link
Member Author

vkhoroz commented Jan 23, 2024

@mike-sul please, take a look at updated messages:

$ fioctl keys tuf updates rotate-offline-key -r root -k bin/bad-tuf-root.tgz -s
ERROR: Error reading current TUF Root private key from a specified file:
 Found no private key for key IDs: [abe7f32a17acc760d06bd146e03a9aa8a8c48ba813594e133263e0e19ccbdb7f].

Does it work well for you?

@vkhoroz vkhoroz merged commit ff54ca1 into main Jan 24, 2024
8 checks passed
@vkhoroz vkhoroz deleted the vkhoroz-verbose-tuf-errors branch January 24, 2024 13:08
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.

4 participants