Make EC code available from within the FIPS provider by mattcaswell · Pull Request #9380 · openssl/openssl

mattcaswell · 2019-07-15T15:03:50Z

This is built on top of #9111 so until that is merged this one will remain in WIP. Only the last 3 commits are relevant to this PR.

levitte

I'm not savvy enough re EC to give a functional comment... but code looks clean, so if CIs agree (and as long as @davidmakepeace's code is merged first), I'm fine with this.

You might want to ask someone with better EC knowledge to cast an eye on this as well.

mattcaswell · 2019-07-15T16:26:51Z

Fixup commit pushed to address a travis failure.

levitte · 2019-07-15T18:12:07Z

Looks like that wasn't the only failure.

crypto/ec/fips-dso-ecp_nistz256.o: In function `ecp_nistz256_get_affine':
/home/travis/.ccache/tmp/ecp_nistz2.stdout.travis-20190703065617.20133.pwp246.i:(.text+0xb80): undefined reference to `ecp_nistz256_from_mont'
/home/travis/.ccache/tmp/ecp_nistz2.stdout.travis-20190703065617.20133.pwp246.i:(.text+0xbec): undefined reference to `ecp_nistz256_from_mont'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Makefile:13275: recipe for target 'providers/fips.so' failed
make[1]: *** [providers/fips.so] Error 1
make[1]: Leaving directory '/home/travis/build/openssl/openssl'
Makefile:196: recipe for target 'tests' failed
make: *** [tests] Error 2

It might be because -ansi is used...

paulidale · 2019-07-16T02:15:08Z

#9111 is merged.

mattcaswell · 2019-07-16T10:11:19Z

Rebased this now that #9111 is merged. I've not looked at the travis failure yet.

mattcaswell · 2019-07-16T11:00:58Z

I can't recreate it locally, but I think I figured out the travis failure. Fixup commit pushed.

romen

I did just a quick code review, without running any test, and so far it looks good to me (I will try to find more time to review properly).

crypto/ec/ec_curve.c

doc/man3/EC_GROUP_new.pod

mattcaswell · 2019-07-16T13:11:03Z

Fixup commit pushed addressing the documentation feedback above. My earlier fixup does seem to have resolved the travis error.

mattcaswell · 2019-07-16T13:11:29Z

Now out of WIP.

mattcaswell · 2019-07-24T10:41:44Z

Rebased to resolve conflicts with master.

@levitte, @romen are there any further comments on this PR?

mattcaswell · 2019-07-26T11:07:38Z

Ping @levitte, @romen...any further comments?

If not @levitte - can you reconfirm?

levitte · 2019-07-27T18:15:37Z

include/openssl/ec.h

 const char *EC_curve_nid2nist(int nid);
 int EC_curve_nist2nid(const char *name);
-int EC_GROUP_check_named_curve(const EC_GROUP *group, int nist_only);
+int EC_GROUP_check_named_curve(const EC_GROUP *group, int nist_only,


You seem to add new functions with the _ex suffix, which is commendable considering we want to avoid API breaks,... except here. Any reason why?

I believe it is because EC_GROUP_check_named_curve is new functionality only existing in master

Yes - exactly.

romen · 2019-07-27T18:34:45Z

Ping @levitte, @romen...any further comments?

Doing some testing, but so far everything seems fine!

romen · 2019-07-27T18:47:08Z

@mattcaswell what's the plan about crypto/ec/ecp_nistp224.c, crypto/ec/ecp_nistp256.c and crypto/ec/ecp_nistp521.c in relation to FIPS_MODE?

Currently this wound not link with enable-ec_nistp_64_gcc_128 because of references to BN_CTX_new()

mattcaswell · 2019-07-28T12:25:17Z

Currently this wound not link with enable-ec_nistp_64_gcc_128 because of references to BN_CTX_new()

Oh - oops. Thanks for the hint. I forgot about those files. I've fixed that how and rebased due to a conflict with master. Please take another look. The ectest fails with enable-ec_nistp_64_gcc_128 but that seems to be due to #9251 rather than this PR.

Edited by @romen to correctly mention issue 9251

romen · 2019-07-28T13:31:08Z

I will have a look, we might also want to rebase this on #9474 after it's merged

Test that EC code works properly in the FIPS provider

Document the new EC functions that are OPENSSL_CTX aware.

mattcaswell · 2019-08-05T16:46:59Z

Rebased to resolve a conflict with master.

Ping?

romen

LGTM

mattcaswell · 2019-08-06T10:24:31Z

Pushed. Thanks!

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #9380)

Test that EC code works properly in the FIPS provider Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #9380)

Document the new EC functions that are OPENSSL_CTX aware. Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #9380)

mattcaswell added the branch: master Applies to master branch label Jul 15, 2019

levitte approved these changes Jul 15, 2019

View reviewed changes

mattcaswell force-pushed the ec-in-fips branch from 61c76f1 to 85ca883 Compare July 16, 2019 10:08

romen reviewed Jul 16, 2019

View reviewed changes

crypto/ec/ec_curve.c Outdated Show resolved Hide resolved

doc/man3/EC_GROUP_new.pod Outdated Show resolved Hide resolved

mattcaswell changed the title ~~WIP: Make EC code available from within the FIPS provider~~ Make EC code available from within the FIPS provider Jul 16, 2019

mattcaswell force-pushed the ec-in-fips branch from 0bbc12e to 34f33ed Compare July 24, 2019 10:41

levitte requested changes Jul 27, 2019

View reviewed changes

mattcaswell force-pushed the ec-in-fips branch from 007cc63 to 11ff4b4 Compare July 28, 2019 12:23

mattcaswell added 3 commits August 5, 2019 17:42

Make the EC code available from inside the FIPS provider

f98349a

Insert a dummy call to EC code in the FIPS provider

a7c6934

Test that EC code works properly in the FIPS provider

Add documentation for new EC functions

624b644

Document the new EC functions that are OPENSSL_CTX aware.

mattcaswell force-pushed the ec-in-fips branch from 11ff4b4 to 624b644 Compare August 5, 2019 16:46

romen approved these changes Aug 6, 2019

View reviewed changes

mattcaswell closed this Aug 6, 2019

levitte pushed a commit that referenced this pull request Aug 6, 2019

Make the EC code available from inside the FIPS provider

a9612d6

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #9380)

levitte pushed a commit that referenced this pull request Aug 6, 2019

Insert a dummy call to EC code in the FIPS provider

04ca002

Test that EC code works properly in the FIPS provider Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #9380)

levitte pushed a commit that referenced this pull request Aug 6, 2019

Add documentation for new EC functions

f585cef

Document the new EC functions that are OPENSSL_CTX aware. Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #9380)

Uh oh!

Conversation

mattcaswell commented Jul 15, 2019

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Jul 15, 2019

Uh oh!

levitte commented Jul 15, 2019

Uh oh!

paulidale commented Jul 16, 2019

Uh oh!

mattcaswell commented Jul 16, 2019

Uh oh!

mattcaswell commented Jul 16, 2019

Uh oh!

romen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattcaswell commented Jul 16, 2019

Uh oh!

mattcaswell commented Jul 16, 2019

Uh oh!

mattcaswell commented Jul 24, 2019

Uh oh!

mattcaswell commented Jul 26, 2019

Uh oh!

levitte Jul 27, 2019

Choose a reason for hiding this comment

Uh oh!

romen Jul 27, 2019

Choose a reason for hiding this comment

Uh oh!

levitte Jul 27, 2019

Choose a reason for hiding this comment

Uh oh!

mattcaswell Jul 28, 2019

Choose a reason for hiding this comment

Uh oh!

romen commented Jul 27, 2019

Uh oh!

romen commented Jul 27, 2019

Uh oh!

mattcaswell commented Jul 28, 2019 • edited by romen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romen commented Jul 28, 2019

Uh oh!

mattcaswell commented Aug 5, 2019

Uh oh!

romen left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Aug 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mattcaswell commented Jul 28, 2019 •

edited by romen

Loading