[Canonical] Updating UEFI and Db certificates during default patching#346
[Canonical] Updating UEFI and Db certificates during default patching#346rane-rajasi wants to merge 6 commits into
Conversation
rane-rajasi
requested review from
kjohn-msft,
michellemcdaniel,
najams,
nikhim-um,
sadiksubhani9-sudo,
vbonu16 and
yashnap
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #346 +/- ##
==========================================
- Coverage 93.84% 93.19% -0.65%
==========================================
Files 105 105
Lines 18218 18376 +158
==========================================
+ Hits 17096 17126 +30
- Misses 1122 1250 +128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “update certificates” flow intended to run during patch installation, adds related constants/error codes, and adjusts tests around package manager detection.
Changes:
- Adds certificate-update plumbing to
PackageManager(mokutil detection, cert status retrieval helpers, andupdate_certs()orchestration). - Invokes
package_manager.update_certs()at the start of patch installation. - Adds
CERTIFICATE_UPDATEerror code + certificate-related constants, and comments out an EnvLayer unsupported-distro test.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/tests/Test_EnvLayer.py | Comments out the Azure Linux 4 / RHEL 10 unsupported-distro test. |
| src/core/src/package_managers/ZypperPackageManager.py | Adds abstract stub cert-update methods that override base behavior. |
| src/core/src/package_managers/YumPackageManager.py | Adds abstract stub cert-update methods that override base behavior. |
| src/core/src/package_managers/TdnfPackageManager.py | Adds abstract stub cert-update methods that override base behavior. |
| src/core/src/package_managers/PackageManager.py | Adds shared cert-update orchestration and mokutil/cert status command constants. |
| src/core/src/package_managers/AptitudePackageManager.py | Adds apt/fwupd-based cert update flow, but also overrides key base methods with abstract stubs. |
| src/core/src/core_logic/PatchInstaller.py | Calls package_manager.update_certs() during installation start. |
| src/core/src/bootstrap/Constants.py | Adds Certificates enum + LATEST_CERTIFICATE_TIMESTAMP + CERTIFICATE_UPDATE error code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pass | ||
|
|
||
| # region Update certificates in factory defaults | ||
| def update_certs(self): |
There was a problem hiding this comment.
Lets add VM security type condition too. This update should not be done in CVMs in current form.
There was a problem hiding this comment.
@deepaksh-microsoft, can you elaborate on this?
There was a problem hiding this comment.
I want to make sure I understand this correctly; do you mean certificates should not be applied/updated for CVMs? If yes, do you know how to identify from within a VM if it is a CVM?
| self.proposed_repo_file_path = "/etc/apt/sources.list.d/proposed.list" | ||
| self.proposed_pin_file_path = "/etc/apt/preferences.d/proposed" | ||
| self.add_proposed_repo_cmd = ( | ||
| "bash -c 'echo \"deb http://archive.ubuntu.com/ubuntu/ " |
There was a problem hiding this comment.
I agree with the copilot review here. Can we use https
There was a problem hiding this comment.
These commands/instructions, to update certs, are shared by the partners. (All partner comm docs in PR review post). They own the tooling/guideline to update certs, we are adding this mechanism as a safety step for our default patching customers
There was a problem hiding this comment.
Please check with the doc authors. The site supports https and they may have overlooked that when writing the example.
| self.composite_logger.log("[APM][Certs] Cert update completed and verified.") | ||
| success = True | ||
| else: | ||
| msg = "[APM][Certs] Cert update commands completed but latest certs not detected." |
There was a problem hiding this comment.
What does this condition actually mean? Like there was no error in the cert install, but the latest certs weren't available? Could we have updated to something other than the latest certs, and in that case, would we need a reboot below? Just trying to understand what could go wrong that would lead us to not end up in the exception case, but still not install the certs
There was a problem hiding this comment.
They tooling used to update here is fwupd (firmware update manager). In this method of updating certs, the exact version of current certs is never pinned or set/referred while installing. fwupd fetches and installs latest available certs. This is beneficial because our code doesn't become dated in the future (if it was pinned to an old version) but adds the need to verify if the certs we intend to update were applied. Hence the validation on latest certs.
| step_name, str(command), str(code), str(out)) | ||
| self.composite_logger.log_error(msg) | ||
| self.status_handler.add_error_to_status(msg, Constants.PatchOperationErrorCodes.CERTIFICATE_UPDATE) | ||
| if raise_on_error: |
There was a problem hiding this comment.
When would we not want to raise_on_error?
There was a problem hiding this comment.
The cleanup or unpinning code in try_update_certs(), in finally block, doesn't raise errors
|
|
||
| def __run_cert_apt_command(self, command, step_name, raise_on_error=False): | ||
| """Run apt/dpkg commands through package-manager wrapper.""" | ||
| out, code = self.invoke_package_manager_advanced(command, raise_on_exception=False) |
There was a problem hiding this comment.
Same question as below, could this just lead us to failing silently?
There was a problem hiding this comment.
No. By not raising an exception from invoke_package_manager_advanced(), __run_cert_apt_command() retains the control on whether to raise an execption
invoke_package_manager_advanced() will run the cmd and return its output code and text.
__run_cert_apt_command() will analyze the output code, if error, log it in status blob and raise, if needed.
raise_on_error gives the ability to handle errors for eg, with appropriate logging, reporting, etc. There are some paths in code where an exception may not be needed which can be managed with raise_on_error=False
| if is_mokutil_installed or try_install_mokutil_status: | ||
| self.try_update_certs() | ||
| else: | ||
| error_msg = "[PM][UpdateCerts] Mokutil is not installed or could not be installed. Cannot fetch current certs." |
There was a problem hiding this comment.
nit: For clarity of logging, I think this should be and. If we're in this case, both mokutil is not installed AND it could not be installed.
|
|
||
| # restore | ||
| self.__restore_mocks() | ||
| # def test_get_package_manager_azure_linux_4_and_rhel10_not_supported(self): |
There was a problem hiding this comment.
Why are we commenting out tests and not just deleting? If we need them in the future, we can just grab them from past commits.
There was a problem hiding this comment.
It's not related to this PR. I've commented it out here because it was failing and causing other tests to fail or not run. It's being fixed in a separate PR: #347. Depending on which one of these 2 PRs go in first, the second one will merge code with first and fix and uncomment this
| pass | ||
|
|
||
| # region Update certificates in factory defaults | ||
| def update_certs(self): |
There was a problem hiding this comment.
Are there any tests for the new cert code?
There was a problem hiding this comment.
Will be adding new UTs in next iteration
rane-rajasi
changed the title
rane-rajasi
changed the title
| self.proposed_repo_file_path = "/etc/apt/sources.list.d/proposed.list" | ||
| self.proposed_pin_file_path = "/etc/apt/preferences.d/proposed" | ||
| self.add_proposed_repo_cmd = ( | ||
| "bash -c 'echo \"deb http://archive.ubuntu.com/ubuntu/ " |
There was a problem hiding this comment.
Please check with the doc authors. The site supports https and they may have overlooked that when writing the example.
|
|
||
| # Update certificates in factory defaults | ||
| self.check_mokutil_exists_cmd = "command -v mokutil" | ||
| self.get_kek_cert_status_cmd = "mokutil --kek | grep 'CN='" # todo: review if this should be changed to mokutil --kek 2>&1 | grep -oP 'CN=\K[^,]+' | sort -u | tr '\n' '; ' || echo "none" |
There was a problem hiding this comment.
What is the status of this TODO?
| .format(str(code), cert_type, cmd)) | ||
| self.status_handler.add_error_to_status(error_msg, Constants.PatchOperationErrorCodes.CERTIFICATE_UPDATE) | ||
| if raise_on_exception: | ||
| raise Exception(error_msg, "[{0}]".format(Constants.ERROR_ADDED_TO_STATUS)) |
There was a problem hiding this comment.
Will this exception be translated into VMGuestPatchClientError at some point in the stack? If so, where?
| # region Update certificates in factory defaults | ||
| def try_install_mokutil(self): | ||
| """ Attempts to install mokutil """ | ||
| pass |
There was a problem hiding this comment.
Is this coming in another iteration of the PR? The doc describes support for tdnf. Does LPE support it?
| # region Update certificates in factory defaults | ||
| def try_install_mokutil(self): | ||
| """ Attempts to install mokutil """ | ||
| pass |
There was a problem hiding this comment.
Same here. Is this coming later?
| pass | ||
|
|
||
| # region Update certificates in factory defaults | ||
| def update_certs(self): |
|
|
||
| class UEFISettings(EnumBackport): | ||
| ENABLE_UEFI_CERT_UPDATE = 'EnableUEFICertUpdate' | ||
| ENABLED_By = 'EnabledBy' |
There was a problem hiding this comment.
nit: y should be capital
Description:
This PR aims to provide a best effort attempt to update UEFI and Db certificates for our Canonical default patching customers. This is an ask from a partner to help address Microsoft certificate rotations: https://discourse.ubuntu.com/t/microsoft-uefi-ca-rotation-what-it-means-for-ubuntu-users-and-vendors/82652
Changes:
Pending: