Add typing: asn, exceptions, hashes, pwdbased, utils.#125
Add typing: asn, exceptions, hashes, pwdbased, utils.#125roberthdevries wants to merge 12 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
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
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
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.
Please also resolve merge conflicts.
922501b to
425813b
Compare
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
- [Low] *MlKem _with_random drops buffer-protocol support for rand —
wolfcrypt/ciphers.py:2096-2098,2140 - [Low] setup.py dependencies out of sync with pyproject (typing-extensions) —
setup.py:62-63
Review generated by Skoll
dgarske
left a comment
There was a problem hiding this comment.
Please resolve merge conflicts. Also see #125 (review)
This has some fallout in random.py to simplify checks. Also one test is slightly adapted to produced the desired failure.
Tests are now also type checked as this helps verifying the correctness of the type annotations.
Also added a line to the change log to mention that typing information has been added.
e053b55 to
3977253
Compare
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] make_key_with_random drops buffer-protocol handling, inconsistent with encapsulate_with_random —
wolfcrypt/ciphers.py:2151 - [Low] New TypeError validation branches (with_random / seed) are not covered by tests —
wolfcrypt/ciphers.py:2096-2099 - [Low] Random.native_object changed from settable attribute to read-only property —
wolfcrypt/random.py:56-60 - [Info] Abstract properties typed - int are overridden by None class attributes —
wolfcrypt/ciphers.py:175-177
Review generated by Skoll
| ret = _lib.wc_KyberKey_MakeKeyWithRandom( | ||
| mlkem_priv.native_object, _ffi.from_buffer(rand), len(rand) | ||
| ) | ||
| ret = _lib.wc_KyberKey_MakeKeyWithRandom(mlkem_priv.native_object, rand, len(rand)) |
There was a problem hiding this comment.
🟠 [Medium] make_key_with_random drops buffer-protocol handling, inconsistent with encapsulate_with_random
This PR replaces _ffi.from_buffer(rand) with a bare rand in wc_KyberKey_MakeKeyWithRandom(mlkem_priv.native_object, rand, len(rand)), but does NOT add the validation/normalization that its sibling MlKemPublic.encapsulate_with_random gained in the same PR (lines 2096-2101: memoryview(rand) guard + rand = bytes(rand)). The old _ffi.from_buffer(rand) accepted any buffer-protocol object (bytes, bytearray, memoryview); passing such objects straight into cffi now depends on cffi's narrower direct-conversion rules, so a bytearray/memoryview caller that worked before can now raise TypeError. Two public entry points that take a rand buffer for the same crypto module now behave differently. Tests only exercise bytes (ref_rand_for_make_key = h2b(...)), so this asymmetry is not caught.
Fix: Mirror the validation/bytes(rand) conversion used in encapsulate_with_random so the two *_with_random APIs accept the same input types consistently.
| :rtype: tuple[bytes, bytes] | ||
| """ | ||
|
|
||
| try: |
There was a problem hiding this comment.
🔵 [Low] New TypeError validation branches (with_random / seed) are not covered by tests
The PR adds a memoryview(rand) buffer-protocol guard that raises a custom TypeError in encapsulate_with_random (and analogous refactors normalize seed in make_key_from_seed/sign_with_seed). The tests exercise only the happy path with bytes; the new TypeError branch (e.g. passing a non-buffer such as an int or str) is not tested, so a regression in that guard would go unnoticed. Combined with the finding above, the divergence between make_key_with_random and encapsulate_with_random is also untested.
Fix: Add a small negative test for the new buffer-protocol validation branches.
| # Can occur during interpreter shutdown | ||
| pass | ||
|
|
||
| @property |
There was a problem hiding this comment.
🔵 [Low] Random.native_object changed from settable attribute to read-only property
native_object (a non-underscore, effectively public attribute) is converted from a plain writable attribute to a read-only @property backed by _native_object. Any external code that assigned rng.native_object = ... now fails with AttributeError (this is exactly why the test test_delete_descriptor_binding.py had to drop its r.native_object = None line). The change is otherwise reasonable and the internal __del__/init-failure handling is now safer, but this is a backward-incompatible surface change on a public name that is not called out in the ChangeLog (which only mentions 'Add typing annotations').
Fix: Confirm no external consumer sets Random.native_object; consider a brief ChangeLog note that it is now read-only. No code change required.
| @abstractmethod | ||
| def block_size(self) -> int: ... | ||
|
|
||
| @property |
There was a problem hiding this comment.
⚪ [Info] Abstract properties typed - int are overridden by None class attributes
In this typing-focused PR, several abstract properties are declared to return int (_Cipher.key_size, _Cipher.block_size, _Cipher._key_sizes; _Hash.digest_size) but concrete subclasses override them with class attributes set to None (Aes.key_size = None, ChaCha.key_size = None, Sha3.digest_size = None). This is fine at runtime (ABC only checks the name exists, and the falsy None is intentionally used as a sentinel in _Cipher.__init__), but it is type-inconsistent with the abstract signatures and will produce warnings under the newly added [tool.ty] config (all = "warn"). Consider annotating these as int | None (or Optional[int]) to match reality.
Fix: Widen the abstract property annotations to int | None where subclasses legitimately use None as a sentinel, to keep the type declarations consistent.
No description provided.