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

Fix verify_callback in the openssl s_client/s_server app #18805

Closed
wants to merge 1 commit into from
Closed

Fix verify_callback in the openssl s_client/s_server app #18805

wants to merge 1 commit into from

Conversation

beldmit
Copy link
Member

@beldmit beldmit commented Jul 14, 2022

We need to check that error cert is available before printing its data

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

@beldmit
Fix verify_callback in the openssl s_client/s_server app
9f8af99 
We need to check that error cert is available before printing its data
@beldmit beldmit 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 triaged: bug The issue/pr is/fixes a bug labels Jul 14, 2022
@beldmit
Copy link
Member Author

beldmit commented Jul 14, 2022

Originally reported as https://bugzilla.redhat.com/show_bug.cgi?id=2107208

@t8m t8m added the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Jul 15, 2022
t8m
t8m approved these changes Jul 15, 2022
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

It is in 1.1.1 too. At least to me it seems it could happen there as well as check_policy is the same there and the verify_callback too.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jul 15, 2022
@beldmit
Copy link
Member Author

beldmit commented Jul 15, 2022

Not sure if it will be cherry-picked there but I agree

@beldmit
Copy link
Member Author

beldmit commented Jul 18, 2022

Ping @openssl/committers for 2nd review

@DDvO DDvO 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 Jul 18, 2022
@DDvO
Copy link
Contributor

DDvO commented Jul 18, 2022

BTW, nice to see that for s_client and s_server such useful info is printed on cert verify error.
The verify app and the CMP implementation, any maybe further cert verifiers, do something similar.
Would be good to factor this out in a general function in apps/lib/apps.c or even in the crypto lib.

@openssl-machine
Copy link
Collaborator

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.

@DDvO DDvO 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 Jul 19, 2022
@hlandau
Copy link
Member

hlandau commented Jul 20, 2022

Merged to master, 3.0 and 1.1.1. Thank you.

@hlandau hlandau closed this Jul 20, 2022
openssl-machine pushed a commit that referenced this pull request Jul 20, 2022
@beldmit @hlandau
Fix verify_callback in the openssl s_client/s_server app
fad0f80 
We need to check that error cert is available before printing its data

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #18805)
openssl-machine pushed a commit that referenced this pull request Jul 20, 2022
@beldmit @hlandau
Fix verify_callback in the openssl s_client/s_server app
cc90ba6 
We need to check that error cert is available before printing its data

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #18805)

(cherry picked from commit fad0f80)
openssl-machine pushed a commit that referenced this pull request Jul 20, 2022
@beldmit @hlandau
Fix verify_callback in the openssl s_client/s_server app
86945b1 
We need to check that error cert is available before printing its data

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #18805)

(cherry picked from commit fad0f80)
@beldmit beldmit deleted the fix_s_cb branch July 20, 2022 07:47
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@beldmit @sftcd
Fix verify_callback in the openssl s_client/s_server app
8b08931 
We need to check that error cert is available before printing its data

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#18805)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdukhovni vdukhovni vdukhovni approved these changes

@DDvO DDvO DDvO approved these changes

@t8m t8m t8m 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 branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@beldmit @DDvO @openssl-machine @hlandau @vdukhovni @t8m