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

Jump to bottom

chunk 6 of CMP contribution to OpenSSL #10297

Closed
wants to merge 18 commits into from
Closed

chunk 6 of CMP contribution to OpenSSL #10297

wants to merge 18 commits into from

Conversation

Akretsch
Copy link
Contributor

@Akretsch Akretsch commented Oct 30, 2019 •
edited

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.

6th chunk: CMP messages
in crypto/cmp/cmp_protect.c, crypto/cmp/cmp_msg.c and related files

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

@Akretsch
Copy link
Contributor Author

@mattcaswell, I created this PR starting from the last version of mpeylo#198 plus minor fixes related to formatting. I would like to ask you for a review.

@Akretsch Akretsch mentioned this pull request Oct 30, 2019
Closed
mattcaswell
mattcaswell requested changes Oct 30, 2019
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

For the record I've already done some review work on this PR while it was in preview elsewhere. I've done another pass now. Comments below.

crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_protect.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_protect.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_protect.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_protect.c Outdated Show resolved Hide resolved
doc/man3/OSSL_CMP_log_open.pod Outdated Show resolved Hide resolved
FdaSilvaYY
FdaSilvaYY reviewed Oct 30, 2019
View reviewed changes
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
test/cmp_protect_test.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
test/cmp_protect_test.c Outdated Show resolved Hide resolved
@Akretsch
Copy link
Contributor Author

Akretsch commented Nov 4, 2019

rebased to openssl/master and fixes related to @mattcaswell and @FdaSilvaYY review comments added

@Akretsch
Copy link
Contributor Author

Akretsch commented Nov 5, 2019

All review comments from @mattcaswell and @FdaSilvaYY should be answered now.

@Akretsch
Copy link
Contributor Author

Akretsch commented Nov 7, 2019

more formatting nits fixed, rebased to openssl/master

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Nov 7, 2019
@Akretsch Akretsch mentioned this pull request Nov 8, 2019
Closed
2 tasks
@Akretsch
Copy link
Contributor Author

@bernd-edlinger : Again we need a 2nd reviewer for a chunk of the CMP contribution. Could you please have a look to this PR?

@Akretsch
Copy link
Contributor Author

rebased to openssl/master and review comment from @bernd-edlinger fixed

@DDvO
Copy link
Contributor

DDvO commented Nov 14, 2019

The AppVeyor hick-up currently reported is unrelated to this PR.

@Akretsch Akretsch force-pushed the cmpossl_incremental6 branch 2 times, most recently from 0933ded to e57b123 Compare November 14, 2019 14:19
@Akretsch
Copy link
Contributor Author

review comment form @bernd-edlinger fixed, rebased to openssl/master

@Akretsch
Copy link
Contributor Author

The failed Travis CI build isn't related to us.

FdaSilvaYY
FdaSilvaYY reviewed Nov 21, 2019
View reviewed changes
Copy link
Contributor

@FdaSilvaYY FdaSilvaYY left a comment

Choose a reason for hiding this comment

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

I just making some comments.
Feel free to treat them.

crypto/cmp/cmp_err.c Show resolved Hide resolved
crypto/cmp/cmp_local.h Show resolved Hide resolved
crypto/cmp/cmp_msg.c Show resolved Hide resolved
crypto/cmp/cmp_msg.c Show resolved Hide resolved
crypto/cmp/cmp_msg.c Show resolved Hide resolved
crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved
crypto/cmp/cmp_status.c Show resolved Hide resolved
crypto/crmf/crmf_pbm.c Outdated Show resolved Hide resolved
crypto/err/openssl.txt Show resolved Hide resolved
test/cmp_hdr_test.c Show resolved Hide resolved
DDvO and others added 16 commits December 5, 2019 13:33
@DDvO @Akretsch
simplify and correct error handling/reporting of ossl_cmp_calc_protec…
20ba36a 
…tion()
@DDvO @Akretsch
simplify and correct error handling in test_X509_STORE()
1c075b9
@DDvO @Akretsch
avoid type cast in cmp_protect_test.c and fix various formatting nits
54b75e4
@Akretsch
improved util/openssl-format-check.pl found few more formatting nits,…
7f092f0 
… fixed
@Akretsch
fix indentation in some preprocessor directives
757eb2c
@Akretsch
drop MSTR workaraound
da90a5a
@Akretsch
fix indetation in cmp_msg.c
12825da
@Akretsch
check return code in test
f762647
@Akretsch
fix formatting nit
364f807
@Akretsch
useless init
e32ea29
@Akretsch
fix indentation
bd674ab
@Akretsch
chunk 6 review comments: rename variable and modify error check
3331498
@Akretsch
chunk 6 review comments: rename variable, fix some indentation, fix t…
2aaa55f 
…est assertion
@Akretsch
drop useless pair of parenthesis
8c10d1d
@DDvO @Akretsch
fix two memory leaks regarding X509 extensions in cmp_msg.c
10ea39b
@Akretsch
internalize two macros
f5321b9
@Akretsch
Copy link
Contributor Author

Can this PR be flagged for merging now?

mattcaswell
mattcaswell approved these changes Dec 10, 2019
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Reconfirm

@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 Dec 10, 2019
@HBrock
Copy link

HBrock commented Dec 12, 2019

@mattcaswell, can this PR be merged now?

@mattcaswell mattcaswell 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 Dec 12, 2019
@mattcaswell
Copy link
Member

@mattcaswell, can this PR be merged now?

Yes. We leave a 24 hour grace period from "approval: done" stage to "approval: ready to merge" stage. But that has now passed, so I will merge this shortly.

openssl-machine pushed a commit that referenced this pull request Dec 12, 2019
@DDvO @mattcaswell
chunk 6 of CMP contribution to OpenSSL
3dbc515 
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10297)
@mattcaswell
Copy link
Member

Pushed to master. Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DDvO DDvO DDvO left review comments

@FdaSilvaYY FdaSilvaYY FdaSilvaYY left review comments

@mattcaswell mattcaswell mattcaswell approved these changes

@bernd-edlinger bernd-edlinger bernd-edlinger approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Akretsch @DDvO @HBrock @mattcaswell @FdaSilvaYY @bernd-edlinger