-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Remove duplicate P256Keypair::ECDSA_sign_hash code #20078
Remove duplicate P256Keypair::ECDSA_sign_hash code #20078
Conversation
- The ECDSA_sign_hash method is a near identical copy of of ECDSA_sign_msg, that takes a raw hash. - This is problematic since some platforms, like Android, cannot directly sign a pre-computed hash with OS-aided APIs, and overall this is not consistent with signature APIs that work on messages, and where a digest is an internal implementation detail. - Overall, the method adds little value and prevents easy transition to different signing algorithms over time if the hash assumption is kept Fixes project-chip#18430 This PR: - Removes the sign_hash API - Replaces its usage throughout the SDK - Updates all tests - Leaves the ECDSA_verify_hash_signature (since it's only used in one place, already in native code, and always against raw public keys) Testing done: - Cert tests still pass, including device attestation during commissioning - Unit tests still pass including updated unit tests
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.
There are still Darwin and Android bits that do sign_hash that are dead code (and will likely not compile, because they are override
).
src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
Outdated
Show resolved
Hide resolved
PR #20078: Size comparison from cab048f to e3a762c Increases (15 builds for cc13x2_26x2, efr32, mbed, p6)
Decreases (17 builds for cc13x2_26x2, cyw30739, efr32, k32w, linux, telink)
Full report (26 builds for cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, p6, telink)
|
PR #20078: Size comparison from cab048f to 5f92f85 Increases (15 builds for cc13x2_26x2, efr32, mbed, p6)
Decreases (40 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, telink)
Full report (49 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
PR #20078: Size comparison from cab048f to b9e0cc6 Increases (28 builds for cc13x2_26x2, efr32, linux, mbed, p6)
Decreases (40 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, telink)
Full report (49 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
Problem
that takes a raw hash.
a pre-computed hash with OS-aided APIs, and overall this is not consistent
with signature APIs that work on messages, and where a digest is an internal
implementation detail.
signing algorithms over time if the hash assumption is kept
Fixes #18430
Change overview
already in native code, and always against raw public keys)
Testing