Add initial support for crypto callbacks.

[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

[dgarske]

🟠 [Medium] cipher_callback path returns success (0) without performing any cipher operation

For WC_ALGO_TYPE_CIPHER, callback() calls self.cipher_callback(device_id) and then unconditionally return 0. The cipher fields of wc_CryptoInfo are intentionally not exposed in the cdef (commented out per the PR description), so a subclass overriding cipher_callback has no way to read inputs or write outputs, yet a no-op override would cause wolfCrypt to be told the cipher succeeded while no encryption/decryption was actually performed. Out of the box this is masked because the default cipher_callback raises NotImplementedError (caught -> CRYPTOCB_UNAVAILABLE), but exposing an overridable cipher_callback hook that returns success-without-effect is a footgun, especially for a crypto library where a silent no-op cipher is dangerous.

Fix: Until the cipher struct is actually exposed, either remove the cipher_callback hook or always return CRYPTOCB_UNAVAILABLE for cipher so wolfCrypt falls back to software, rather than returning 0/success for an operation that did nothing.

[dgarske]

🟠 [Medium] Hash callbacks and error paths are untested; default-device-id test asserts nothing

Only the RNG callback path is exercised. There is no coverage for hash_update_callback / hash_finalize_callback (the most complex branch in callback(), including the NULL-digest update vs finalize distinction and the digest-length validation), no coverage for the length-mismatch error branches (which is exactly where the success-on-exception bug above manifests), and no coverage for the cipher / unknown-algo fallback to CRYPTOCB_UNAVAILABLE. test_default_device_id only print()s the value and asserts nothing, so it cannot fail meaningfully.

Fix: Add tests for the hash update/finalize paths, the length-mismatch/error paths, and the unknown-algo fallback; make test_default_device_id assert on the return value.

[dgarske]

🟠 [Medium] del unregister not guarded against interpreter-shutdown errors

__del__ calls self._unregister() which dereferences module-level _lib. During interpreter shutdown _lib/wc_CryptoCb_UnRegisterDevice may already be torn down, raising AttributeError (or worse) from a destructor, producing noisy 'Exception ignored in del' output. The existing Random.__del__ in wolfcrypt/random.py explicitly guards this exact scenario with a try/except for shutdown. The new code should follow the same established pattern. (Also note __exit__ followed by __del__ calls _unregister twice; this is harmless since unregister is idempotent, but the shutdown guard is still warranted.)

Fix: Guard the destructor's unregister against shutdown-time teardown, matching the pattern already used in Random.__del__.

[dgarske]

🟠 [Medium] CRYPTO_CB cdef hash union depends on SHA/SHA256/SHA384/SHA512/SHA3 cdefs being present

The wc_CryptoInfo.hash.u union added under if features["CRYPTO_CB"] references the C types wc_Sha, wc_Sha256, wc_Sha384, wc_Sha512, and wc_Sha3*. Those typedef struct {...} declarations are emitted only under their own feature guards (if features["SHA"], ["SHA256"], etc., lines 845-908). If a build enables CRYPTO_CB while any of those hash features is disabled, the cdef will reference an undeclared type and ffibuilder.cdef()/compile will fail. The default build enables all of them so it works today, but the implicit coupling is fragile and undocumented.

Fix: Make the CRYPTO_CB hash-union cdef robust to disabled hash features, or document/assert the dependency so a non-default feature combination fails with a clear message instead of an opaque cdef error.

[dgarske]

🔵 [Low] Inconsistent logging style in callback (f-string vs lazy %-formatting)

Line 104 uses an eagerly-interpolated f-string inside log.debug(...), while line 109 uses lazy %s formatting. Mixing the two is inconsistent and the f-string form is what pylint's logging-fstring-interpolation flags (the file already disables several pylint checks at the top). Pick one style; lazy %-args avoid formatting cost when debug logging is disabled.

Fix: Use lazy %-style logging consistently across the callback.

[roberthdevries]

Fixed.

[dgarske]

🔵 [Low] wolfCrypt_Init() added with no corresponding cleanup

wolfCrypt_Init() is now called unconditionally on package import (a reasonable change, and required for the cryptocb registration to work), but there is no matching wolfCrypt_Cleanup() registered (e.g. via atexit). For a process-lifetime library this is generally acceptable, but it is worth a deliberate decision/comment so it is clear cleanup was intentionally omitted rather than forgotten.

Fix: Confirm the omission of wolfCrypt_Cleanup() is intentional; add a brief comment or an atexit cleanup if appropriate.

[roberthdevries]

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

[roberthdevries]

Fixed.