Remove internal use of securesystemslib.hash #977
Conversation
fixes secure-systems-lab#943 * Internally, the features added by the module (i.e. custom blake name support, and file hashing) are not needed. Builtin hashlib is used now directly. * External users port the relevant features into their code base: theupdateframework/python-tuf#2815 in-toto/in-toto#861 Signed-off-by: Lukas Puehringer <[email protected]>
| @@ -180,7 +180,7 @@ def _get_hash_algorithm(public_key: Key) -> str: | |||
| ) | |||
|
|
|||
| # trigger UnsupportedAlgorithm if appropriate | |||
There was a problem hiding this comment.
This is now a ValueError, which breaks the API and is also inconsistent with above, where we raise an UnsupportedAlgorithmError for some unsupported algorithms. Same is true for other signers in this PR
I'll mark the PR as draft until I fix this.
There was a problem hiding this comment.
Turns out, other signers exhaustively check public key schemes prior to using the hash algorithm name. This menas UnsupportedAlgorithmError is not expected when a digest object is created in those signers.
d5016ae adds a similar scheme check to GCPSigner to make use of the already existing raise UnsupportedAlgorithmError, previously only used for invalid key types.
f5f162f proposes an alternative, less invasive but IMO more clunky fix, which just re-raises hashlib's exception as UnsupportedAlgorithmError.
Either way, we may want to consider a more comprehensive solution in #593 and #766.
GCPSigner is expected to raise UnsupportedAlgorithmError, if the hash algorithm encoded in scheme is invalid. Switching from securesystemslib.hash to hashlib, requires wrapping and re-raising to maintain the expected behavior. This is a temporary solution until secure-systems-lab#766. Signed-off-by: Lukas Puehringer <[email protected]>
Minimally refactor EAFP to LBYL to avoid clunky re-raise. Signed-off-by: Lukas Puehringer <[email protected]>
|
sigstore fails because its tuf installation still requires securesystemslib.hash. This should be fixed when sigstore updates to a tuf version, which includes theupdateframework/python-tuf#2815. |
yeah I'd prefer to not remove hash until there is a tuf release that does not use it |
| in ["ecdsa", "ecdsa-sha2-nistp256", "ecdsa-sha2-nistp384"] | ||
| and public_key.scheme.endswith(("256", "384")) | ||
| ): | ||
| algo = public_key.scheme[-3:] |
There was a problem hiding this comment.
I'm a little surprised if this works since it seems to change the algo value ("sha256" -> "256").
The GCP tests currently only run on push so you can trigger it by pushing the branch to main repo
I'm fine with the simpler solution too.
There was a problem hiding this comment.
Good catch. 🤦 That happens when code isn't tested right away.
This reverts commit d5016ae.
lukpueh
changed the title
Internally, it isn't used anymore, but we keep it for existing (transitive) users until they had a chance to update. Signed-off-by: Lukas Puehringer <[email protected]>
Signed-off-by: Lukas Puehringer <[email protected]>
|
Okay, I fixed it. Sorry for the slip up. I re-added the hash module (now unused), and reverted to the easier solution, which might be more clunky but also more clearly doesn't change the behavior. I also pushed the branch to the main repo and GCPSigner still works: Note that the existing tests don't test the failure case asserting for the specific exception. I can add one if desired. |
fixes #943
Internally, the features added by the module (i.e. custom blake name
support, and file hashing) are not needed. Builtin hashlib is used now
directly.
External users port the relevant features into their code base:
Port securesystemslib.hash module theupdateframework/python-tuf#2815
Port securesystemslib.hash file hashing in-toto/in-toto#861