Suppport to verify aboot swi image for secure boot#969
Suppport to verify aboot swi image for secure boot#969xumia merged 5 commits intosonic-net:masterfrom
Conversation
|
|
||
| import click | ||
|
|
||
| from M2Crypto import X509 |
There was a problem hiding this comment.
M2Crypto [](start = 5, length = 8)
Add this new dependency in setup.py?
#Closed
There was a problem hiding this comment.
It will be add in build_debian.sh in sonic-buildimage.
There was a problem hiding this comment.
Your statement is true. But by best practice, you need to add new dependency in setup.py
In reply to: 446767082 [](ancestors = 446767082)
sonic_installer/bootloader/aboot.py
Outdated
| try: | ||
| subprocess.check_call(['/usr/bin/unzip', '-tq', image_path]) | ||
| # TODO: secureboot check signature | ||
| if not self._verify_secureboot_image(image_path): |
There was a problem hiding this comment.
self._verify_secureboot_image(image_path) [](start = 19, length = 41)
Just return, no need to use if-block #Closed
sonic_installer/bootloader/aboot.py
Outdated
| else: | ||
| print( 'Unexpected format for line in swi[x]-signature file: %s' % line ) | ||
| return None | ||
| except KeyError: |
There was a problem hiding this comment.
except KeyError: [](start = 8, length = 16)
if only one line of code could raise exception, could you minimize your try-block? #Closed
sonic_installer/bootloader/aboot.py
Outdated
| signingCert = X509.load_cert_string(cert) | ||
| return signingCert | ||
| else: | ||
| print( 'Unexpected format for line in swi[x]-signature file: %s' % line ) |
There was a problem hiding this comment.
print [](start = 28, length = 5)
print to stderr #Closed
sonic_installer/bootloader/aboot.py
Outdated
| def base64Decode(cls, text): | ||
| try: | ||
| return base64.standard_b64decode(text) | ||
| except TypeError: |
There was a problem hiding this comment.
TypeError [](start = 15, length = 9)
Is it better to catch in getCert() ? #Closed
|
This pull request introduces 1 alert when merging 7b12c00 into 16a33f2 - view on LGTM.com new alerts:
|
| return True | ||
|
|
||
| def _verify_secureboot_image(self, image_path): | ||
| if isSecureboot(): |
There was a problem hiding this comment.
if isSecureboot(): [](start = 8, length = 18)
If not, should you return True? #Closed
sonic_installer/bootloader/aboot.py
Outdated
| if not cert or not current_cert: | ||
| return False | ||
| # Verify the signing certificates are from the same issuer | ||
| return str(cert.get_issuer()) == str(current_cert.get_issuer()) |
There was a problem hiding this comment.
I guess there is use case of change issuer in long run. Could you comment out this part of code? #Closed
There was a problem hiding this comment.
@qiluo-msft , we only need to verify the image has been signed, not need to verify the issuer, right?
|
Fix the build environment in Azure/sonic-build-tools#128 #Closed |
|
Retest this please #Closed |
* Suppport to verify aboot swi image for secure boot * Simplify the code * Fix not return value bug * Add m2crypto to setup.py * Change to only verify the image signed by a correct certificate
2 participants
- What I did
The feature is to verify the aboot swi image, it makes sure the installing image should be signed when secure boot enabled.
- How I did it
- How to verify it
- Previous command output (if the output of a command-line utility has changed)
- New command output (if the output of a command-line utility has changed)