Skip to content

CMS_add0_cert: if cert already present, do not throw error but ignore it#19199

Closed
DDvO wants to merge 4 commits intoopenssl:masterfrom
siemens:CMS_add0_crl-cert-present
Closed

CMS_add0_cert: if cert already present, do not throw error but ignore it#19199
DDvO wants to merge 4 commits intoopenssl:masterfrom
siemens:CMS_add0_crl-cert-present

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Sep 12, 2022 •
edited
Loading

It makes little sense to forbid adding a cert that was already present
and just causes trouble in case multiple cert sources are combined.

(Side note: After correcting this, the error reason CMS_R_CERTIFICATE_ALREADY_PRESENT becomes unused. Yet for strict API compatibility I think we cannot remove it.)

Update doc accordingly and remove the wrong statement that a CRL must not be added twice.

On this occasion also add checks on failing cert/CRL up_ref calls and improve coding style.

@DDvO DDvO added 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: feature The issue/pr requests/adds a feature triaged: refactor The issue/pr requests/implements refactoring branch: master Applies to master branch labels Sep 12, 2022
t8m
t8m requested changes Sep 13, 2022
View reviewed changes
@DDvO DDvO requested a review from paulidale September 19, 2022 15:14
@openssl-machine
Copy link
Collaborator

openssl-machine commented Oct 20, 2022

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@openssl-machine
Copy link
Collaborator

openssl-machine commented Nov 20, 2022

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

@DDvO
Copy link
Contributor Author

DDvO commented Dec 19, 2022

Ping for 2nd review

@slontis slontis added the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Dec 19, 2022
@slontis
Copy link
Member

slontis commented Dec 19, 2022

This is a change in behavior, so it probably needs an OTC decision. (There is no OTC meeting for a few weeks).

@openssl-machine
Copy link
Collaborator

openssl-machine commented Jan 19, 2023

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

@t8m t8m added the hold: needs tests The PR needs tests to be added to it label Feb 7, 2023
@t8m
Copy link
Member

t8m commented Feb 7, 2023

OTC: This is OK for master if testcase is added and it has a CHANGES.md entry.

@t8m t8m removed the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Feb 7, 2023
@DDvO DDvO force-pushed the CMS_add0_crl-cert-present branch from f20eaa9 to 4da9967 Compare February 13, 2023 11:57
@DDvO DDvO added tests: present The PR has suitable tests present and removed hold: needs tests The PR needs tests to be added to it labels Feb 13, 2023
@DDvO
Copy link
Contributor Author

DDvO commented Feb 13, 2023

OTC: This is OK for master if testcase is added and it has a CHANGES.md entry.

I've added the requested test case and CHANGES.md entry
and on this occasion further improved the respective documentation.

Looking again at the code I found a potential crash.
So in a separate commit I fixed CMS_add1_crl() to prevent double free on failure of CMS_add0_crl().

@DDvO DDvO mentioned this pull request Feb 13, 2023
Closed
@DDvO DDvO requested review from hlandau and slontis February 21, 2023 06:18
@paulidale paulidale 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 23, 2023
@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 Feb 24, 2023
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
@DDvO
CMS_add1_crl(): prevent double free on failure of CMS_add0_crl()
6f9e531 
Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from #19199)
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
@DDvO
CMS_add0_cert: if cert already present, do not throw error but ignore it
65def9d 
Also add checks on failing cert/CRL up_ref calls; improve coding style.

Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from #19199)
@DDvO
Copy link
Contributor Author

DDvO commented Feb 24, 2023

Merged to master - thanks @t8m and @paulidale for the reviews.

@DDvO DDvO closed this Feb 24, 2023
@adalinesimonian
Copy link

adalinesimonian commented Apr 14, 2023 •
edited
Loading

Is there any intent to backport this to 1.1.1 or accept a PR doing so? The old behaviour has been causing problems downstream. In my case specifically, Synology MailPlus fails to sign emails using S/MIME certs issued with a duplicate certificate entry by the CA, such as Sectigo. 1.1.1 is still relied on by a number of hardware vendors, and this is such a small change that to my uninitiated eyes it seems like a good candidate for a backport.

Edit: I see that 1.1.1 is going to be EOL very soon. So likely, the answer is no. :)

@t8m
Copy link
Member

t8m commented Apr 17, 2023

The answer is no. And this is also not present in 3.0 and 3.1 branches either, because strictly speaking this is not a bug fix and it could break some (fairly special though) use cases.

@DDvO
Copy link
Contributor Author

DDvO commented Apr 17, 2023

The answer is no. And this is also not present in 3.0 and 3.1 branches either, because strictly speaking this is not a bug fix and it could break some (fairly special though) use cases.

Thanks for pointing out the actual reason.

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

@slontis slontis Awaiting requested review from slontis

@hlandau hlandau Awaiting requested review from hlandau

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug triaged: feature The issue/pr requests/adds a feature triaged: refactor The issue/pr requests/implements refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@DDvO @openssl-machine @slontis @t8m @adalinesimonian @paulidale