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

Convert dhparam so that it no longer needs low level APIs #13231

Closed
wants to merge 6 commits into from

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Oct 23, 2020

This converts the dhparam app so that it no longer needs to use any low level APIs, as well as adding a test to check that the behaviour is consistent with historic behaviour.

It does not address the issue that stdin does not currently work with the app (as described in issue #13185).

It is marked as pending because it requires the commits from #13228. Those commits are not included in this PR to ease review, but this means that the new test will fail.

I've also added an OMC hold on this because, in accordance with the recent OTC decision the "-C" option has been removed. However that decision needs OMC ratification.

Note that I developed and tested the new test in the 1.1.1 branch first. However I had to change it to get it to work in master because of subtle differences in the way the "-text" output works between the two versions (as per issue #13220).

@mattcaswell mattcaswell changed the title [WIP, pending #13228]: Convert dhparam so that it no longer needs low level APIs Convert dhparam so that it no longer needs low level APIs Nov 18, 2020
mattcaswell added 2 commits Oct 14, 2020
Previously changes left some variables behind that were no longer needed.
We now remove them.
@mattcaswell mattcaswell force-pushed the mattcaswell:convert-dhparam branch to c20c942 Nov 18, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 18, 2020

Now that #13228 has gone in I have taken this out of WIP and rebased it to resolve conflicts with master.

I have also removed the commits that dealt with removing the "-C" option since that was already handled by #13384. The OMC hold that was on this PR was related to that part of the change so I've also removed the hold.

This is now ready for review. Please take a look.

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 19, 2020

Ping?

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 19, 2020

Hmm. Travis failure - fixup pushed. I definitely already did that same fixup yesterday - but somehow I lost it.

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Nov 19, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 20, 2020

Fixup pushed to address a travis failure. @paulidale - please can you reconfirm?

Copy link
Contributor

@paulidale paulidale left a comment

Still good.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Nov 22, 2020

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Nov 23, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13231)
openssl-machine pushed a commit that referenced this pull request Nov 23, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13231)
openssl-machine pushed a commit that referenced this pull request Nov 23, 2020
Previously changes left some variables behind that were no longer needed.
We now remove them.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13231)
openssl-machine pushed a commit that referenced this pull request Nov 23, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13231)
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 23, 2020

Pushed. Thanks.

3.0 New Core + FIPS automation moved this from Reviewer approved to Done Nov 23, 2020
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