Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ot base implementation from Chou Orlandi with fixes #389

Merged
merged 20 commits into from
Sep 20, 2022

Conversation

Gugi264
Copy link
Contributor

@Gugi264 Gugi264 commented Mar 5, 2022

Closes #388

@codecov
Copy link

codecov bot commented Mar 5, 2022

Codecov Report

Merging #389 (4733c66) into master (16cda54) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 4733c66 differs from pull request most recent head f6d67d9. Consider uploading reports for the commit f6d67d9 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##              master      #389   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity      3618      3640   +22     
===========================================
  Files            408       411    +3     
  Lines          10209     10290   +81     
  Branches         776       780    +4     
===========================================
+ Hits           10209     10290   +81     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../alexandra/fresco/tools/ot/base/BigIntElement.java 100.00% <ø> (ø)
...exandra/fresco/tools/ot/base/BigIntNaorPinkas.java 100.00% <ø> (ø)
...lexandra/fresco/demo/cli/CmdLineProtocolSuite.java 100.00% <100.00%> (ø)
...ra/fresco/tools/ot/base/AbstractChouOrlandiOT.java 100.00% <100.00%> (ø)
...dra/fresco/tools/ot/base/AbstractNaorPinkasOT.java 100.00% <100.00%> (ø)
...xandra/fresco/tools/ot/base/BigIntChouOrlandi.java 100.00% <100.00%> (ø)
.../fresco/tools/ot/base/BouncyCastleChouOrlandi.java 100.00% <100.00%> (ø)
...a/fresco/tools/ot/base/BouncyCastleECCElement.java 100.00% <100.00%> (ø)
...a/fresco/tools/ot/base/BouncyCastleNaorPinkas.java 100.00% <100.00%> (ø)
...dk/alexandra/fresco/lib/fixed/math/Reciprocal.java 100.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16cda54...f6d67d9. Read the comment docs.

@jonas-lj jonas-lj requested a review from jot2re March 8, 2022 08:40
@jot2re
Copy link
Collaborator

jot2re commented Apr 8, 2022

As discussed in the comments on issue #388 I unfortunately have to reject the PR.

@jot2re jot2re closed this Apr 8, 2022
@jot2re jot2re reopened this Jul 7, 2022
@jot2re
Copy link
Collaborator

jot2re commented Jul 8, 2022

After more discussion with Claudio Orlandi, I realised that the amount of changes to make the protocol secure are really minimal. Thus I think it still makes sense to merge it.
I am really sorry for being too quick to reject the PR previously.
I understand that you probably don't have time to implement the suggestions at this point. If not, I will probably just do it myself.

@Gugi264
Copy link
Contributor Author

Gugi264 commented Jul 11, 2022

Hi, what are the necessary changes? If they are really only small I can implement them.

@jot2re
Copy link
Collaborator

jot2re commented Jul 12, 2022

Hi, what are the necessary changes? If they are really only small I can implement them.

In relation to https://eprint.iacr.org/2021/1218 they are minimal. But as mentioned in the comments, there are a few other things that should be added. I think the easiest is if I just do it, since it is fresh in my mind anyway :)

@Gugi264
Copy link
Contributor Author

Gugi264 commented Jul 12, 2022

Ok :)

@jot2re
Copy link
Collaborator

jot2re commented Sep 2, 2022

@quacktiamauct can you implement the changes I have requested?

Copy link
Collaborator

@jot2re jot2re left a comment

Choose a reason for hiding this comment

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

There are still a lot of security issues from the previous review that has not been addressed.
I have fixed some of them and explicitly written down the remaining issues and how to fix them

@quackzar quackzar linked an issue Sep 15, 2022 that may be closed by this pull request
@jot2re jot2re self-requested a review September 19, 2022 12:25
Copy link
Collaborator

@jot2re jot2re left a comment

Choose a reason for hiding this comment

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

Look good. I think the only issue is that there is not full coverage with the current tests. If you could add a couple of tests to ensure this (so the CI/CD won't complain once it gets merged) that would be great

@quackzar quackzar requested a review from jot2re September 20, 2022 07:07
…th the normal NaorPinkas OT where the wrong subgroup is used
@jot2re jot2re merged commit e6993b6 into aicis:master Sep 20, 2022
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.

Fix parameters for Naor Pinkas OT HL17 baseOt implementation
3 participants