On Wed, 10 Apr 2024 18:02:38 GMT, Volodymyr Paprotski <d...@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. 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 `ECOperations`, `forParameters`, and the constructors where it checks if the `montgomeryOps` was null or set. It would be nice if we could have one set of `fields` Maps by putting the montgomery entry into the `fields` to replace it. I think that should work because `IntegerMontgomeryFieldModuloP` extends `IntegerFieldModuloP`. `instanceof` or other `montgomeryOps` checks would still need to exist because not all the `fields` support mongomery, and the older implementation would still be accessible for your fuzz tester. At least that is my theory. > > 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.. It would be nice to remove the nesting and it would be nice to be a singleton. Maybe some combination of what I mentioned above chance can help that. I haven't fully thought this out either. ------------- PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2050148475
