Add initial support for crypto callbacks.#114
Add initial support for crypto callbacks.#114roberthdevries wants to merge 2 commits intowolfSSL:masterfrom
Conversation
JeremiahM37
left a comment
There was a problem hiding this comment.
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
| def __enter__(self) -> Self: | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_value, traceback) -> None: |
There was a problem hiding this comment.
🔴 [High] Double unregister of device on context-manager exit + GC
Both __exit__ and __del__ call self._unregister(). When used as a context manager (the test does exactly this: with RngCryptoCallback(10): ...), the device is unregistered on __exit__ and again later when the object is garbage-collected. If a second CryptoCallback was registered with the same device_id between exit and GC (a reasonable usage pattern, e.g., in successive tests), the GC-time unregister will rip out the live registration belonging to the new object, causing test pollution / NULL callback dispatch. There is also no flag to record "already unregistered."
def __exit__(self, exc_type, exc_value, traceback) -> None:
self._unregister()
def __del__(self) -> None:
self._unregister()
def _unregister(self) -> None:
_lib.wc_CryptoCb_UnRegisterDevice(self.device_id)Recommendation: Track registration state with a _registered flag, set it to False after unregistering, and follow the same try/except AttributeError pattern used by Random.__del__ in wolfcrypt/random.py to avoid errors during interpreter shutdown.
There was a problem hiding this comment.
The tracking of the registration is done in the wolfcrypt library itself. Unregistering a device twice is a noop.
__del__ is not certain to be called at any time soon (unless a gc.collect() call is done explicitly) so if more fine-grained control is required to control the lifespan of the callback, a context manager is the typical solution in Python.
So the unregister in __exit__ must not be removed.
There was a problem hiding this comment.
wc_CryptoCb_UnRegisterDevice looks up by devId only — no ownership check (cryptocb.c:428). The double-unregister is only a no-op if the slot is still INVALID_DEVID at the second call. In the cross-instance case (cb1 exits → cb2 registers same devId → cb1 GC'd), cb1's del will find cb2's live slot and ClearDev it. A _registered flag in del avoids this without removing exit's unregister.
* Comment out code in build_ffi.py instead of quoted string. * Check return value of wolfCrypt_init() * Complete algorithm type hash table * Change dicts into defaultdicts with a sensible value when key is missing. * Convert debug print to debug log messages * Add length checks to produce nicer exceptions.
|
|
||
| from wolfcrypt.exceptions import WolfCryptError | ||
|
|
||
| ALGO_TYPE_NAME: Final = defaultdict( |
There was a problem hiding this comment.
Module-level dicts read _lib.WC_ALGO_TYPE_* / _lib.WC_HASH_TYPE_* at import, but those are only in the cdef when CRYPTO_CB is enabled — so import wolfcrypt.cryptocb fails with AttributeError before reaching the if _lib.CRYPTO_CB_ENABLED: guard. Move the dicts inside the guard, and make the CryptoCallback import in wolfcrypt/__init__.py conditional too.
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_CryptoInfostructure 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_CryptoInfostruct.Cffi
cdefand the compiler seem to disagree.However as it is, callbacks work for random and hash functions.