cryptography
cryptography copied to clipboard
pyca
Expose EC group order
Here's a patch to expose the group order of EllipticCurve objects. As mentioned on IRC, this makes it easier to write code to enforce "Low-S" ECDSA signatures without hard-coding parameters (but does not itself implement anything of the sort).
Assuming I cleaned this up and added a test or two, is this something you'd consider merging?
I have not had a chance to review this in any detail, but I wanted to flag that we are (slowly) migrating away from cffi, so new PRs shouldn't be introducing any new usages of it.
If we want to add group order is there any reason to do it dynamically or should we just add it as a class constant on the curve classes?
That makes sense to me.
On Mon, Mar 18, 2024, 11:28 AM Paul Kehrer @.***> wrote:
If we want to add group order is there any reason to do it dynamically or should we just add it as a class constant on the curve objects?
— Reply to this email directly, view it on GitHub https://github.com/pyca/cryptography/pull/10600#issuecomment-2004218807, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBC23FH7C22QNCZDHCDYY4BZZAVCNFSM6AAAAABE2VONFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBUGIYTQOBQG4 . You are receiving this because you commented.Message ID: @.***>
The main reason I didn't just add them as constants is because it's kinda hard to demonstrate their correctness/legitimacy. Someone has to hard-code them somewhere (or calculate from first principles I suppose), and since openssl has already done that (in openssl/crypto/ec/ec_curve.c) I thought it'd be best to piggyback off that effort.
But, if you're happy to just have them as constants, then that works for me.
As for testing for correctness, it just occurred to me that an easy test would be to generate a signature, replace s with group_order-s, and check that the signature is still valid.
I think we're fine with constants -- we can check them against authoritative sources as part of review 😄
Yes, thanks for the reminder. I'll close this PR for now, and open a new one following your suggested approach of adding constants.