Changes to signing build system #166
Conversation
ahasztag
requested review from
tomchy,
jnsgsr,
robertstypa,
adsz-nordic,
kszromek-nordic and
nordic-mik7
|
pytest coverage results
Note: This message is automatically posted and updated by the CI (latest/test-sdk-dfu/master/319) |
ahasztag
force-pushed
the
NCSDK-31069_sign_script_build_system
branch
3 times, most recently
from
de2abff to
2afba07
Compare
ahasztag
force-pushed
the
NCSDK-31069_sign_script_build_system
branch
2 times, most recently
from
a69847f to
2fb17e4
Compare
ncs/encrypt_script.py
Outdated
| plaintext = plaintext_file.read() | ||
|
|
||
| def generate_digest_size_for_plain_text(self, plaintext: bytes): | ||
| """Class to generate digests for plaintext using specified hash algorithms.""" |
There was a problem hiding this comment.
nit: consider different docstring, something like:
"""Generate digest and return its size for the given plaintext."""
| digest, plaintext_len = digest_generator.generate_digest_size_for_plain_text(firmware) | ||
| encrypted_asset, encrypted_cek = self.generate_kms_artifacts(firmware, key_name, context) | ||
| encrypted_payload, tag, encryption_info = self.generate_encryption_info_and_encrypted_payload( | ||
| encrypted_asset, encrypted_cek, key_id |
There was a problem hiding this comment.
nit: the type of encrypted_asset in the generate method is bytes, but in the generate_encryption_info_and_encrypted_payload method, it is expected to be of type Path. This mismatch can lead to runtime errors. Please ensure that the type of encrypted_asset is consistent across these methods.
There was a problem hiding this comment.
Fixed, it should by bytes everywhere
There was a problem hiding this comment.
nit: there is an inconsistency between how type hinting is used for the input and output parameters among files and even in the same file
Please refer to the following methods:
There is type hinting for input and output parameters:
def generate(
self, encrypted_asset: bytes, encrypted_cek: bytes, key_id: int, kw_alg: SuitKWAlgorithms
) -> tuple[bytes, bytes, bytes]:
There is type hinting for input parameters, and no type hinting for the output.
def generate_encryption_info_and_encrypted_payload(self, encrypted_asset: Path, encrypted_cek, key_id: int):
There is no type hinting at all.
def generate_suit_encryption_info(self, iv, encrypted_cek, key_id):
This is nothing critical and can be solved/improved later.
There was a problem hiding this comment.
Ok, I've attempted to fix it to at least some extent
ncs/sign_script.py
Outdated
| class SignerError(Exception): | ||
| """Signer exception.""" | ||
|
|
||
|
|
||
| class Signer: | ||
| def _import_module_from_path(module_name, file_path): | ||
| # Helper function to import a python module from a file path. |
There was a problem hiding this comment.
nit: This comment could be added as a docstring.
suit_generator/cmd_encrypt.py
Outdated
|
|
||
|
|
||
| def _import_module_from_path(module_name, file_path): | ||
| # Helper function to import a python module from a file path. |
There was a problem hiding this comment.
nit: This comment could be added as a docstring.
| return module | ||
|
|
||
|
|
||
| def _import_encryptor(encrypt_script: Path) -> SuitEncryptorBase: |
There was a problem hiding this comment.
nit: _import_module_from_path contains a comment (which could be a docstring), but there is neither a comment nor a docstring. It would be nice to have something to be consistent
| :param algorithm: The name of the algorithm to be used. | ||
| For file based KMS, this can be used to verify if the key in the | ||
| provided file contains a key of the correct type. | ||
| :param context: The context to be used |
There was a problem hiding this comment.
nit: add a period at the end of the :param context: description for consistency.
ahasztag
force-pushed
the
NCSDK-31069_sign_script_build_system
branch
from
2fb17e4 to
d63bde6
Compare
Ref: NCSDK-31069 Signed-off-by: Artur Hadasz <[email protected]>
Ref: NCSDK-30935 Signed-off-by: Artur Hadasz <[email protected]>
ahasztag
force-pushed
the
NCSDK-31069_sign_script_build_system
branch
from
d63bde6 to
0590490
Compare
No description provided.