Skip to content

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

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

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

@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

Also added a line to the change log to mention that typing information
has been added.
dgarske
dgarske previously approved these changes Jun 22, 2026
@dgarske dgarske assigned dgarske and unassigned roberthdevries Jun 22, 2026

@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] 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 seedswolfcrypt/ciphers.py:2383,2536
  • [Info] New module wolfcrypt/types.py shadows the stdlib types module namewolfcrypt/types.py:1
  • [Info] Inconsistent # ty:ignore comment lacks the space used everywhere elsewolfcrypt/__init__.py:53

Review generated by Skoll

Comment thread pyproject.toml Outdated
Comment thread wolfcrypt/ciphers.py
Comment thread wolfcrypt/types.py Outdated
Comment thread wolfcrypt/__init__.py Outdated
@dgarske dgarske assigned roberthdevries and unassigned dgarske Jun 22, 2026

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

Comment thread tests/test_ciphers.py
@@ -320,7 +319,7 @@ def chacha_obj(vectors):
return r

@pytest.fixture

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

Comment thread wolfcrypt/ciphers.py

class RsaPublic(_Rsa):
def __init__(self, key=None, hash_type=None, rng=None):
class RsaPublic(_Rsa, SupportsRsaVerify):

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.

⚪ [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 dgarske assigned roberthdevries and unassigned dgarske Jul 2, 2026

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

Please also resolve merge conflicts.

#125 (review)

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