On Thu, 11 Apr 2024 17:15:21 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:
>>> In `ECOperations.java`, if I understand this correctly, it is to replace >>> the existing `PointMultiplier` with montgomery-based PointMuliplier. But >>> when I look at the code, I see both are still options. If I read this >>> correctly, it checks for the old `IntegerFieldModuloP`, then looks for the >>> new `IntegerMontgomeryFieldModuloP`. It appears to use the new one always. >>> Why doesn't it just replace the old implementation entry in the `fields` >>> Map? Is there a reason to keep it around? >> >> Hmm, thats a good point I haven't fully considered; i.e. (if I read >> correctly) "for `CurveDB.P_256` remove the fallback path to non-montgomery >> entirely".. that might also help in cleaning a few things up in the >> construction. Maybe even get rid of this nested ECOperations inside >> ECOperations.. Perhaps nesting isnt a big deal, but all attempts to make the >> ECC stack clearer is positive! >> >> One functional reason that might justify keeping it as-is, is fuzz-testing; >> with the fallback available, I am able to write the included Fuzz tests and >> have them check the values against the existing implementation. While I also >> included a few KAT tests using openssl-generated values, the fuzz tests >> check millions of values and it does add a lot more certainty about >> correctness of this code. >> >> Can it be removed? For the operations that do not involve multiplication >> (i.e. `setSum(*)`), montgomery is expensive. I think I did go through the >> uses of this code some time back (i.e. ECDHE, ECDSA and KeyGeneration) and >> existing IntegerPolynomialP256 is no longer used (I should verify that >> again) and only P256OrderField remains non-montgomery. So removing >> references to IntegerPolynomialP256 in ECOperations should be possible and >> cleaner. Removing IntegerPolynomialP256 from MontgomeryIntegerPolynomialP256 >> is harder (fromMontgomery() uses IntegerPolynomialP256) but perhaps also >> worth some thought.. >> >> I tend to like `ECOperationsFuzzTest.java` and would prefer to keep it, but >> it could also be chucked up as part of 'scaffolding' and removed in name of >> code quality? >> >> Thanks @ascarpino >> >> PS: Perhaps there is some middle ground, remove the `ECOperations >> montgomeryOps` nesting, and construct (somehow?? singleton makes most things >> inaccessible..) the reference ECOperations in the fuzz test instead.. not >> sure how yet, but perhaps worth a further thought.. > >> > In `ECOperations.java`, if I understand this correctly, it is to replace >> > the existing `PointMultiplier` with montgomery-based PointMuliplier. But >> > when I look at the code, I see both are still options. If I read this >> > correctly, it checks for the old `IntegerFieldModuloP`, then looks for the >> > new `IntegerMontgomeryFieldModuloP`. It appears to use the new one always. >> > Why doesn't it just replace the old implementation entry in the `fields` >> > Map? Is there a reason to keep it around? >> >> Hmm, thats a good point I haven't fully considered; i.e. (if I read >> correctly) "for `CurveDB.P_256` remove the fallback path to non-montgomery >> entirely".. that might also help in cleaning a few things up in the >> construction. Maybe even get rid of this nested ECOperations inside >> ECOperations.. Perhaps nesting isnt a big deal, but all attempts to make the >> ECC stack clearer is positive! >> >> One functional reason that might justify keeping it as-is, is fuzz-testing; >> with the fallback available, I am able to write the included Fuzz tests and >> have them check the values against the existing implementation. While I also >> included a few KAT tests using openssl-generated values, the fuzz tests >> check millions of values and it does add a lot more certainty about >> correctness of this code. > > I hadn't looked at your fuzz test until you mentioned it. I see you are > using reflection to change the values. Is that what you mean by "fallback"? > I'm assuming there is no to access the older implementation without > reflection. > >> >> Can it be removed? For the operations that do not involve multiplication >> (i.e. `setSum(*)`), montgomery is expensive. I think I did go through the >> uses of this code some time back (i.e. ECDHE, ECDSA and KeyGeneration) and >> existing IntegerPolynomialP256 is no longer used (I should verify that >> again) and only P256OrderField remains non-montgomery. So removing >> references to IntegerPolynomialP256 in ECOperations should be possible and >> cleaner. Removing IntegerPolynomialP256 from MontgomeryIntegerPolynomialP256 >> is harder (fromMontgomery() uses IntegerPolynomialP256) but perhaps also >> worth some thought.. >> >> I tend to like `ECOperationsFuzzTest.java` and would prefer to keep it, but >> it could also be chucked up as part of 'scaffolding' and removed in name of >> code quality? > > I wouldn't rip out the old implementation. I have been wondering if we > should make the older implementation available, maybe by security property. > I was looking at the static Maps at the top of `ECOperatio... @ascarpino Fixed as suggested... actually.. that was _waaay_ easier then I thought it would be (I saw singleton and assumed private constructor.. nope, ECOperations() is public, no reflection required!! Ended up with cleaner implementation _and_ cleaner tests! Thanks!) ------------- PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2057895950
