Skip to content

Add typing: asn, exceptions, hashes, pwdbased, utils.#125

Open
roberthdevries wants to merge 5 commits into
wolfSSL:masterfrom
roberthdevries:add-more-typing
Open

Add typing: asn, exceptions, hashes, pwdbased, utils.#125
roberthdevries wants to merge 5 commits into
wolfSSL:masterfrom
roberthdevries:add-more-typing

Conversation

@roberthdevries

Copy link
Copy Markdown
Contributor

No description provided.

@roberthdevries roberthdevries force-pushed the add-more-typing branch 2 times, most recently from 65d14db to 1c2b082 Compare May 23, 2026 14:02
@Trooper-X

Copy link
Copy Markdown

This also adds the ruff and ty dependencies.
Would be nice if this MR gets merged.

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lengthwolfcrypt/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 itwolfcrypt/ciphers.py:2360-2367
  • [Low] ChaCha init renamed size to _size, breaking the documented backward-compatible keywordwolfcrypt/ciphers.py:544
  • [Low] HKDF helpers annotate hash_cls as instance type instead of class typewolfcrypt/hkdf.py:33,78,105
  • [Low] Random no longer nulls native_object on init failure; del frees an uninitialized RNGwolfcrypt/random.py:37-52
  • [Low] RsaPublic.init made key a required positional argumentwolfcrypt/ciphers.py:771-774

Review generated by Skoll

Comment thread wolfcrypt/ciphers.py
Comment thread wolfcrypt/ciphers.py
Comment thread wolfcrypt/ciphers.py
Comment thread wolfcrypt/ciphers.py Outdated
Comment thread wolfcrypt/random.py Outdated
Comment thread wolfcrypt/ciphers.py
This has some fallout in random.py to simplify checks.
Also one test is slightly adapted to produced the desired failure.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_extensionswolfcrypt/ciphers.py:28, wolfcrypt/hashes.py:28
  • [Medium] Removed _ffi.from_buffer() drops bytearray/memoryview support for seed/rand inputswolfcrypt/ciphers.py:2383, 2548, 2559, 2067, 2110
  • [Medium] **_Cipher.new() dropped kwargs, breaking PEP 272 extra keyword argumentswolfcrypt/ciphers.py:187-199
  • [Medium] HKDF functions annotate hash_cls as instance type instead of class typewolfcrypt/hkdf.py:33, 78, 105
  • [Medium] asn.py leaves function arguments unannotated while enabling ANN ruff ruleswolfcrypt/asn.py:81, 99
  • [Low] test_mldsa now relies on cffi's low-level TypeError instead of an explicit guardtests/test_mldsa.py:186
  • [Low] RsaPublic.init made key a required positional argumentwolfcrypt/ciphers.py:781

Review generated by Skoll

Comment thread wolfcrypt/ciphers.py
Comment thread wolfcrypt/asn.py Outdated
Comment thread tests/test_mldsa.py Outdated
Comment thread wolfcrypt/ciphers.py
Tests are now also type checked as this helps verifying the correctness
of the type annotations.
@dgarske dgarske self-requested a review June 11, 2026 20:50

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 inconsistentlywolfcrypt/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 randwolfcrypt/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

Comment thread wolfcrypt/ciphers.py
@@ -2004,7 +2068,7 @@ def encapsulate_with_random(self, rand):
ct = _ffi.new(f"unsigned char[{ct_size}]")
ss = _ffi.new(f"unsigned char[{ss_size}]")
ret = _lib.wc_KyberKey_EncapsulateWithRandom(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*🔵 [Low] ML-KEM _with_random helpers no longer accept bytearray/memoryview for rand

These calls dropped _ffi.from_buffer(rand) and now pass rand directly to wc_KyberKey_EncapsulateWithRandom / wc_KyberKey_MakeKeyWithRandom (const unsigned char*). Unlike decode_key/decapsulate, rand is not run through t2b, so a bytes value still works but a bytearray or memoryview (which from_buffer previously handled) now raises a cffi TypeError. The annotation was narrowed to rand: bytes, which documents the intent, but it is still a silent runtime behavior change for callers passing other buffer types. Tests only pass bytes (unhexlify(...)), so the change is untested.

Fix: Either normalize rand with t2b() (matching the other ML-KEM methods) to preserve buffer-protocol support, or explicitly document that only bytes is accepted now.

Comment thread wolfcrypt/hkdf.py
@@ -28,8 +28,9 @@


if _lib.HKDF_ENABLED:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] hkdf.py forces a runtime import of _Hmac for a type annotation (no from future import annotations)

Unlike the other modified modules (asn.py, ciphers.py, hashes.py, pwdbased.py), hkdf.py does not add from __future__ import annotations. As a result the new annotation def HKDF(hash_cls: type[_Hmac], ...) is evaluated at import time, which is why a real runtime import from wolfcrypt.hashes import _Hmac was added inside if _lib.HKDF_ENABLED:. _Hmac is only defined when _lib.HMAC_ENABLED. In a build where HKDF is enabled but HMAC is not, import wolfcrypt.hkdf would now raise ImportError where it previously succeeded. wolfSSL's HKDF structurally depends on HMAC so this is unlikely in practice, but the new import coupling is avoidable.

Fix: Add from __future__ import annotations to hkdf.py and move the _Hmac import under TYPE_CHECKING (or guard it on _lib.HMAC_ENABLED) so the annotation does not create a hard runtime dependency.

Comment thread wolfcrypt/ciphers.py
@@ -2306,45 +2369,38 @@ def make_key(cls, mldsa_type, rng=None):
return mldsa_priv

@classmethod

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [Medium] ML-DSA seed handling: bytearray/memoryview now rejected, and the two seed methods validate inconsistently

Before this PR both methods accepted any buffer-protocol object (bytes, bytearray, memoryview) via memoryview(seed) followed by _ffi.from_buffer(seed_view). The PR removes that normalization and the two methods now diverge:

  • make_key_from_seed only does len(seed) and passes seed straight to wc_dilithium_make_key_from_seed(...) (a const byte* argument). CFFI accepts bytes, list and tuple for that argument but raises TypeError for bytearray and memoryview (verified empirically against _cffi_backend). So a bytearray/memoryview seed that worked before now fails with an opaque cffi error.
  • sign_with_seed adds an explicit if not isinstance(seed, (list, tuple, bytes)): raise TypeError(...), which also rejects bytearray/memoryview but with a different (clearer) error.

Net effect: the accepted seed set changed from {bytes, bytearray, memoryview} to {bytes, list, tuple}. bytes (the common path, and the only one exercised by tests) still works, but bytearray/memoryview are a silent regression, and the two methods now raise different error types for the same bad input. No test covers list/tuple seeds or bytearray/memoryview seeds, so CI will not catch the change.

Fix: Apply the same validation/normalization in both methods. If the input-type contract is intentionally being narrowed, note it in ChangeLog.rst and add tests covering list/tuple (and rejected bytearray) seeds so the behavior is pinned.


Note: Referenced line (wolfcrypt/ciphers.py:2371-2392 (make_key_from_seed), 2516-2540 (sign_with_seed)) is outside the diff hunk. Comment anchored to nearest changed region.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants