chunk 7 of CMP contribution to OpenSSL by Akretsch · Pull Request #10620 · openssl/openssl

Akretsch · 2019-12-12T17:15:35Z

Certificate Management Protocol (CMP, RFC 4210) extension to OpenSSL
Also includes CRMF (RFC 4211) and HTTP transfer (RFC 6712)

CMP and CRMF API is added to libcrypto, and the "cmp" app to the openssl CLI.
Adds extensive man pages and tests. Integration into build scripts.

7th chunk: CMP protection verification
in crypto/cmp/cmp_vfy.c and related files.

This PR includes also some improvements in code formatting (triggered by #10363) , logging and internal documentation.

Checklist

documentation is added or updated
tests are added or updated

Akretsch · 2019-12-12T17:23:09Z

@mattcaswell, I created this PR starting from the last version of mpeylo#199. If successfully build I would like to ask you for a review.

Akretsch · 2019-12-13T07:49:44Z

The time exceeding travis job is probably not related to us.

Akretsch · 2019-12-13T11:25:01Z

moved some file updates between the commits, the final result is the same.

DDvO · 2019-12-13T19:21:17Z

I've just pushed a new version that completes the cleanup of commits that @Akretsch did 8 hours before.
@mattcaswell, when doing the merge after this PR has been approved, I suggest not squashing these four commits into one:

commit 3c550e0fbcd69810de5cf85d71f29c278859ec90 (HEAD -> cmpossl_incremental7,  origin/cmpossl_fixup6_incremental7)
Author: Dr. David von Oheimb <[email protected]>
Date:   Fri Dec 13 19:45:53 2019 +0100

    chunk 7 of CMP contribution to OpenSSL: CMP message validation and related tests

commit 6d87fc4a1a23014a96364ea6352cf016fbb6967b
Author: Dr. David von Oheimb <[email protected]>
Date:   Fri Dec 13 20:07:08 2019 +0100

    fix various formatting nits in CMP contribution chunks 1-6 found by the new util/check-format.pl
    correct wording in doc, comments, and parameter names: self-signed -> self-issued where appropriate

commit 241a9a67b007124aa79816b25ab3a11049f074c2
Author: Dr. David von Oheimb <[email protected]>
Date:   Fri Dec 13 19:50:20 2019 +0100

    improve CMP logging after changes according to comments on CMP chunk7 preview

commit c5e413ee7c2bc69b44c2e3ca3e1972aaf97cebb9
Author: Dr. David von Oheimb <[email protected]>
Date:   Fri Dec 13 18:54:15 2019 +0100

    add internal doc files actually belonging to CMP contribution chunk 6

DDvO · 2019-12-14T12:05:25Z

There is - like after the push before - one strange Travis CI failure, which I cannot reproduce locally:

80-test_ocsp.t ..................... ok
80-test_pkcs12.t ................... ok

The job exceeded the maximum time limit for jobs, and has been terminated.

DDvO · 2019-12-18T14:25:11Z

For what ever reason, Travis keeps aborting its very last job on this PR at

80-test_ocsp.t ..................... ok
80-test_pkcs12.t ................... ok
The job exceeded the maximum time limit for jobs, and has been terminated.´

Does anyone understand why? This does not seem CMP related.

mattcaswell · 2019-12-18T14:26:11Z

It seems to be a frequent and very annoying problem at the moment. Don't know why - but it's definitely not CMP related.

h-vetinari · 2019-12-18T14:41:19Z

It's been plaguing the CI since 77fedcd, with one of the jobs running into a hard cutoff of 50min by Travis. See #10639 for a fix by @slontis.

DDvO · 2019-12-18T15:07:14Z

Thanks @mattcaswell and @h-vetinari for your (comforting to me) comments!

mattcaswell · 2019-12-19T10:51:50Z

Travis failures seem relevant. Also, already needs a rebase!

mattcaswell

For the record I have already reviewed and provided extensive comments on the preview version of this PR.

I've just taken another pass through and just have a few minor comments.

crypto/cmp/cmp_vfy.c

doc/internal/man3/ossl_cmp_certReq_new.pod

Akretsch · 2019-12-19T13:09:48Z

@mattcaswell I fixed the Travis failure and did a rebase.

crypto/cmp/cmp_msg.c

DDvO · 2019-12-27T11:06:47Z

@mattcaswell I fixed the Travis failure and did a rebase.

@Akretsch, when adding fixes to this PR please always use --fixup to make clear which commit is improved and to make sure that auto-squashing will work fine. I've done this by amending your recent commit (of Dec 19).

crypto/cmp/cmp_local.h

crypto/cmp/cmp_msg.c

util/missingcrypto.txt

crypto/err/err_prn.c

DDvO · 2020-02-13T14:30:17Z

Thanks a lot @bernd-edlinger for very swiftly finishing your review
and the quick re-approval by @mattcaswell!

So looks like I can squash and merge this chunk after the usual one day grace period?

mattcaswell · 2020-02-13T14:31:12Z

So looks like I can squash and merge this chunk after the usual one day grace period?

Looks like it - assuming no last minute problems from the CIs.

DDvO · 2020-02-14T09:12:40Z

AppVeyor is always pretty slow, and this time it took even some 12 hours to complete (after presumably being busy grinding on other PRs), but finally it confirmed that the builds and tests pass.

DDvO · 2020-02-14T09:20:36Z

I'll squash the commits (up to a few ones) already now in preparation of the later merge,
which among others should raise the chances that the merge
will be rewarded by that lovely purple-colored 'merged' icon 💜

DDvO · 2020-02-15T16:35:19Z

Done squashing as far as appropriate (and already adding the reviewer & pr info).
Do I need an explicit 'ready to merge' label before (potentially rebasing again) and pushing?

bernd-edlinger · 2020-02-16T07:51:30Z

We do usually the final squash and rebase without pushing that to the PR again,
but directly to the openssl git server, once the grace period is over, the labels are just
optional, the final decision is human.
Howver now, I have problems to fetch the revision f431d68
from this pr which @mattcaswell originally approved, so I am unable to double-check that the
squashing / re-basing did not introduce incorrectness.

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> (Merged from openssl#10620)

in particular: consolidate documentation of CMP logging and error reporting functions fix compilation problem with clang on some platforms rename OSSL_CMP_log etc. to ossl_cmp_log etc. since these macros are CMP-internal move chopping of trailing separator to ossl_cmp_add_error_txt(), also fix handling of leading separator internalize X509_print_ex_brief() as x509_print_ex_brief() Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> (Merged from openssl#10620)

…he new util/check-format.pl in addition: correct wording in doc, comments, and parameter names: self-signed -> self-issued where appropriate Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> (Merged from openssl#10620)

add CMP message validation and related tests; while doing so: * add ERR_add_error_mem_bio() to crypto/err/err_prn.c * move ossl_cmp_add_error_txt() as ERR_add_error_txt() to crypto/err/err_prn.c * add X509_STORE_CTX_print_verify_cb() to crypto/x509/t_x509.c, adding internally x509_print_ex_brief(), print_certs(), and print_store_certs() * move {ossl_cmp_,}X509_STORE_get1_certs() to crypto/x509/x509_lu.c Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> (Merged from openssl#10620)

DDvO · 2020-02-17T07:00:18Z

We do usually the final squash and rebase without pushing that to the PR again,
but directly to the openssl git server, once the grace period is over,

Yes, but @mattcaswell recently asked me regarding PR #10667 to squash it before it was merged, and this had the nice effect that GitHub has been able to recognize the merge automatically, which is indicated by the purple 'Merged' label. I appreciate this, and others as well, and @beldmit even asked to 'incorporate the brilliant changes he made to the workflow' - see #11038

the labels are just optional, the final decision is human.

Good to know, thanks @bernd-edlinger.

However now, I have problems to fetch the revision f431d68
from this pr which @mattcaswell originally approved,

Yes, this indeed is a drawback. One could still follow up the commits via the link you gave and via its parent link etc, but this would be pretty cumbersome.
A workaround would be to save the original/unsquashed PR branch in a backup branch.

so I am unable to double-check that the squashing / re-basing did not introduce incorrectness.

Before force-pushing I double-checked this using

git diff origin/cmpossl_incremental7

DDvO · 2020-02-17T07:13:38Z

@Akretsch, if you still have the previous state of cmpossl_incremental7 locally, please push it as cmpossl_incremental7-orig such that we have it as backup/reference just in case.

Akretsch · 2020-02-17T07:24:29Z

cmpossl_incremental7-orig pushed

bernd-edlinger · 2020-02-17T07:26:38Z

yes, that would be nice,
please do git reset --hard f431d686479a36131bdbbab23f9399c15d066637 before pushing that
since that was the originally approved version. It should be available in your repo,
but I cannot pull if from github, although I know for sure, that revision is still in their database.

DDvO · 2020-02-17T07:55:28Z

@bernd-edlinger, good idea to reset to the commit in question first.
This did not work out for @Akretsch (since he had fetched last on Feb 3rd), but for me.
I've force-pushed f431d68 to cmpossl_incremental7-orig now.

levitte

I just noticed these two put_error calls, which suggest that ERR_add_error_txt() isn't as generic as the function name suggests and possibly should have stayed in the CMP subsystem...

crypto/err/err_prn.c

hou2gou · 2020-02-19T02:05:00Z

@DDvO Have the CMP feature and CRMF feature been completed? Are the test cases completed? When are these features planned for openssl 3.0.0?

FdaSilvaYY · 2020-02-19T21:16:10Z

@DDvO Have the CMP feature and CRMF feature been completed? Are the test cases completed? When are these features planned for openssl 3.0.0?

@yangyangtiantianlonglong Next chunk is here. A you may find their roadmap in this repo.

DDvO · 2020-02-20T08:37:08Z

@yangyangtiantianlonglong, the state of the CMP contribution is shown and kept up-to-date at https://github.com/mpeylo/cmpossl/wiki#planning-and-current-status-of-the-contribution-chunks.
So you can we plan to include everything in OpenSSL 3.0.
The contribution includes extensive tests.

DDvO · 2020-08-04T11:16:34Z

Where is this callback? I get:
test/cmp_vfy_test.c(72): error C2065: 'OSSL_CMP_print_cert_verify_cb': undeclared identifier

Looks like you had an inconsistent source tree or build.
The callback function is meanwhile called X509_STORE_CTX_print_verify_cb.

Akretsch mentioned this pull request Dec 12, 2019

WIP: CMP implementation, incremental PR chunk 7: CMP protection verification mpeylo/cmpossl#199

Closed

2 tasks

Akretsch force-pushed the cmpossl_incremental7 branch from a7518fc to 8444614 Compare December 13, 2019 11:21

Akretsch mentioned this pull request Dec 13, 2019

WIP: CMP implementation, incremental PR chunk 8: CMP generic and mock server mpeylo/cmpossl#201

Closed

1 task

DDvO force-pushed the cmpossl_incremental7 branch from 8444614 to 3c550e0 Compare December 13, 2019 19:16

DDvO force-pushed the cmpossl_incremental7 branch 2 times, most recently from 145680a to c90f4c9 Compare December 18, 2019 14:22

mattcaswell requested changes Dec 19, 2019

View reviewed changes

crypto/cmp/cmp_vfy.c Outdated Show resolved Hide resolved

crypto/cmp/cmp_vfy.c Outdated Show resolved Hide resolved

doc/internal/man3/ossl_cmp_certReq_new.pod Outdated Show resolved Hide resolved

Akretsch force-pushed the cmpossl_incremental7 branch from c90f4c9 to cb5fade Compare December 19, 2019 12:26

bernd-edlinger reviewed Dec 26, 2019

View reviewed changes

crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved

DDvO force-pushed the cmpossl_incremental7 branch 2 times, most recently from 31e679e to ef36932 Compare December 27, 2019 09:17

bernd-edlinger reviewed Dec 29, 2019

View reviewed changes

crypto/cmp/cmp_local.h Outdated Show resolved Hide resolved

bernd-edlinger reviewed Dec 29, 2019

View reviewed changes

crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved

DDvO force-pushed the cmpossl_incremental7 branch from e3f210f to 12b8616 Compare December 29, 2019 16:13

bernd-edlinger reviewed Jan 3, 2020

View reviewed changes

util/missingcrypto.txt Outdated Show resolved Hide resolved

bernd-edlinger reviewed Jan 3, 2020

View reviewed changes

crypto/err/err_prn.c Outdated Show resolved Hide resolved

DDvO force-pushed the cmpossl_incremental7 branch 2 times, most recently from ce0192a to 49a8b5e Compare January 4, 2020 11:32

mattcaswell added the approval: done This pull request has the required number of approvals label Feb 13, 2020

DDvO force-pushed the cmpossl_incremental7 branch from f431d68 to 1b738b3 Compare February 15, 2020 14:16

DDvO force-pushed the cmpossl_incremental7 branch from 1b738b3 to 437d3f0 Compare February 15, 2020 14:54

DDvO added 4 commits February 17, 2020 07:43

add internal doc files actually belonging to CMP contribution chunk 6

fcc25be

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> (Merged from openssl#10620)

DDvO force-pushed the cmpossl_incremental7 branch from 437d3f0 to 31b28ad Compare February 17, 2020 06:50

openssl-machine merged commit 31b28ad into openssl:master Feb 17, 2020

DDvO mentioned this pull request Feb 17, 2020

X509_PUBKEY_set(): Fix memory leak #11038

Merged

levitte reviewed Feb 17, 2020

View reviewed changes

crypto/err/err_prn.c Show resolved Hide resolved

crypto/err/err_prn.c Show resolved Hide resolved

Uh oh!

Conversation

Akretsch commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Akretsch commented Dec 12, 2019

Uh oh!

Akretsch commented Dec 13, 2019

Uh oh!

Akretsch commented Dec 13, 2019

Uh oh!

DDvO commented Dec 13, 2019

Uh oh!

DDvO commented Dec 14, 2019

Uh oh!

DDvO commented Dec 18, 2019

Uh oh!

mattcaswell commented Dec 18, 2019

Uh oh!

h-vetinari commented Dec 18, 2019

Uh oh!

DDvO commented Dec 18, 2019

Uh oh!

mattcaswell commented Dec 19, 2019

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Akretsch commented Dec 19, 2019

Uh oh!

Uh oh!

DDvO commented Dec 27, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Feb 13, 2020

Uh oh!

mattcaswell commented Feb 13, 2020

Uh oh!

DDvO commented Feb 14, 2020

Uh oh!

DDvO commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bernd-edlinger commented Feb 16, 2020

Uh oh!

DDvO commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Feb 17, 2020

Uh oh!

Akretsch commented Feb 17, 2020

Uh oh!

bernd-edlinger commented Feb 17, 2020

Uh oh!

DDvO commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hou2gou commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FdaSilvaYY commented Feb 19, 2020

Uh oh!

DDvO commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akretsch commented Dec 12, 2019 •

edited

Loading

DDvO commented Feb 14, 2020 •

edited

Loading

DDvO commented Feb 15, 2020 •

edited

Loading

DDvO commented Feb 17, 2020 •

edited

Loading

DDvO commented Feb 17, 2020 •

edited

Loading

hou2gou commented Feb 19, 2020 •

edited

Loading

DDvO commented Feb 20, 2020 •

edited

Loading