python: T8859: validate min_keysize once in verify_diffie_hellman_length#5194
python: T8859: validate min_keysize once in verify_diffie_hellman_length#5194andamasov wants to merge 1 commit into
Conversation
The previous implementation wrapped str(min_keysize) in try/except but never used the result, and the later int(min_keysize) call on line 444 could still raise ValueError for non-numeric inputs (e.g. 'abc'), surfacing as an unhandled exception instead of returning False. Convert min_keysize once inside the try/except (catching TypeError and ValueError), and reuse that value for the comparison. The unused keysize variable is dropped. No behavior change for valid integer / integer-string inputs; the only observable change is that previously-uncaught ValueError on a non-numeric min_keysize now correctly returns False as the function's contract implies.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👍 |
|
✅ No typos found in changed files. |
|
CI integration ❌ failed! Details
|
There was a problem hiding this comment.
I agree validation could be better here but I don't think this solution is the best one.
One note: this function is not actually used in any real scripts. Its only callers are unit tests for this very function (https://github.com/vyos/vyos-1x/blob/current/src/tests/test_configverify.py#L25-L31). Which means we can change it freely however we want, since we only need to update two tests for that.
If we are to change it, I believe it should be like this:
- Its signature should be
def verify_diffie_hellman_length(file: str, min_keysize: int):—calling it with an argument that makes no sense is a logic problem in a script. - It should throw a
TypeErrorifmin_keysizeis not an integer at all. - It should also throw a
ValueErrorifmin_keysizeis not a positive integer (because key size of-512makes no more sense thanfgsfds).
2 participants
Tracked in Phorge T8859.
Defect
verify_diffie_hellman_length(file, min_keysize)inpython/vyos/configverify.pywrapsstr(min_keysize)intry/except, but:int(min_keysize)call on the comparison line can still raiseValueErrorfor non-numeric inputs (e.g.'abc'), surfacing as an unhandled exception instead of returningFalseas the function intends.keysizevariable was computed but never used — the function only consumesmin_keysizeafterward.Fix
Convert
min_keysizeonce insidetry/except (TypeError, ValueError), store asmin_keysize_int, and reuse for the bits comparison. Unusedkeysizevariable dropped.Behavior change
None for valid integer / integer-string inputs (the common case from callers in
src/conf_mode/vpn_ipsec.pyand similar). The only observable change is that a non-numericmin_keysizenow correctly returnsFalseinstead of propagatingValueError.Provenance
Found by Copilot during review of vyos/vyos-1x#5074 — the configverify changes were dropped from that PR per dmbaturin's scope narrowing. This defect is independent of the broader configverify redesign concerns (MTU helpers taking defaults as args) and can be fixed in isolation.
Test plan
vpn_ipsec,vpn_openconnectand any others callingverify_diffie_hellman_length).min_keysizeand verify the function returnsFalseinstead of raising.min_keysizeagainst a known-good DH file and verify return isTruewhen bits ≥ keysize.🤖 Generated by robots