Skip to content

Make the random generator of _Rsa and RsaPublic configurable.#93

Open
roberthdevries wants to merge 1 commit intowolfSSL:masterfrom
roberthdevries:rsa-public-add-rng-param
Open

Make the random generator of _Rsa and RsaPublic configurable.#93
roberthdevries wants to merge 1 commit intowolfSSL:masterfrom
roberthdevries:rsa-public-add-rng-param

Conversation

@roberthdevries
Copy link
Copy Markdown
Contributor

This was the only class that has hardcoded the random number generator in the implementation.

@dgarske
Copy link
Copy Markdown
Contributor

dgarske commented Apr 7, 2026

Okay to test. Contributor agreement on file.

dgarske
dgarske previously approved these changes Apr 8, 2026
@dgarske
Copy link
Copy Markdown
Contributor

dgarske commented Apr 8, 2026

@roberthdevries please resolve the merge conflict. Thanks! Awesome work on these cleanups.

This was the only class that has hardcoded the random number generator
in the implementation.
@roberthdevries
Copy link
Copy Markdown
Contributor Author

Merge conflict is now resolved.

Copy link
Copy Markdown
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 1 skipped

Posted findings

  • [High] Mutable default argument rng=Random() in _Rsa.__init__wolfcrypt/ciphers.py:653
  • [Medium] RsaPrivate.__init__ not updated to accept/forward rng parameterwolfcrypt/ciphers.py:849-851
  • [Medium] No tests for the new rng parametertests/test_ciphers.py
Skipped findings
  • [Medium] No tests for the new rng parameter

Review generated by Skoll via openclaw

_hash_type = None

def __init__(self):
def __init__(self, rng=Random()):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [High] Mutable default argument rng=Random() in _Rsa.__init__
🚫 BLOCK bug

The default argument rng=Random() is a classic Python mutable-default-argument bug. Random() is evaluated once at class definition time, and the same object is reused for every call that omits rng. This means all RsaPrivate instances (whose __init__ calls _Rsa.__init__(self) without providing rng on line 851) will share the exact same Random object. If one instance is garbage-collected and its Random object cleaned up, every other instance sharing that default will hold a dangling reference. Even without cleanup issues, sharing a single RNG across independent key objects is a correctness and isolation problem. Ironically, the RsaPublic subclass does this correctly with the rng=None / if rng is None: rng = Random() pattern — the base class should use the same approach.

Suggestion:

Suggested change
def __init__(self, rng=Random()):
def __init__(self, rng=None):
self.native_object = _ffi.new("RsaKey *")
ret = _lib.wc_InitRsaKey(self.native_object, _ffi.NULL)
if ret < 0: # pragma: no cover
raise WolfCryptError("Invalid key error (%d)" % ret)
if rng is None:
rng = Random()
self._random = rng

_Rsa.__init__(self, rng)

idx = _ffi.new("word32*")
idx[0] = 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] RsaPrivate.__init__ not updated to accept/forward rng parameter
💡 SUGGEST api

The PR's stated goal is to make the random generator of _Rsa and RsaPublic configurable. However, RsaPrivate.__init__ (line 849) was not updated to accept an rng parameter, and its call to _Rsa.__init__(self) on line 851 still relies on the default. Since RsaPrivate extends RsaPublic, users would reasonably expect RsaPrivate to also support a configurable RNG. Additionally, RsaPrivate.make_key (line 829) and RsaPublic.from_pem (line 718) also don't forward an rng parameter, leaving the configurability incomplete.

Suggestion:

Suggested change
idx[0] = 0
def __init__(self, key=None, hash_type=None, rng=None): # pylint: disable=super-init-not-called
if rng is None:
rng = Random()
_Rsa.__init__(self, rng) # pylint: disable=non-parent-init-called

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.

3 participants