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

FFC cleanups #20359

Closed
wants to merge 1 commit into from
Closed

FFC cleanups #20359

wants to merge 1 commit into from

Conversation

slontis
Copy link
Member

@slontis slontis commented Feb 22, 2023

Checklist
  • documentation is added or updated
  • tests are added or updated

@slontis slontis added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: present The PR has suitable tests present labels Feb 22, 2023
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Feb 22, 2023
@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Feb 22, 2023
mattcaswell
mattcaswell requested changes Feb 22, 2023
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

This seems to be a mix of some refactoring, a bug fix, and addition of some tests. Is the refactoring valid for backport to 3.1/3.0?

crypto/dh/dh_backend.c Show resolved Hide resolved
crypto/dsa/dsa_lib.c Show resolved Hide resolved
test/evp_extra_test2.c Show resolved Hide resolved
@slontis
Copy link
Member Author

slontis commented Feb 22, 2023

This seems to be a mix of some refactoring, a bug fix, and addition of some tests. Is the refactoring valid for backport to 3.1/3.0?

It is all related to coverage testing. Whilst doing the tests, I can see lines that will never get hit, and also spotted a bug.
perhaps just the backport of the bug for 3.0 is valid.

@mattcaswell
Copy link
Member

It is all related to coverage testing.

Nice!!! Paying more attention to coverage testing generally is something we should get more into the habit of.

Whilst doing the tests, I can see lines that will never get hit, and also spotted a bug.
perhaps just the backport of the bug for 3.0 is valid.

I suggest splitting the commits so that the refactoring is in a separate commit. I think backporting the bug and the tests that identify the bug into 3.0/3.1 is valid - but we keep the refactoring as master only.

@slontis slontis changed the title FFC fixups and coverage test FFC cleanups Feb 27, 2023
@slontis slontis removed branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Feb 27, 2023
@slontis
Copy link
Member Author

slontis commented Feb 27, 2023

This PR is now just the refactoring part that will go back to master only.
PR #20385 contains the bug fix that will target multiple branches.

@slontis slontis 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 Feb 28, 2023
@openssl-machine openssl-machine 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 Mar 2, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m t8m added the triaged: refactor The issue/pr requests/implements refactoring label Mar 2, 2023
@t8m
Copy link
Member

t8m commented Mar 2, 2023

Please rebase

@t8m
Copy link
Member

t8m commented Mar 20, 2023

Please rebase

ping @slontis

@slontis
FFC cleanups
cf3307d 
Discovered during coverage testing.

Remove unneccesary check when using ossl_dh_get0_params() and
ossl_dsa_get0_params(). These point to addresses and can not fail
for any existing calls.

Make dsa keygen tests only available in the FIPS module - as they are
not used in the default provider.

Change ossl_ffc_set_digest() to return void as it cannot fail.
@slontis
Copy link
Member Author

slontis commented Mar 20, 2023

rebased

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Mar 21, 2023
@openssl-machine openssl-machine 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 Mar 22, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@tmshort
Copy link
Contributor

tmshort commented Mar 31, 2023

@mattcaswell reconfirm?

@t8m
Copy link
Member

t8m commented Apr 3, 2023

Merged to master branch. Thank you for your contribution.

@t8m t8m closed this Apr 3, 2023
openssl-machine pushed a commit that referenced this pull request Apr 3, 2023
@slontis @t8m
FFC cleanups
a76ccb9 
Discovered during coverage testing.

Remove unneccesary check when using ossl_dh_get0_params() and
ossl_dsa_get0_params(). These point to addresses and can not fail
for any existing calls.

Make dsa keygen tests only available in the FIPS module - as they are
not used in the default provider.

Change ossl_ffc_set_digest() to return void as it cannot fail.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #20359)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

@mattcaswell mattcaswell Awaiting requested review from mattcaswell

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 severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@slontis @mattcaswell @openssl-machine @t8m @tmshort @paulidale