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

Run ssl_test_old in fips #11534

Closed
wants to merge 4 commits into from
Closed

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Apr 13, 2020

In a similar way to #11511 and #11508, we run ssl_test_old twice: once with a non-default library context with the default provider loaded into it, and once with a non-default library context with the FIPS provider loaded into it. In both cases we load the "null" provider into the default context to make sure we don't accidentally pick up algorithms from there.

These tests will fail since they require all the key gen PRs to be merged first as well as #11494 and #11507 (and I have not included them here). However, aside from the dependencies this should be fairly complete and can be reviewed.

@mattcaswell mattcaswell added this to In progress in 3.0 New Core + FIPS via automation Apr 13, 2020
@levitte
Copy link
Member

@levitte levitte commented Apr 16, 2020

You might want to edit the subject, as #11328, #11303, #11332, and #11494 are now merged

test/recipes/80-test_ssl_old.t Outdated Show resolved Hide resolved
@mattcaswell mattcaswell changed the title [WIP, pending on #11328, #11303, #11332, #11371, #11494, #11507]: Run ssl_test_old in fips [WIP, pending on #11371, #11507]: Run ssl_test_old in fips Apr 16, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Apr 16, 2020

You might want to edit the subject, as #11328, #11303, #11332, and #11494 are now merged

Done

test/recipes/80-test_ssl_old.t Outdated
'-provider_name', 'fips', '-mac_name', 'HMAC',
'-macopt', 'digest:SHA256', '-macopt', 'hexkey:00',
'-section_name', 'fips_sect'])),
"fipsinstall");

This comment has been minimized.

@levitte

levitte Apr 16, 2020
Member

Considering how this is copied all over the place, we might want to consider making this a separate script.

This comment has been minimized.

@mattcaswell

mattcaswell Apr 16, 2020
Author Member

Yes...and a separate "make test" dependency so that we don't have to do this everywhere? Not this PR though I think.

This comment has been minimized.

@levitte

levitte Apr 16, 2020
Member

Oh! That too

This comment has been minimized.

@levitte

levitte Apr 16, 2020
Member

Yeah, I'm getting to a point where I'll do this, in a separate PR

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Apr 16, 2020
Copy link
Member

@levitte levitte left a comment

Provided the CIs agree

@levitte levitte dismissed their stale review Apr 16, 2020

WIP... making my approval a bit premature

3.0 New Core + FIPS automation moved this from Reviewer approved to Needs review Apr 16, 2020
@levitte levitte changed the title [WIP, pending on #11371, #11507]: Run ssl_test_old in fips [WIP, pending on #11371]: Run ssl_test_old in fips Apr 16, 2020
@mattcaswell mattcaswell changed the title [WIP, pending on #11371]: Run ssl_test_old in fips [Pending on #11508]: Run ssl_test_old in fips Apr 17, 2020
@mattcaswell mattcaswell force-pushed the mattcaswell:ssltest_old branch Apr 17, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Apr 17, 2020

Rebased now that #11371 has been merged. I have had to include the same libssl fixup commit that I've added to #11508 here, in order to get the tests to pass.

Please don't review the libssl changes in this PR. Please provide any review comments on that aspect in #11508 instead.

I've taken this out of WIP, although it cannot now be pushed until #11508 goes in.

@slontis
Copy link
Contributor

@slontis slontis commented Apr 18, 2020

Fixed the fips disabled config.

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Apr 18, 2020

Ping - this needs review (and for the record I'm fine with @slontis's update to this PR).

3.0 New Core + FIPS automation moved this from Needs review to Reviewer approved Apr 19, 2020
@mattcaswell mattcaswell changed the title [Pending on #11508]: Run ssl_test_old in fips Run ssl_test_old in fips Apr 19, 2020
@mattcaswell mattcaswell force-pushed the mattcaswell:ssltest_old branch to 2c0337c Apr 19, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Apr 19, 2020

Rebased now that #11508 has gone in. No other changes were made.

3.0 New Core + FIPS automation moved this from Reviewer approved to Needs review Apr 19, 2020
test/recipes/80-test_ssl_old.t Show resolved Hide resolved
test/recipes/80-test_ssl_old.t Show resolved Hide resolved
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Apr 20, 2020

@levitte - I would prefer to push this as is, and modify the fipsinstall stuff with a follow on PR since this PR is now otherwise ready-to-merge.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Apr 20, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@slontis
Copy link
Contributor

@slontis slontis commented Apr 20, 2020

Considering there seemed to be some problem in travis when the fipsinstall stuff was done - that is probably a good idea i.e- it was merged with an error - @paulidale looked at this today..

@levitte
Copy link
Member

@levitte levitte commented Apr 20, 2020

with a follow on PR

Okie

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Apr 20, 2020

Pushed. Thanks!

openssl-machine pushed a commit that referenced this pull request Apr 20, 2020
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #11534)
openssl-machine pushed a commit that referenced this pull request Apr 20, 2020
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #11534)
3.0 New Core + FIPS automation moved this from Needs review to Done Apr 20, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Apr 20, 2020

I would prefer to push this as is, and modify the fipsinstall stuff with a follow on PR since this PR is now otherwise ready-to-merge.

#11580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants