-
Notifications
You must be signed in to change notification settings - Fork 360
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
security: handle exception on early anaconda certificate import #6114
base: main
Are you sure you want to change the base?
Conversation
187771c
to
893b962
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify my question below.
pyanaconda/startup_utils.py
Outdated
def prompt_for_reboot(): | ||
print(_("The installation cannot continue and the system will be rebooted")) | ||
print(_("Press ENTER to continue")) | ||
input() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this block installation in non-interactive / cmdline mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I'll check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, to check if we are in interactive mode would be non-trivial but doable at this early stage (asking runtime module for kickstart settings and perhaps also checking cmdline options).
But given sys.exit does not reboot at this stage I think we can just remove the prompt. Then the behaviour would be the same as with kickstart parsing error.
updated the PR, @jkonecny12 what do you think?
893b962
to
c15ebc0
Compare
Present human readable error message. The case for this fix is missing --dir option. The option will be probably made required in pykickstart but we still want to handle import exceptions in user friendly way. Related: INSTALLER-4030 Resolves: rhbz#2342245
c15ebc0
to
0390870
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
/kickstart-test reboot-initial-setup-tui |
/kickstart-test --testtype smoke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! :)
Just a small suggestion to maybe note where the certificate that failed to be processed is coming from. :)
sync_run_task(task_proxy) | ||
except SecurityInstallationError as e: | ||
log.error(e) | ||
print(_("\nAn error occurred during certificate import:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so far ever going only via kickstart, right ? Then I would suggest adding that to the error message, something like:
An error occurred during certificate import from kickstart:
That should make it easier to track down & fix the issue, especially if its a third party seeing the error message and reporting it back (eq. an automated kickstart based deployment going wrong somewhere).
Present human readable error message.
The case for this fix is missing --dir option. The option will be probably made required in pykickstart but we still want to handle import exceptions in user friendly way.
Related: INSTALLER-4030