-
Notifications
You must be signed in to change notification settings - Fork 50
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
Abstract methods added to Key for signing scheme dissection #837
base: main
Are you sure you want to change the base?
Changes from 1 commit
c6d2729
8c0ff83
cbe34d8
84fc3ea
bc624ac
0aecbc6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,11 +4,13 @@ | |
from abc import ABCMeta, abstractmethod | ||
from typing import Any, Dict, Optional, Tuple, Type, cast | ||
|
||
import securesystemslib.hash as sslib_hash | ||
from securesystemslib._vendor.ed25519.ed25519 import ( | ||
SignatureMismatch, | ||
checkvalid, | ||
) | ||
from securesystemslib.exceptions import ( | ||
UnsupportedAlgorithmError, | ||
UnsupportedLibraryError, | ||
UnverifiedSignatureError, | ||
VerificationError, | ||
|
@@ -56,6 +58,11 @@ | |
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class UnsupportedKeyType(Exception): # noqa: N818 | ||
pass | ||
Comment on lines
+62
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could be marked private (_UnsupportedKeyType) since I don't think we intend to leak it outside... but that's a nit |
||
|
||
|
||
# NOTE Key dispatch table is defined here so it's usable by Key, | ||
# but is populated in __init__.py (and can be appended by users). | ||
KEY_FOR_TYPE_AND_SCHEME: Dict[Tuple[str, str], Type] = {} | ||
|
@@ -196,6 +203,32 @@ def verify_signature(self, signature: Signature, data: bytes) -> None: | |
""" | ||
raise NotImplementedError | ||
|
||
@abstractmethod | ||
def get_hash_algorithm_str(self) -> Any: | ||
"""Returns payload hash algorithm used for this key as a str | ||
|
||
Raises: | ||
UnsupportedAlgorithmError: if key type not suported | ||
""" | ||
raise NotImplementedError | ||
|
||
@abstractmethod | ||
def get_hash_algorithm(self) -> Any: | ||
"""Returns payload hash algorithm used for this key as a HashAlgorithm""" | ||
raise NotImplementedError | ||
|
||
@abstractmethod | ||
def get_padding_name_str(self) -> Any: | ||
"""Return payload padding name used for this key as a str""" | ||
|
||
raise NotImplementedError | ||
|
||
@abstractmethod | ||
def get_padding_name(self, hash_algorithm: Any) -> Any: | ||
"""Return payload padding name used for this key as a AsymmetricPadding""" | ||
|
||
raise NotImplementedError | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't really see a benefit of adding these to the Key interface. The only subclass, which implements them, is SSlibKey. Why don't make these SSlibKey-only methods? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In _azure_signer.py and _gcp_signer.py both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm. Good question. It looks like all But adding new behaviour to an abstract base class, which is specific to one subclass only, and raising NotImplementedError everywhere else, does not feel right. Would it be okay to add regular functions, e.g. to _utils? Thoughts, @jku? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I admit I didn't look at the specific uses in AzureSigner and GCPSigner yet but I would say it's fine for them to handle SSlibKey only -- that sounds correct to me, what else could the key be? Whether that should be done in practice by type checking inside the AzureSigner and GCPSigner functions or by changing some public argument types I can't say without having a closer look... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks great! If the change isn't likely to bother any user, then I'd go with it. I'll give a detailed review after #836 is merged. |
||
|
||
class SSlibKey(Key): | ||
"""Key implementation for RSA, Ed25519, ECDSA keys""" | ||
|
@@ -301,35 +334,6 @@ def from_crypto( | |
|
||
return SSlibKey(keyid, keytype, scheme, keyval) | ||
|
||
@staticmethod | ||
def _get_hash_algorithm(name: str) -> "HashAlgorithm": | ||
"""Helper to return hash algorithm for name.""" | ||
algorithm: HashAlgorithm | ||
if name == "sha224": | ||
algorithm = SHA224() | ||
if name == "sha256": | ||
algorithm = SHA256() | ||
if name == "sha384": | ||
algorithm = SHA384() | ||
if name == "sha512": | ||
algorithm = SHA512() | ||
|
||
return algorithm | ||
|
||
@staticmethod | ||
def _get_rsa_padding( | ||
name: str, hash_algorithm: "HashAlgorithm" | ||
) -> "AsymmetricPadding": | ||
"""Helper to return rsa signature padding for name.""" | ||
padding: AsymmetricPadding | ||
if name == "pss": | ||
padding = PSS(mgf=MGF1(hash_algorithm), salt_length=PSS.AUTO) | ||
|
||
if name == "pkcs1v15": | ||
padding = PKCS1v15() | ||
|
||
return padding | ||
|
||
def _verify_ed25519_fallback(self, signature: bytes, data: bytes) -> None: | ||
"""Helper to verify ed25519 sig if pyca/cryptography is unavailable.""" | ||
try: | ||
|
@@ -364,9 +368,8 @@ def _validate_curve(key, curve): | |
]: | ||
key = cast(RSAPublicKey, self._crypto_key()) | ||
_validate_type(key, RSAPublicKey) | ||
padding_name, hash_name = self.scheme.split("-")[1:] | ||
hash_algorithm = self._get_hash_algorithm(hash_name) | ||
padding = self._get_rsa_padding(padding_name, hash_algorithm) | ||
hash_algorithm = self.get_hash_algorithm() | ||
padding = self.get_padding_name(hash_algorithm) | ||
key.verify(signature, data, padding, hash_algorithm) | ||
|
||
elif ( | ||
|
@@ -428,3 +431,62 @@ def verify_signature(self, signature: Signature, data: bytes) -> None: | |
raise VerificationError( | ||
f"Unknown failure to verify signature by {self.keyid}" | ||
) from e | ||
|
||
def get_hash_algorithm_str(self) -> str: | ||
# key scheme should always be of format xxx-xxx-xxx | ||
comps = self.scheme.split("-") | ||
if len(comps) != 3: # noqa: PLR2004 | ||
raise UnsupportedKeyType("Invalid scheme found") | ||
|
||
if self.keytype == "rsa": | ||
# hash algorithm is encoded as last scheme portion | ||
hash_algo = self.scheme.split("-")[-1] | ||
elif self.keytype in [ | ||
"ecdsa", | ||
"ecdsa-sha2-nistp256", | ||
"ecdsa-sha2-nistp384", | ||
]: | ||
# nistp256 uses sha-256, nistp384 uses sha-384 | ||
bits = self.scheme.split("-nistp")[-1] | ||
hash_algo = f"sha{bits}" | ||
else: | ||
raise UnsupportedAlgorithmError( | ||
f"Unsupported key type {self.keytype} in key {self.keyid}" | ||
) | ||
|
||
# trigger UnsupportedAlgorithm if appropriate | ||
_ = sslib_hash.digest(hash_algo) | ||
|
||
return hash_algo | ||
|
||
def get_hash_algorithm(self) -> "HashAlgorithm": | ||
name = self.get_hash_algorithm_str() | ||
algorithm: HashAlgorithm | ||
if name == "sha224": | ||
algorithm = SHA224() | ||
if name == "sha256": | ||
algorithm = SHA256() | ||
if name == "sha384": | ||
algorithm = SHA384() | ||
if name == "sha512": | ||
algorithm = SHA512() | ||
|
||
return algorithm | ||
|
||
def get_padding_name_str(self) -> str: | ||
padding_name = self.scheme.split("-")[1] | ||
|
||
return padding_name | ||
|
||
def get_padding_name( | ||
self, hash_algorithm: "HashAlgorithm" | ||
) -> "AsymmetricPadding": | ||
name = self.get_padding_name_str() | ||
padding: AsymmetricPadding | ||
if name == "pss": | ||
padding = PSS(mgf=MGF1(hash_algorithm), salt_length=PSS.AUTO) | ||
|
||
if name == "pkcs1v15": | ||
padding = PKCS1v15() | ||
|
||
return padding |
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.
These stubs are no longer needed, right?