Skip to content

Comments

Merge some cnf#11347

Closed
richsalz wants to merge 3 commits intoopenssl:masterfrom
richsalz:merge-some-cnf
Closed

Merge some cnf#11347
richsalz wants to merge 3 commits intoopenssl:masterfrom
richsalz:merge-some-cnf

Conversation

@richsalz
Copy link
Contributor

@richsalz richsalz commented Mar 17, 2020

This is based on #11338. This merges a few configuration files into one, and uses flags to identify the right section (hence the dependence on 11338). It also removes a whole bunch of outdated comments. I am going through all the test config (er, .cnf :) files in order to make it easier to run the testsuite with just one provider, for example. This PR is a pre-requisite to that testing.

@richsalz richsalz mentioned this pull request Mar 20, 2020
Closed
@richsalz richsalz mentioned this pull request Mar 28, 2020
Closed
2 tasks
@richsalz
Copy link
Contributor Author

richsalz commented Apr 24, 2020

Rebased to fix a conflict in test/recipes/80-test_ca.t

This also cleans up comments and syntax in several config files, leveraging the doc updates that have already gone in.

@richsalz
Copy link
Contributor Author

richsalz commented Apr 24, 2020

I am currently having a problem. Any clues?

Request is in newreq.pem, private key is in newkey.pem
../../util/wrap.pl /usr/bin/perl ../../apps/CA.pl -newreq -extra-req '-section userreq' => 0
ok 2 - creating certificate request
====
../../util/wrap.pl ../../apps/openssl ca -rand_serial -inform DER -config "../../apps/openssl.cnf" -policy policy_anything -out newcert.pem -infiles newreq.pem 
Using configuration from ../../apps/openssl.cnf
Unable to load certificate request
80:07:44:AE:7B:7F:00:00:error:asn1 encoding routines:asn1_check_tlen:wrong tag:crypto/asn1/tasn_dec.c:1135:
80:07:44:AE:7B:7F:00:00:error:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error:crypto/asn1/tasn_dec.c:293:Type=X509_REQ
==> 256
====
ok 3 - signing certificate request
====
../../util/wrap.pl ../../apps/openssl verify -CAfile ./demoCA/cacert.pem newcert.pem 
Can't open newcert.pem for reading, No such file or directory
80:47:0D:11:B5:7F:00:00:error:system library:BIO_new_file:No such file or directory:crypto/bio/bss_file.c:69:calling fopen(newcert.pem, r)
80:47:0D:11:B5:7F:00:00:error:BIO routines:BIO_new_file:no such file:crypto/bio/bss_file.c:77:
Unable to load certificate file
==> 512

@richsalz
Copy link
Contributor Author

richsalz commented Apr 25, 2020

I figured it out. I had dropped an "-outform DER" when I modified openssl command args. In doing so, I noticed that quotes around $cnf were missing in the SM2 case, so I fixed two variable assignments to do the right thing.

Please review this.

t8m
t8m reviewed Apr 27, 2020
View reviewed changes
t8m
t8m reviewed Apr 28, 2020
View reviewed changes
@richsalz
Copy link
Contributor Author

richsalz commented Apr 28, 2020

Fixed all the space-after-comma, etc., issues. Just resolved the comments, to avoid email deluge. Fixed other things as well. Fixup commit pushed. Thanks for the careful read!

t8m
t8m approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Good cleanup work!

@t8m t8m added approval: review pending This pull request needs review by a committer and removed approval: otc review pending labels Apr 28, 2020
@richsalz
Copy link
Contributor Author

richsalz commented May 19, 2020

Rebased and squashed a fixup commit. @t8m needs a re-review, and @openssl-otc needs a review.

richsalz added 2 commits May 19, 2020 17:57
@richsalz
Cleanup cert config files for tests
734f24b 
Merge test/P[12]ss.cnf into one config file
Merge CAss.cnf and Uss.cnf into ca-and-certs.cnf
Remove Netscape cert extensions, add keyUsage comment from some cnf files
@richsalz
Update manpage to fix examples, other minor tweaks
92c765f
t8m
t8m approved these changes May 20, 2020
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Approved again.

@t8m
Copy link
Member

t8m commented May 20, 2020

@openssl/committers Ping for the second review.

mspncp
mspncp approved these changes May 26, 2020
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

LGTM. Just one typo. Please don't force-push, only add a fixup commit. Then no reapproval is required.

@t8m
Copy link
Member

t8m commented Jun 2, 2020

Still approved after the typo fix.

@t8m t8m 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 Jun 2, 2020
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Jun 3, 2020
@openssl-machine
Copy link
Collaborator

openssl-machine commented Jun 3, 2020

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jun 3, 2020
openssl-machine pushed a commit that referenced this pull request Jun 3, 2020
@richsalz @t8m
Cleanup cert config files for tests
4e6e57c 
Merge test/P[12]ss.cnf into one config file
Merge CAss.cnf and Uss.cnf into ca-and-certs.cnf
Remove Netscape cert extensions, add keyUsage comment from some cnf files

Reviewed-by: Matthias St. Pierre <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #11347)
openssl-machine pushed a commit that referenced this pull request Jun 3, 2020
@richsalz @t8m
Update manpage to fix examples, other minor tweaks
2b584ff 
Reviewed-by: Matthias St. Pierre <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #11347)
@t8m
Copy link
Member

t8m commented Jun 3, 2020

Merged to master. Thank you, Rich, for the PR and patience with the reviews.

@t8m t8m closed this Jun 3, 2020
@richsalz
Copy link
Contributor Author

richsalz commented Jun 3, 2020

Thank you, @t8m for your diligence. You're often the only one and I appreciate it, as do others I am sure.

@richsalz richsalz deleted the merge-some-cnf branch June 3, 2020 13:13
@DDvO
Copy link
Contributor

DDvO commented Jun 6, 2020

@richsalz, looks like this can be merged.

@DDvO
Copy link
Contributor

DDvO commented Jun 6, 2020

@richsalz, looks like this can be merged.

Oops, my Firefox tab was partly outdated. So it is already merged :)

@levitte
Copy link
Member

levitte commented Jun 6, 2020

@DDvO, always reload first

@DDvO
Copy link
Contributor

DDvO commented Jun 8, 2020

@DDvO, always reload first

That's understood, but often I need to reload twice to get the current picture.
Which may be an issue not with Firefox itself but with one of its add-ons/extensions
somehow interfering with the non-trivial web technology used by the GitHub pages.

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

+1 more reviewer

@mspncp mspncp mspncp approved these changes

Reviewers whose approvals may not affect merge requirements

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@richsalz @t8m @openssl-machine @DDvO @levitte @mspncp