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

md2: test doesn't require direct low level access #10821

Closed
wants to merge 1 commit into from
Closed

md2: test doesn't require direct low level access #10821

wants to merge 1 commit into from

Conversation

paulidale
Copy link
Contributor

Low level access isn't required beyond the block size which remains available.

  • tests are added or updated

@paulidale paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Jan 13, 2020
@paulidale paulidale added this to In progress in 3.0 New Core + FIPS via automation Jan 13, 2020
@levitte
Copy link
Member

levitte commented Jan 13, 2020

Do we test md2 with evp_test? In that case, I see no use for this test program and would simply throw it away.

@romen
Copy link
Member

romen commented Jan 13, 2020

Should we though have in place something that is testing that the legacy low level API is still working? (I see this is not the case anymore for this PR, but I am just wondering about the risk of losing coverage on the ancient API)

@paulidale
Copy link
Contributor Author

We don't seem to have any other MD2 tests.

This test is unchanged by the deprecation work -- i.e. the low level APIs were not tested previously.

@levitte
Copy link
Member

levitte commented Jan 13, 2020

My suggestion would be to copy the test vectors to the appropriate evp_test input file, and refractor this one into testing the low-level API. Otherwise, this program is just redundant cruft.

@paulidale
Copy link
Contributor Author

I'm more inclined to copy the test cases to EVP and just remove this one.

@levitte
Copy link
Member

levitte commented Jan 13, 2020

Sure. After all, that should still test the low level stuff, just via EVP

@paulidale
Copy link
Contributor Author

The low level calls will be as well tested as they were :)

I've pushed the changes.

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Jan 13, 2020
@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 13, 2020
@levitte
Copy link
Member

levitte commented Jan 13, 2020

Approval is, of course, as long as CIs agree

@paulidale
The MD2 test uses the EVP APIs not the low level ones.
e53a2bf 
The test can be moved into the EVP tests and the separate executable removed.
@paulidale
Copy link
Contributor Author

CI failure was relevant. Forgot to delete 05-test_md2.t. Scarily, most CIs passed with this omission.

@paulidale
Copy link
Contributor Author

@levitte this will need a new approval -- the only change is the file deletion.

@paulidale paulidale added approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels Jan 13, 2020
@levitte
Copy link
Member

levitte commented Jan 13, 2020

Scarily, most CIs passed with this omission.

MD2 is disabled by default, isn't it? So of course, only the jobs that enable it explicitly will get failures...

@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 13, 2020
openssl-machine pushed a commit that referenced this pull request Jan 14, 2020
@paulidale
The MD2 test uses the EVP APIs not the low level ones.
d3ac2ea 
The test can be moved into the EVP tests and the separate executable removed.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #10821)
@paulidale paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 14, 2020
@paulidale
Copy link
Contributor Author

Merged to master with one end of line space removed.

@paulidale paulidale closed this Jan 14, 2020
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Jan 14, 2020
@paulidale paulidale deleted the dep-md2-fix branch January 14, 2020 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
No open projects
3.0 New Core + FIPS
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@paulidale @levitte @romen