[roberthdevries]

This patch does not add support for types of callbacks. Adding support for callbacks for cipher functions is complicated by issues with cffi code generation for the wc_CryptoInfo structure with two layers of anonymous unions.
Uncommenting more parts of the cipher struct causes errors regarding conflicting struct sizes of other parts of the wc_CryptoInfo struct.
Cffi cdef and the compiler seem to disagree.

However as it is, callbacks work for random and hash functions.

[JeremiahM37]

Skoll Code Review

Scan type: review
Overall recommendation: REQUEST_CHANGES
Findings: 14 total — 9 posted, 5 skipped
7 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] Importing wolfcrypt fails when CRYPTO_CB is disabled at build time — wolfcrypt/__init__.py:49 and wolfcrypt/cryptocb.py:33-58
• [High] Double unregister of device on context-manager exit + GC — wolfcrypt/cryptocb.py:73-77, 117-118
• [Medium] Return value of wolfCrypt_Init not checked — wolfcrypt/__init__.py:52
• [Medium] Debug print() statements left in production callback dispatcher — wolfcrypt/cryptocb.py:80, 84, 85, 107, 111
• [Medium] Hash digest dict KeyError on unmapped hash types — wolfcrypt/cryptocb.py:84, 90
• [Medium] Digest output buffer not size-validated — wolfcrypt/cryptocb.py:90
• [Medium] RNG output length not validated — wolfcrypt/cryptocb.py:96-97
• [Medium] No tests for hash callbacks despite PR claim — tests/test_cryptocb.py
• [Low] Free-floating string literal used as 'comment' in build_ffi.py is misleading — scripts/build_ffi.py:1376-1412

Skipped findings

• [Medium] Anonymous-union cdef relies on '...' partial layout — may not match reality
• [Low] ctx==NULL fallback would never be reached — wrong return code
• [Low] Type annotations on overridable methods declare positional args with leading underscores
• [Low] rng_callback signature passes raw cffi pointer with no type annotation
• [Info] Stale exception import in init.py guard

Review generated by Skoll

[dgarske]

Hi @roberthdevries nice work on this. See peer review feedback and also fix merge conflicts. Thanks

[dgarske]

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] Callback exceptions are converted to 0 (success) with unwritten output buffer — wolfcrypt/__init__.py:57-63, wolfcrypt/cryptocb.py:103-138
• [Medium] cipher_callback path returns success (0) without performing any cipher operation — wolfcrypt/cryptocb.py:125-127,149-150
• [Medium] Hash callbacks and error paths are untested; default-device-id test asserts nothing — tests/test_cryptocb.py:33-53
• [Medium] del unregister not guarded against interpreter-shutdown errors — wolfcrypt/cryptocb.py:100-101,152-153
• [Medium] CRYPTO_CB cdef hash union depends on SHA/SHA256/SHA384/SHA512/SHA3 cdefs being present — scripts/build_ffi.py:1424-1440
• [Low] Inconsistent logging style in callback (f-string vs lazy %-formatting) — wolfcrypt/cryptocb.py:104,109
• [Low] wolfCrypt_Init() added with no corresponding cleanup — wolfcrypt/__init__.py:53-55

Review generated by Skoll

[roberthdevries]

Not all issues are addressed. Will continue working on this after typing is merged so that this part can be typed as well.

[dgarske]

@roberthdevries now that the other PR's have been merged please take another look at the remaining items. Thanks!

[dgarske]

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 8 total — 8 posted, 0 skipped
8 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] Import-time reference to _lib.INVALID_DEVID reintroduces issue #2659 regression — wolfcrypt/hashes.py:43
• [High] device_id silently ignored for Sha256/Sha384/Sha512 (callback never fires) — wolfcrypt/hashes.py:235-298
• [High] Uncaught callback exceptions report success (0) to wolfCrypt with an unfilled buffer — wolfcrypt/__init__.py:64-69
• [Medium] Sha3 constructor does not expose device_id despite _init supporting it — wolfcrypt/hashes.py:352-358
• [Medium] CryptoCallback.del not guarded for interpreter shutdown; double-unregister and self-reference cycle — wolfcrypt/cryptocb.py:96-103
• [Medium] Missing test coverage for callback error paths and SHA-2 device_id — tests/test_cryptocb.py:1-87
• [Low] Hash update-vs-finalize discriminator drops input when both in and digest are set — wolfcrypt/cryptocb.py:112-126
• [Low] lib.pyi wc_HashInfo union field name does not match C member — wolfcrypt/_ffi/lib.pyi:592-601

Review generated by Skoll

[dgarske]

#114 (review)

[roberthdevries]

Now that the INVALID_DEVID constant is always available, the need for the regression test for the no-pqc job becomes questionable.
Propose to remove it from the pipeline

[roberthdevries]

Not all review comments addressed. More time needed.

[dgarske]

Hi @roberthdevries , is this PR ready for another review or are you planning to push further work?

[dgarske]

Hi @roberthdevries , checking on this again to see if you have pushed and finalized all the work planned. This is a really neat feature additional.

[Copilot]

🟡 Changes recommended

CFFI field names break callback-enabled builds, and hash hooks cannot distinguish independent hash contexts.

2 open issues

• CFFI hash field aliases mismatch native names and break callback builds · New
• Hash hooks discard per-instance context, mixing interleaved hash states · New

What changed in this PR

Adds initial wolfCrypt callback support for random generation and hashing, with software fallback for unsupported operations.

Changes:

• Adds callback registration, dispatch, and error handling.
• Threads device IDs through cryptographic wrappers.
• Extends CFFI bindings and adds callback tests.

File	Description
wolfcrypt/​random.py	Uses the native invalid-device constant.
wolfcrypt/​hashes.py	Adds device-aware hash and HMAC initialization.
wolfcrypt/​exceptions.py	Retains native error codes.
wolfcrypt/​cryptocb.py	Adds callback registration and dispatch.
wolfcrypt/​ciphers.py	Adds device selection for streaming AES-GCM.
wolfcrypt/​_ffi/​lib.pyi	Declares callback types and native APIs.
wolfcrypt/​__init__.py	Initializes wolfCrypt and registers the callback bridge.
tests/​test_cryptocb.py	Tests callbacks, fallback, errors, and registration ownership.
scripts/​build_ffi.py	Enables callbacks and extends native bindings.
.github/​workflows/​python-app.yml	Updates the regression-test explanatory comment.

_{Review effort: Balanced}

[philljj]

I'm not sure .memmove() is safe for copying a wc_Sha256 here.

We use

int wc_Sha256Copy(wc_Sha256* src, wc_Sha256* dst)

for deep copying the wc_Sha256 , because with WOLFSSL_SMALL_STACK_CACHE there are nested pointer structures.

The class does define it for the _copy member:

if _lib.SHA256_ENABLED:                                                         
    class Sha256(_Sha):                                                         
...                           
        _delete = staticmethod(_lib.wc_Sha256Free)                              
        _copy = staticmethod(_lib.wc_Sha256Copy)
...

Maybe can be done through the class copy member and it will polymorphically use the right one

[philljj]

skoll:

HIGH-1: Native hash copies can share allocations that are freed twice

• File: wolfcrypt/cryptocb.py:201-205
• Function: CryptoCallback._hash_copy

Description: The new callback replaces wolfCrypt's native copy operation with memmove and returns success, bypassing resource duplication. With WOLFSSL_SMALL_STACK_CACHE, SHA-256 initialization allocates a W buffer, so the original and copy now share that pointer. digest() frees its temporary copy, releasing the original's buffer; subsequent cleanup can free it again. This affects callback-enabled hashing against supported local wolfSSL builds. The pinned wolfSSL implementation allocates independent storage in wc_Sha256Copy, which this PR bypasses.

Code:

_ffi.memmove(dst, src, _ffi.sizeof(NATIVE_HASH_TYPE[hash_type]))
self._hash_contexts[dst_key] = copy.deepcopy(self._hash_contexts[src_key])
return 0

Recommendation: Rework callback hash copying to preserve native resource ownership through the appropriate wc_*Copy lifecycle while also copying the Python context. Add a repeated digest/copy/free regression with WOLFSSL_SMALL_STACK_CACHE enabled.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #114

Scan targets checked: wolfcrypt-py-src, wolfcrypt-py-bugs
Not reviewed (no scan target covers them): .github/workflows/python-app.yml, tests/test_cryptocb.py, wolfcrypt/_ffi/lib.pyi
Coverage: 3 of 7 in-scope changed file(s) opened by the reviewer; not opened: wolfcrypt/__init__.py, wolfcrypt/ciphers.py, wolfcrypt/exceptions.py, wolfcrypt/random.py

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Lite

[wolfSSL-Fenrir-bot]

HMAC on a hash-callback device produces an incorrect MAC because digest() finalizes a memmove'd copy with no callback… · Logic errors

The new device_id routes HMAC's inner hash through CryptoCallback, which stores hash state by native-object address. _Hmac has no _copy, so digest() memmoves the struct into a new address and finalizes it with an empty context. The ipad, key and data are dropped and the returned MAC is wrong.

Suggested fix: Before accepting device_id for HMAC, decide how hash-callback state follows the Hmac struct copy in digest(); consult the wolfCrypt crypto-callback/HMAC documentation.

Related known findings (similar but distinct; listed for context, not part of this finding)

• F-6629: that finding is about device-state aliasing and use-after-free from the memmove. This one is a silently wrong MAC caused by the new device_id plumbing combined with the address-keyed _hash_contexts added in cryptocb.py.

[philljj]

my skoll run also hit this:

HIGH-2: HMAC device IDs expose embedded hashes to callbacks without copying their state

• File: wolfcrypt/hashes.py:506-507
• Function: _Hmac._hmac_init

Description: Forwarding device_id lets wolfCrypt's embedded HMAC hash invoke the new stateful hash callbacks. However, HMAC still inherits digest()'s raw memmove path because it has no _copy function. The temporary HMAC therefore has a different embedded-hash address without a corresponding entry in _hash_contexts. Finalization starts with an empty context and silently computes the wrong MAC. An isolated probe using the PR's Python code and the installed wolfSSL library reproduced this for HmacSha256(b'key', b'data', device_id=19) with callbacks implementing ordinary SHA-256. The new HMAC test uses a callback that handles no hashes, so it misses this interaction.

Code:

def _hmac_init(self, hmac: int, key: bytes, device_id: int) -> int:
    ret = _lib.wc_HmacInit(self._native_object, _ffi.NULL, device_id)

Recommendation: The fix needs a design decision for preserving embedded callback state during HMAC digest operations; consult wolfCrypt's HMAC API and the package's non-mutating digest() contract, and cover the chosen behavior with a hash-capable callback regression.