Skip to content

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

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

Add typing: asn, exceptions, hashes, pwdbased, utils.#125
roberthdevries wants to merge 3 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
@abstractmethod
def _decrypt(self, destination: _ffi.CData, source: bytes) -> int: ...

@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] _Cipher.new() dropped kwargs, breaking PEP 272 extra keyword arguments

The signature changed from def new(cls, key, mode, IV=None, **kwargs) to def new(cls, key, mode, IV=None) -> _Cipher. The original **kwargs (kept with a pylint: disable=W0613) silently absorbed extra keyword arguments, which is part of the PEP 272 new() convention (callers may pass things like counter or segment_size). Removing it means any caller passing an extra keyword now gets a TypeError. This is a subtle, possibly unintended API narrowing.

Fix: Keep **kwargs for backward compatibility unless you intend to formally drop the PEP 272 extra-argument contract.

Comment thread wolfcrypt/asn.py
raise WolfCryptError(f"Unknown hash class {hash_cls.__name__}")

def make_signature(data, hash_cls, key=None):
def make_signature(data: bytes, hash_cls: type[_Hash], key = None) -> 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] asn.py leaves function arguments unannotated while enabling ANN ruff rules

This PR adds "ANN" to the ruff select list and only ignores it for scripts/*.py and tests/*.py (not wolfcrypt/). Yet make_signature(data, hash_cls, key = None) leaves key unannotated and check_signature(signature, data, hash_cls, pub_key) leaves pub_key unannotated. ruff's ANN001 (missing-type-function-argument) would flag both, so ruff check with the new config would report violations in a file the PR is explicitly typing. This is an internal inconsistency in the typing pass.

Fix: Complete the annotations in asn.py (and verify ruff ANN passes for wolfcrypt/) so the newly enabled rule does not fail lint.

Comment thread tests/test_mldsa.py
# test that the seed type is checked (should be bytes-like, not string)
with pytest.raises(TypeError):
_ = mldsa_priv.sign_with_seed(message, "")
_ = mldsa_priv.sign_with_seed(message, " " * ML_DSA_SIGNATURE_SEED_LENGTH)

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] test_mldsa now relies on cffi's low-level TypeError instead of an explicit guard

The removed code in sign_with_seed/make_key_from_seed previously raised a clear, library-owned TypeError ('seed must support the buffer protocol...') when given a non-buffer seed. With that guard gone, the test was changed from sign_with_seed(message, "") to sign_with_seed(message, " " * ML_DSA_SIGNATURE_SEED_LENGTH) so that the length check passes and the str then reaches CFFI, which raises TypeError. The test still passes, but it now asserts on CFFI's internal type rejection ('must be a bytes or list or tuple, not str') rather than a wolfcrypt-controlled error. This couples the test to CFFI internals and produces a less user-friendly error for callers.

Fix: Optional: re-add an explicit type guard for clearer errors; otherwise acceptable as-is.

Comment thread wolfcrypt/ciphers.py

class RsaPublic(_Rsa):
def __init__(self, key=None, hash_type=None, rng=None):
def __init__(self, key: BytesOrStr, hash_type: int | None = None, rng: Random | None = None) -> None:

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] RsaPublic.init made key a required positional argument

The signature changed from __init__(self, key=None, hash_type=None, rng=None) to __init__(self, key: BytesOrStr, hash_type=None, rng=None), removing the default for key. RsaPublic() / RsaPublic(key=None) now raises TypeError/t2b(None) TypeError instead of the previous (already-broken) path that crashed at len(key). All in-repo callers (and RsaPrivate, which uses its own init) pass a key, so impact is minimal, but it is a public signature change worth noting in case external code instantiated RsaPublic without a key.

Fix: Low priority; document the signature change. No functional regression for existing callers.

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