Add typing: asn, exceptions, hashes, pwdbased, utils.#125
Add typing: asn, exceptions, hashes, pwdbased, utils.#125roberthdevries wants to merge 8 commits into
Conversation
65d14db to
1c2b082
Compare
|
This also adds the |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 7 total — 7 posted, 0 skipped
6 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] AES-SIV single-block associated-data length uses char count of original input, not encoded byte length —
wolfcrypt/ciphers.py:397-400 - [Medium] sign_with_seed no longer accepts bytearray/memoryview seeds (regression) —
wolfcrypt/ciphers.py:2513-2546 - [Medium] make_key_from_seed now silently UTF-8-encodes a str seed instead of rejecting it —
wolfcrypt/ciphers.py:2360-2367 - [Low] ChaCha init renamed size to _size, breaking the documented backward-compatible keyword —
wolfcrypt/ciphers.py:544 - [Low] HKDF helpers annotate hash_cls as instance type instead of class type —
wolfcrypt/hkdf.py:33,78,105 - [Low] Random no longer nulls native_object on init failure; del frees an uninitialized RNG —
wolfcrypt/random.py:37-52 - [Low] RsaPublic.init made key a required positional argument —
wolfcrypt/ciphers.py:771-774
Review generated by Skoll
This has some fallout in random.py to simplify checks. Also one test is slightly adapted to produced the desired failure.
8f9280c to
1af1a55
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 7 total — 7 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] New undeclared runtime dependency on typing_extensions —
wolfcrypt/ciphers.py:28, wolfcrypt/hashes.py:28 - [Medium] Removed _ffi.from_buffer() drops bytearray/memoryview support for seed/rand inputs —
wolfcrypt/ciphers.py:2383, 2548, 2559, 2067, 2110 - [Medium] **_Cipher.new() dropped kwargs, breaking PEP 272 extra keyword arguments —
wolfcrypt/ciphers.py:187-199 - [Medium] HKDF functions annotate hash_cls as instance type instead of class type —
wolfcrypt/hkdf.py:33, 78, 105 - [Medium] asn.py leaves function arguments unannotated while enabling ANN ruff rules —
wolfcrypt/asn.py:81, 99 - [Low] test_mldsa now relies on cffi's low-level TypeError instead of an explicit guard —
tests/test_mldsa.py:186 - [Low] RsaPublic.init made key a required positional argument —
wolfcrypt/ciphers.py:781
Review generated by Skoll
Tests are now also type checked as this helps verifying the correctness of the type annotations.
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] ML-DSA seed handling: bytearray/memoryview now rejected, and the two seed methods validate inconsistently —
wolfcrypt/ciphers.py:2371-2392 (make_key_from_seed), 2516-2540 (sign_with_seed) - [Low] *ML-KEM _with_random helpers no longer accept bytearray/memoryview for rand —
wolfcrypt/ciphers.py:2059-2077 (encapsulate_with_random), 2103-2119 (make_key_with_random) - [Low] hkdf.py forces a runtime import of _Hmac for a type annotation (no from future import annotations) —
wolfcrypt/hkdf.py:30-33
Review generated by Skoll
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] ML-DSA seed validation now rejects bytearray/memoryview (regression) —
wolfcrypt/ciphers.py:2383,2537 - [Medium] Advertised list/tuple seed support is untested and likely fails at the cffi boundary —
wolfcrypt/ciphers.py:2372,2391 - [Low] setup.py install_requires not synced with new typing-extensions runtime dependency —
setup.py:62-63 - [Low] Hard runtime import of private cffi symbol _cffi_backend.Lib only to satisfy a cast —
wolfcrypt/__init__.py:20-22,56
Review generated by Skoll
dgarske
left a comment
There was a problem hiding this comment.
See #125 (review)
If you disagree just make note. We are getting close on this and thank you for your efforts
Also added a line to the change log to mention that typing information has been added.
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] typing-extensions dependency has no minimum version (override requires =4.4.0) —
pyproject.toml:27 - [Low] bytes(seed) silently accepts an int, dropping the friendly type check for ML-DSA seeds —
wolfcrypt/ciphers.py:2383,2536 - [Info] New module wolfcrypt/types.py shadows the stdlib types module name —
wolfcrypt/types.py:1 - [Info] Inconsistent # ty:ignore comment lacks the space used everywhere else —
wolfcrypt/__init__.py:53
Review generated by Skoll
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] ChaCha20 encrypt/decrypt round-trip test never executes (decorated as a fixture) —
tests/test_ciphers.py:321-327 - [Info] RsaPublic constructor key parameter is now mandatory (breaking API change) —
wolfcrypt/ciphers.py:782-799
Review generated by Skoll
| @@ -320,7 +319,7 @@ def chacha_obj(vectors): | |||
| return r | |||
|
|
|||
| @pytest.fixture | |||
There was a problem hiding this comment.
🟠 [Medium] ChaCha20 encrypt/decrypt round-trip test never executes (decorated as a fixture)
The diff edits the signature of test_chacha_enc_dec from (chacha_obj) to (chacha_obj, vectors) (adding the vectors fixture that its body references at line 325). However, the function is decorated with @pytest.fixture, so pytest registers it as a fixture named test_chacha_enc_dec and never collects it as a test. As a result the ChaCha20 encrypt->set_iv->decrypt round-trip assertion (assert plaintext == dec) is dead code and provides zero coverage. The signature change indicates awareness that the body needs vectors, but the underlying problem is the @pytest.fixture decorator on a test_-prefixed function. (The @pytest.fixture line is pre-existing, but the PR touches this exact function.)
Fix: Remove the @pytest.fixture decorator so the function is collected and run as an actual test. Since the diff already modified this line to fix the missing vectors parameter, this is the natural moment to also make the test execute.
|
|
||
| class RsaPublic(_Rsa): | ||
| def __init__(self, key=None, hash_type=None, rng=None): | ||
| class RsaPublic(_Rsa, SupportsRsaVerify): |
There was a problem hiding this comment.
⚪ [Info] RsaPublic constructor key parameter is now mandatory (breaking API change)
RsaPublic.__init__ changed from def __init__(self, key=None, ...) (with an internal if key is not None: guard) to def __init__(self, key: BytesOrStr, ...) where key is required and unconditionally passed to t2b(key)/wc_RsaPublicKeyDecode. Any caller that previously did RsaPublic() now raises TypeError. This is an intentional, documented change (ChangeLog: "The RsaPublic key parameter is now mandatory..."), and RsaPrivate.__init__ still accepts key=None and uses _Rsa.__init__ directly, so internal keygen paths (RsaPrivate.make_key) are unaffected. Noting for API-compatibility awareness only.
Fix: No change required; the breaking change is intentional and documented in ChangeLog.rst. Ensure release notes make the new mandatory-key requirement prominent for downstream users.
dgarske
left a comment
There was a problem hiding this comment.
Please also resolve merge conflicts.
No description provided.