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.

Sign up for GitHub

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

Jump to bottom

Make EC code available from within the FIPS provider #9380

Closed
wants to merge 3 commits into from
Closed

Make EC code available from within the FIPS provider #9380

wants to merge 3 commits into from

Conversation

mattcaswell
Copy link
Member

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.

@mattcaswell mattcaswell added the branch: master Merge to master branch label Jul 15, 2019
levitte
levitte approved these changes Jul 15, 2019
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Fixup commit pushed to address a travis failure.

@levitte
Copy link
Member

levitte commented Jul 15, 2019

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
Copy link
Contributor

#9111 is merged.

@mattcaswell
Copy link
Member Author

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

@mattcaswell
Copy link
Member Author

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

romen
romen reviewed Jul 16, 2019
View reviewed changes
Copy link
Member

@romen romen left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
doc/man3/EC_GROUP_new.pod Outdated Show resolved Hide resolved
@mattcaswell
Copy link
Member Author

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

@mattcaswell 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
Copy link
Member Author

Now out of WIP.

@mattcaswell
Copy link
Member Author

Rebased to resolve conflicts with master.

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

@mattcaswell
Copy link
Member Author

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

If not @levitte - can you reconfirm?

levitte
levitte requested changes Jul 27, 2019
View reviewed changes
include/openssl/ec.h
@@ -422,7 +447,8 @@ size_t EC_get_builtin_curves(EC_builtin_curve *r, size_t nitems);

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,
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - exactly.

@romen
Copy link
Member

romen commented Jul 27, 2019

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

Doing some testing, but so far everything seems fine!

@romen
Copy link
Member

romen commented Jul 27, 2019

@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
Copy link
Member Author

mattcaswell commented Jul 28, 2019 •
edited by romen
Loading

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
Copy link
Member

romen commented Jul 28, 2019

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

@mattcaswell
Make the EC code available from inside the FIPS provider
f98349a
@mattcaswell
Insert a dummy call to EC code in the FIPS provider
a7c6934 
Test that EC code works properly in the FIPS provider
@mattcaswell
Add documentation for new EC functions
624b644 
Document the new EC functions that are OPENSSL_CTX aware.
@mattcaswell
Copy link
Member Author

Rebased to resolve a conflict with master.

Ping?

romen
romen approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@romen romen left a comment

Choose a reason for hiding this comment

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

LGTM

@mattcaswell
Copy link
Member Author

Pushed. Thanks!

@mattcaswell mattcaswell closed this Aug 6, 2019
levitte pushed a commit that referenced this pull request Aug 6, 2019
@mattcaswell
Make the EC code available from inside the FIPS provider
a9612d6 
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
(Merged from #9380)
levitte pushed a commit that referenced this pull request Aug 6, 2019
@mattcaswell
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 <nic.tuv@gmail.com>
(Merged from #9380)
levitte pushed a commit that referenced this pull request Aug 6, 2019
@mattcaswell
Add documentation for new EC functions
f585cef 
Document the new EC functions that are OPENSSL_CTX aware.

Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
(Merged from #9380)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte requested changes

@romen romen romen approved these changes

Assignees
No one assigned
Labels
branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mattcaswell @levitte @paulidale @romen