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 two CMP bugs and doc #18929

Closed
wants to merge 2 commits into from
Closed

Fix two CMP bugs and doc #18929

wants to merge 2 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Aug 1, 2022

  • fix crash in check_transactionID_or_nonce() on actual being NULL
  • correct handling of fallback subject in OSSL_CMP_CTX_setup_CRM() and its doc

@DDvO DDvO added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Merge to openssl-3.0 branch labels Aug 1, 2022
tmshort
tmshort reviewed Aug 3, 2022
View reviewed changes
crypto/cmp/cmp_msg.c
Comment on lines +256 to +257
static const X509_NAME *determine_subj(OSSL_CMP_CTX *ctx, int for_KUR,
const X509_NAME *ref_subj)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change the order of the arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the order because I realized that the new order is more logical:
The outcome of the local helper function determine_subj()

  • primarily depends on ctx,
  • then possibly on int for_KUR,
  • and then, in certain circumstances dependent on the other two params, it depends on ref_subj.

@DDvO
Copy link
Contributor Author

DDvO commented Aug 15, 2022

Can we please continue with this PR?

@DDvO
Copy link
Contributor Author

DDvO commented Aug 23, 2022

Asking @openssl/committers for 2nd review.

@mattcaswell mattcaswell 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 Aug 23, 2022
@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 Aug 24, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Aug 24, 2022
@DDvO
CMP: fix crash in check_transactionID_or_nonce() on 'actual' being NULL
aeadd29 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #18929)
openssl-machine pushed a commit that referenced this pull request Aug 24, 2022
@DDvO
CMP: correct handling of fallback subject in OSSL_CMP_CTX_setup_CRM()…
7af110f 
… and its doc

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #18929)
@DDvO
Copy link
Contributor Author

DDvO commented Aug 24, 2022

Merged to master and (only the applicable commit) to 3.0 - thanks @t8m and @mattcaswell

@DDvO DDvO closed this Aug 24, 2022
openssl-machine pushed a commit that referenced this pull request Aug 24, 2022
@DDvO
CMP: correct handling of fallback subject in OSSL_CMP_CTX_setup_CRM()…
784126f 
… and its doc

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #18929)

(cherry picked from commit 7af110f)
@DDvO DDvO mentioned this pull request Sep 7, 2022
Closed
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@DDvO @sftcd
CMP: fix crash in check_transactionID_or_nonce() on 'actual' being NULL
26aefbc 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18929)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@DDvO @sftcd
CMP: correct handling of fallback subject in OSSL_CMP_CTX_setup_CRM()…
0ba80f8 
… and its doc

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18929)
DDvO added a commit to mpeylo/cmpossl that referenced this pull request Dec 15, 2022
@DDvO
CMP: correct handling of fallback subject in OSSL_CMP_CTX_setup_CRM()…
55e9982 
… and its doc

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18929)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@DDvO @beldmit
CMP: fix crash in check_transactionID_or_nonce() on 'actual' being NULL
dd2e486 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18929)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@DDvO @beldmit
CMP: correct handling of fallback subject in OSSL_CMP_CTX_setup_CRM()…
134b37a 
… and its doc

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18929)
@DDvO DDvO mentioned this pull request Jun 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmshort tmshort tmshort left review comments

@mattcaswell mattcaswell mattcaswell 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: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DDvO @openssl-machine @t8m @mattcaswell @tmshort