Lokasi ngalangkungan proxy:   [ UP ]  
[Ngawartoskeun bug]   [Panyetelan cookie]                
Skip to content

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

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

Add typing: asn, exceptions, hashes, pwdbased, utils.#125
roberthdevries wants to merge 6 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
Comment thread wolfcrypt/hkdf.py
Comment thread wolfcrypt/ciphers.py

@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: 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 boundarywolfcrypt/ciphers.py:2372,2391
  • [Low] setup.py install_requires not synced with new typing-extensions runtime dependencysetup.py:62-63
  • [Low] Hard runtime import of private cffi symbol _cffi_backend.Lib only to satisfy a castwolfcrypt/__init__.py:20-22,56

Review generated by Skoll

Comment thread wolfcrypt/ciphers.py
) from exception
if len(seed_view) != cls.ML_DSA_KEYGEN_SEED_LENGTH:

if not isinstance(seed, (list, tuple, bytes)):

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 validation now rejects bytearray/memoryview (regression)

The seed validation was rewritten from a buffer-protocol check to an isinstance(seed, (list, tuple, bytes)) check. The previous code did memoryview(seed) and explicitly documented bytearray support (its error message read "seed must support the buffer protocol, such as bytes or bytearray"). The new check accepts only list, tuple, and bytes, so a caller passing a bytearray or memoryview seed - both previously valid - now gets TypeError: seed must be bytes or list/tuple. For a crypto API this is a real backward-incompatible narrowing, and bytearray is the natural choice for zeroizable secret seeds. This affects both MlDsaPrivate.make_key_from_seed and MlDsaPrivate.sign_with_seed.

Fix: Restore buffer-protocol support by normalizing the seed with bytes(seed) (which also handles list/tuple) before the length check and C call, instead of an isinstance allow-list that drops bytearray/memoryview.

Comment thread wolfcrypt/ciphers.py

@classmethod
def make_key_from_seed(cls, mldsa_type, seed):
def make_key_from_seed(cls, mldsa_type: MlDsaType, seed: bytes | list[int] | tuple[int]) -> MlDsaPrivate:

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] Advertised list/tuple seed support is untested and likely fails at the cffi boundary

The docstrings and the new lib.pyi stub now advertise seed: bytes | list[int] | tuple[int], and the validation lets list/tuple pass through to _lib.wc_dilithium_make_key_from_seed(native_object, seed) / wc_dilithium_sign_*_with_seed(...). The cdef in scripts/build_ffi.py declares the seed parameter as const byte* seed. Standard cffi does not auto-convert a Python list/tuple to a pointer argument (only bytes is accepted for char*/byte*; lists require ffi.new("byte[]", seed)), so passing a list would likely raise an opaque cffi TypeError at the call rather than working as documented. The test suite only exercises bytes seeds (rng.bytes(...), bytes(N)) and the "" rejection case, so this newly-advertised path is entirely uncovered.

Fix: Either normalize the seed to bytes before the C call (fixes both this and the bytearray regression) or drop the list/tuple claim. Add a unit test that passes a list[int] seed to make_key_from_seed/sign_with_seed so the advertised type is actually verified.

Comment thread setup.py
@@ -63,5 +63,5 @@
install_requires=["cffi>=1.0.0,<2"],

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] setup.py install_requires not synced with new typing-extensions runtime dependency

This PR adds typing-extensions to [project].dependencies in pyproject.toml and now imports it unconditionally at module load time (from typing_extensions import override in both wolfcrypt/ciphers.py and wolfcrypt/hashes.py). override only exists in the stdlib typing from Python 3.12+, and the project supports 3.10/3.11, so typing-extensions is a hard runtime dependency. setup.py still lists install_requires=["cffi>=1.0.0,<2"] only. The project specifies dependencies in both pyproject.toml and setup.py; whichever path is used for install metadata, leaving the two out of sync is confusing and risks an ImportError for any install path that honors setup.py's install_requires.

Fix: Add typing-extensions to setup.py's install_requires to match pyproject.toml, or remove the duplicate dependency list from setup.py so pyproject.toml is the single source of truth.

Comment thread wolfcrypt/__init__.py
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA
from typing import cast

from _cffi_backend import Lib

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] Hard runtime import of private cffi symbol _cffi_backend.Lib only to satisfy a cast

wolfcrypt/__init__.py now does from _cffi_backend import Lib at module top and uses it as _ffi.addressof(cast(Lib, _lib), "wc_GenerateSeed"). Because typing.cast(Lib, _lib) evaluates its first argument at runtime, Lib must be importable whenever wolfcrypt is imported. This couples package import to a private cffi implementation detail (_cffi_backend.Lib) purely to silence a type checker; the cast has no runtime effect on behavior. If a future cffi release renames/removes that symbol, importing the whole package breaks for zero functional benefit.

Fix: Guard the Lib import under TYPE_CHECKING and use a # type: ignore (or a string-annotated local) instead of a runtime cast, so package import does not depend on a private cffi symbol.

@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.

See #125 (review)
If you disagree just make note. We are getting close on this and thank you for your efforts

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