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

crypto/cmp: fix CertReqId to use in p10cr transactions #20298

Closed
wants to merge 2 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Feb 15, 2023

A colleague noticed that for p10cr transactions both the CMP client does not use the CertReqId specified in RFC 4210.
This is fixed here, extending also the mock server in order to check for the right value
moving the check for the right value from the mock server to the core server implementation and extending it as needed.

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

@DDvO DDvO 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 triaged: bug The issue/pr is/fixes a bug 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 15, 2023
crypto/cmp/cmp_client.c Show resolved Hide resolved
apps/lib/cmp_mock_srv.c Outdated Show resolved Hide resolved
@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Mar 16, 2023
@DDvO DDvO added this to the 3.2.0 milestone Apr 14, 2023
@DDvO
Copy link
Contributor Author

DDvO commented Apr 14, 2023

Rebased to fix merge conflict.

Ping @openssl/committers for 2nd review.

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm tom-cosgrove-arm added approval: otc review pending This pull request needs review by an OTC member and removed approval: review pending This pull request needs review by a committer labels Apr 14, 2023
@DDvO
Copy link
Contributor Author

DDvO commented Apr 14, 2023

@paulidale could you please reconfirm?

@tom-cosgrove-arm
Copy link
Contributor

@paulidale Can you (or another @openss/otc) do an incremental review for the last commit?

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Apr 16, 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 Apr 17, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

DDvO added a commit to siemens/openssl that referenced this pull request Apr 18, 2023
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20298)
DDvO added a commit to siemens/openssl that referenced this pull request Apr 18, 2023
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20298)

(cherry picked from commit 25b18e6)
DDvO added a commit to siemens/openssl that referenced this pull request Apr 18, 2023
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20298)

(cherry picked from commit 25b18e6)
@DDvO
Copy link
Contributor Author

DDvO commented Apr 18, 2023

Merged to master, 3.0, and 3.1 - thanks @paulidale and @tom-cosgrove-arm for the reviews.

@DDvO DDvO closed this Apr 18, 2023
DDvO added a commit to mpeylo/cmpossl that referenced this pull request Jan 16, 2024
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20298)
DDvO added a commit to mpeylo/cmpossl that referenced this pull request Jan 17, 2024
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20298)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 branch: 3.1 Merge to openssl-3.1 tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants