`cmp_vfy.c`: fix major duplications and minor mistakes in diagnostic output on failure validating CMP messages by DDvO · Pull Request #28986 · openssl/openssl

DDvO · 2025-10-23T19:31:59Z

prevent needless and confusing duplication of diagnostic output on failure validating CMP messages
small fixes on content and layout of diagnostics on failure validating signature-based message protection

Copilot

Pull Request Overview

This PR fixes diagnostic output issues in CMP message validation, specifically eliminating duplicate error messages and improving error reporting clarity. The changes focus on refining how validation failures are logged and reported to users.

Key changes:

Restructures error reporting in signature validation to prevent duplicate diagnostics
Adjusts log severity levels for more appropriate classification of validation issues
Improves error message handling flow in certificate verification

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

openssl-machine · 2025-11-23T00:10:35Z

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

…e validating signature-based protection of CMP messages

…utput on failure validating CMP messages

DDvO · 2025-12-22T18:31:28Z

Rebased to fix merge conflict after clang-format

openssl-machine · 2026-01-22T00:11:06Z

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

openssl-machine · 2026-03-07T00:10:44Z

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

openssl-machine · 2026-04-10T00:11:03Z

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

openssl-machine · 2026-05-11T00:11:00Z

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

DDvO · 2026-06-08T16:22:25Z

@openssl/committers can we please get this done soon?
It is a pretty small PR improving just diagnostic output (so nothing critical),
so a low-hanging fruit.

mbroz

LGTM

openssl-machine · 2026-06-09T18:01:02Z

This pull request is ready to merge

esyr

LGTM.

esyr · 2026-06-10T06:13:48Z

+    if (res)
        goto end;
-    }
    /* failed finding a sender cert that verifies the message signature */


Would be nice to leave an empty line before comment and not after.

esyr · 2026-06-10T06:15:21Z

-    if (res)
-        ERR_add_error_mem_bio("\n", bio);
+    if (res) {
+        ERR_add_error_txt(NULL, "\n");


I would print a newline to bio instead, but that's a personal preference.

esyr · 2026-06-10T06:15:35Z

@@ -56,8 +56,10 @@ static int verify_signature(const OSSL_CMP_CTX *cmp_ctx,
 sig_err:


Commit message could've been more descriptive.

…e validating signature-based protection of CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:38:59 2026 (Merged from #28986)

…utput on failure validating CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:39:00 2026 (Merged from #28986)

…e validating signature-based protection of CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:38:59 2026 (Merged from #28986) (cherry picked from commit 0a6b666)

…utput on failure validating CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:39:00 2026 (Merged from #28986) (cherry picked from commit b6eb715)

…e validating signature-based protection of CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:38:59 2026 (Merged from #28986) (cherry picked from commit 0a6b666)

…utput on failure validating CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:39:00 2026 (Merged from #28986) (cherry picked from commit b6eb715)

DDvO · 2026-06-10T06:40:48Z

Thanks @n13l, @mbroz, and @esyr for your reviews and approvals.
@esyr since there were already two approvals and the PR was already waiting long,
I did not handle the nits you reported yesterday. Yet in future I'll make sure to give more descriptive commit messages.

DDvO · 2026-06-10T06:40:54Z

Merged to master, 4.0, 3.6, and 3.5.

…e validating signature-based protection of CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:38:59 2026 (Merged from #28986) (cherry picked from commit 0a6b666)

…utput on failure validating CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:39:00 2026 (Merged from #28986) (cherry picked from commit b6eb715)

esyr · 2026-06-10T07:43:21Z

Yet in future I'll make sure to give more descriptive commit messages.

No problem, those are nits, after all. You can adjust commit messages when applying: ghmerge does interactive rebase and you can use reword action for commits when needed.

…e validating signature-based protection of CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:38:59 2026 (Merged from openssl#28986)

…utput on failure validating CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:39:00 2026 (Merged from openssl#28986)

…e validating signature-based protection of CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:38:59 2026 (Merged from openssl#28986)

…utput on failure validating CMP messages Reviewed-by: Daniel Kubec <[email protected]> Reviewed-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> MergeDate: Wed Jun 10 06:39:00 2026 (Merged from openssl#28986)

DDvO requested a review from Copilot October 23, 2025 20:14

Copilot AI reviewed Oct 23, 2025

View reviewed changes

Comment thread crypto/cmp/cmp_vfy.c

Comment thread crypto/cmp/cmp_vfy.c

DDvO added 2 commits December 22, 2025 19:29

cmp_vfy.c: small fixes on content and layout of diagnostics on failur…

4925375

…e validating signature-based protection of CMP messages

cmp_vfy.c: prevent needless and confusing duplication of diagnostic o…

115df45

…utput on failure validating CMP messages

DDvO force-pushed the fix_cmp_vfy_diagnostics branch from 213417a to 115df45 Compare December 22, 2025 18:31

loci-dev mentioned this pull request Dec 22, 2025

UPSTREAM PR #28986: cmp_vfy.c: fix major duplications and minor mistakes in diagnostic output on failure validating CMP messages auroralabs-loci/openssl#259

Open

DDvO removed the approval: review pending This pull request needs review by a committer label Jan 28, 2026

DDvO added the approval: review pending This pull request needs review by a committer label Feb 4, 2026

esyr added the branch: 4.0 Applies to openssl-4.0 label Mar 10, 2026

n13l approved these changes Jun 8, 2026

View reviewed changes

mbroz approved these changes Jun 8, 2026

View reviewed changes

esyr approved these changes Jun 10, 2026

View reviewed changes

DDvO closed this Jun 10, 2026

esyr mentioned this pull request Sep 9, 2026

[master] CHANGES.md, NEWS.md: update for 4.1.0-alpha1 #32758

Closed

		@@ -56,8 +56,10 @@ static int verify_signature(const OSSL_CMP_CTX *cmp_ctx,
		sig_err:

Uh oh!

Uh oh!

Conversation

DDvO commented Oct 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

openssl-machine commented Nov 23, 2025

Uh oh!

DDvO commented Dec 22, 2025

Uh oh!

openssl-machine commented Jan 22, 2026

Uh oh!

openssl-machine commented Mar 7, 2026

Uh oh!

openssl-machine commented Apr 10, 2026

Uh oh!

openssl-machine commented May 11, 2026

Uh oh!

DDvO commented Jun 8, 2026

Uh oh!

mbroz left a comment

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented Jun 9, 2026

Uh oh!

esyr left a comment

Choose a reason for hiding this comment

Uh oh!

esyr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

esyr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

esyr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

DDvO commented Jun 10, 2026

Uh oh!

DDvO commented Jun 10, 2026

Uh oh!

esyr commented Jun 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants