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

Make it possible to abort signing if keys cannot be loaded, improve cli error handling #346

Merged
merged 7 commits into from
Aug 29, 2023

Conversation

renatav
Copy link
Collaborator

@renatav renatav commented Aug 25, 2023

Description (e.g. "Related to ...", etc.)

When loading signing keys, we check if there is a keystore file, then if we can sign using a yubikey before eventually asking the user to manually enter the key (an option that a user might want to use if they want to store the private key in a different way, neither on their filesystem nor on a YubiKey). The problem with this is that in majority of the cases, a user actually wanted to use a keystore file, but provided an incorrect location and they do not want to copy/paste this key. At that point, we had to manually terminate the application. This PR adds a prompt which gives the user an option to say that they do not want to enter this key, in which case an error is raised. This error is caught and the information that the role's metadata could not be signed is printed.

Also added CLI error handling in order to only print error messages and not full stack traces. Only TAFError exceptions are caught. In other cases, stack trace will still be printed (meaning that an unexpected error occurred). I think that showing just error messages in case of unexpected errors makes it really difficult to debug.

Add prompt_for_keys flag to all functions that load signing keys. If set to True, we'll ask the user if they want to manually enter the key if not using yubikeys and it cannot be loaded from keystore files. If set to False, the execution of the program will stop if not using Yubikeys and the key is not found inside the provided keystore directory.

Closes #242
Closes #334

(This was already fixed to some extended in other PRs, but I think that we can fully close it now)

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)

…anually insert key

- Print only error message instead of the full stack trace if a TAFError is raised
Any unexpected errors will still result in whole stack trace being shown, which
I believe is good for now (we don't want to make it difficult to debug)
- If keystore file is not found (or not correct) and the user does not want to use
a yubikey, ask them if they want to manually enter the key. At the moment,
we often have to manually terminate the command at that point
@renatav renatav requested a review from n-dusan August 25, 2023 22:12
@renatav renatav self-assigned this Aug 25, 2023
@dgreisen
Copy link
Contributor

I don't really see a situation in production where someone would manually type the key in. Instead of a prompt to not enter key, could we add a flag to prompt for keys? Something like --prompt-for-keys? Then if that flag isn't present and key isn't in keystore it immediately fails?

Added prompt_for_keys flag to all functions that load signing keys.
If set to True, we'll ask the user if they want to manually enter
the key if not using yubikeys and it cannot be loaded from keystore files.
If set to False, the execution of the program will stop if not using Yubikeys
and the key is not found inside the provided keystore directory
- Expand paths that start with ~
- Fixed loading delegations from keys-description json files
@renatav renatav force-pushed the renatav/signing-error-messages branch from 6d076ec to d775265 Compare August 28, 2023 23:11
@renatav renatav merged commit 96879e2 into master Aug 29, 2023
25 checks passed
@renatav renatav deleted the renatav/signing-error-messages branch August 29, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants