CMP extension to OpenSSL, incremental PR chunk 1: CRMF API by DDvO · Pull Request #7328 · openssl/openssl

DDvO · 2018-09-28T12:13:58Z

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

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

Incremental pull request as suggested by @mattcaswell: #6811 (comment) and recently agreed by email.

Initial chunk consists of the CRMF API
as declared in the header file include/openssl/crmf.h
and its documentation in doc/man3/OSSL_CRMF_*.pod

Checklist

documentation is added or updated
tests are added or updated

DDvO · 2018-09-28T12:19:57Z

The next chunk is going to be the CRMF implementation, after we have received reviewers' feedback on the CRMF API and dealt with it.

DDvO · 2018-09-28T13:15:04Z

In case one wonders: the CI build failures automatically reported below are just due to include/openssl/crmferr.h and the implementations of the functions declared in include/openssl/crmf.h not yet being part of the PR.

mattcaswell

Some initial thoughts only. I've only reviewed the header file so far.

mattcaswell · 2018-09-28T15:30:39Z

include/openssl/crmf.h

+int OSSL_CRMF_MSG_set1_regCtrl_authenticator(OSSL_CRMF_MSG *msg,
+                                             ASN1_UTF8STRING *auth);
+int OSSL_CRMF_MSG_set1_regCtrl_pkiPublicationInfo(OSSL_CRMF_MSG *msg,
+                                                  OSSL_CRMF_PKIPUBLICATIONINFO *pi);


So I can use the OSSL_CRMF_PKIPUBLICATIONINFO object here. I see the DECLARE_ASN1_FUNCTIONS macro above, which is going to give me *_new, *_free functions etc. But how do I populate this with any data? There seem to be no way of setting any values within this structure. The same comment applies to a number of the other objects in this header file, e.g. OSSL_CRMF_PKIARCHIVEOPTIONS, OSSL_CRMF_ENCRYPTEDVALUE.

Good point, I will see that those get created - although there might not be any CA-side implementation that could digest those.

mattcaswell · 2018-09-28T15:31:11Z

include/openssl/crmf.h

+
+/* crmf_lib.c */
+int OSSL_CRMF_MSG_set1_regCtrl_regToken(OSSL_CRMF_MSG *msg,
+                                        ASN1_UTF8STRING *tok);


There is only a setter for this and other fields. Should there be getters as well?

In general yes, yet this implementation focuses on the CMP (and thus CRMF) client side.
The client typically just sets these fields in its requests and does not need to read them out again.

mattcaswell · 2018-09-28T15:31:48Z

include/openssl/crmf.h

@@ -0,0 +1,121 @@
+/*
+ * Copyright 1995-2018 The OpenSSL Project Authors. All Rights Reserved.


This is a new file. Is the claim of copyright back to 1995 justified? Similarly for the Nokia and Siemens copyright claims below.

OpenSSL copyright might be a tricky question: the CMP/CRMF code builds up and loans a lot from existing OpenSSL code, which generally states copyright going back to 1995. It might be of course OK to adjust that to the earliest version of the file, which will usually be 2007?

The dates for the Nokia and Siemens copyright claims are verified; development was started in 2007.

I'm fine with whatever the earliest date is.

mattcaswell · 2018-09-28T15:33:48Z

include/openssl/crmf.h

+int OSSL_CRMF_MSG_set1_regInfo_certReq(OSSL_CRMF_MSG *msg,
+                                       OSSL_CRMF_CERTREQUEST *cr);
+
+int OSSL_CRMF_MSG_set_version2(OSSL_CRMF_MSG *crm);


Why do we need this? RFC 4211 suggests this value SHOULD be omitted.

Maybe if someone wants to see how a server reacts if that is set? SHOULD is not a SHALL?

If it's OpenSSL policy that such options are not there, then it will vanish. But then there might be a question how to set the optional version field? ;-)

Well it's not a policy exactly. But if we're not supposed to set it according to the RFC, and it doesn't give us any benefit in setting it, then I don't really see any point in providing an API to allow people to set it.

it will be gone

mattcaswell · 2018-09-28T15:38:45Z

include/openssl/crmf.h

+
+/* crmf_pbm.c */
+OSSL_CRMF_PBMPARAMETER *OSSL_CRMF_pbmp_new(size_t slen, int owfnid,
+                                           long itercnt, int macnid);


Why long? We generally don't use long in new APIs any more. It's not particularly helpful because it may not be any bigger than int. Either use int (which we require to be at least 4 bytes in size on all our platforms), or use a int64_t if you require a larger type. Same comment applies to other uses of long in this header.

Thank you for pointing that out.

Probably historic reasons: I think when that was written, ASN1_INTEGER_set() and ASN1_INTEGER_get() were the only available options. I replace them with their int64_t counterparts. The changes will appear soon, have already sent them for quality review before merging.

mattcaswell

I've now reviewed the rest of this PR. A few more comments below.

mattcaswell · 2018-10-05T15:51:01Z

doc/man3/OSSL_CRMF_MSG_get_tmpl.pod

+
+=head1 COPYRIGHT
+
+Copyright 2007-2018 The OpenSSL Project Authors. All Rights Reserved.


Just querying this copyright date again since this is a new file.

I'm still somewhat unsure what dates to put for OpenSSL Project Authors copyright. Basically everything is fine for me.

Options are likely:

first version of the file (we can find out from Sourceforge / Github)

first version / copyright of content somehow used or significantly in the file (that might be 2007 or 1995)

official / successful submission to the project (that might be 2018 or realistically 2019)

For Nokia (c) in source files, it will usually start 2007 (2015 for Siemens) as that was when content was created which eventually ended up / directly influenced in most files.

I'm also fine with any of these options.
My suggestion would be to simply put in all files:

Copyright 2007-2018 The OpenSSL Project Authors. All Rights Reserved.

mattcaswell · 2018-10-05T15:52:28Z

doc/man3/OSSL_CRMF_MSG_get_tmpl.pod

+
+OSSL_CRMF_ENCRYPTEDVALUE_get1_encCert() decrypts the certificate in the given
+encryptedValue B<ecert>, using the private key B<pkey>.
+This is needed for the indirect PoP method as in RFC 4210 section 5.2.8.2.


Although it is implied from the get0/get1 name, not everyone is familiar with that particular OpenSSL idiom so we should explicitly state the memory management semantics for each getter/setter. (Comment applies here and in all the other pod files)

I've taken care of this (for all CRMF doc files), also regarding set0 vs. set1.

mattcaswell · 2018-10-05T15:58:52Z

doc/man3/OSSL_CRMF_MSG_set1_regInfo_certReq.pod

+
+=head1 DESCRIPTION
+
+OSSL_CRMF_MSG_set1_regInfo_utf8Pairs() adds the cutf8Pairs regInfo to the given


cutf8Pairs? Typo? RFC4211 says that this can appear multiple times in the regInfo. Is this possible with this API?

Typo, good catch.

Yes, the code allows to set multiple instances of the utf8Pairs control to be set by calling that function multiple times. I realized that this is also possible for certReq, where the RFC states that there can only be one (which makes sense due to its nature- while using non-normative language).

I would describe this in a NOTES section to the man page. I believe that this is sufficient, and it does not require explicit enforcement by the API, as as nobody will ever be able to use this functions without closely investigating in the RFC what they are for in the first place.

=head1 NOTES Calling the functions multiple times adds multiple instances of the control in question to the regInfo structure of the given B<msg>. However RFC RFC 4211 only expects multiple instances of utf8Pairs in one regInfo structure, but not for certReq.

Does this make sense?

mattcaswell · 2018-10-05T16:03:29Z

doc/man3/OSSL_CRMF_pbmp_new.pod

+structure with new a random salt of given length B<saltlen>, OWF (one-way
+function) NID B<owfnid>, iteration count B<itercnt>, and MAC NID B<macnid>.
+
+The iteration count is enforced to be at least 100, as stipulated by RFC 4211.


Drop this sentence. You already said it above.

It will be gone

mattcaswell · 2018-10-05T16:03:57Z

doc/man3/OSSL_CRMF_pbmp_new.pod

+=head1 NOTES
+
+The OWF (one-way function) and for the MAC (message authentication code) may be
+any with an NID defined in B<openssl/objects.h>,


nit: "a NID"

mattcaswell · 2018-10-05T16:04:45Z

doc/man3/OSSL_CRMF_pbmp_new.pod

+ unsiged char *msg = "Hello";
+ unsiged char *sec = "SeCrEt";
+ unsiged char *mac = NULL;
+ unsiged int maclen;


Typo: unsigned

good eyes, thanks

DDvO · 2018-10-12T15:38:37Z

We have meanwhile reacted to all of Matt's review comments and provided an update with our respective fixes.

mattcaswell

Looks ok to me. Needs a second review from @openssl/committers.

I'm not sure how best to handle this. Until we have the implementation half of this, it probably shouldn't really be merged. On the other hand that will complicate the review of the implementation PR because it will have to include all the commits from this PR.

DDvO · 2018-10-15T17:57:43Z

Pleased to hear.
I can squash all hitherto commits into one, will this help?

mattcaswell · 2018-10-15T17:58:54Z

I can squash all hitherto commits into one, will this help?

Yes - lets do that.

DDvO · 2018-10-17T12:07:59Z

Oops, I had overlooked your swift response.
Just did the squashing; in this branch there were anyway just two commits regarding the CRMF API.

mattcaswell · 2018-10-17T12:36:43Z

Still looking for a second reviewer for this. Ping @openssl/committers

bernd-edlinger · 2018-10-17T12:41:09Z

doc/man3/OSSL_CRMF_MSG_set_validity.pod

Aehm, sorry, but "popo" is the german word for butt.
Could we please have another name?

:-) That's funny!

Actually though it is consistently called popo in the RFC!!

Actually though it is consistently called popo in the RFC!!

Actually they don't use it consistently: I counted only 23 POPOs vs 71-23 = 48 POPs.

curl https://www.ietf.org/rfc/rfc4210.txt | tr ' ' '\n' | grep POP | wc -l 71 curl https://www.ietf.org/rfc/rfc4210.txt | tr ' ' '\n' | grep POPO | wc -l 23

So why not use POP instead of POPO?

I think the waned to have a prove that the joke is really funny...

Both Martin and me are native German speakers.
We're aware of this accidental pun and got used to it :)

yeah, as far as the ASN.1 terms are used, its probably okay.
Honestly I had to laugh and could not stop for a while :) when I read the create_popo thing...

bernd-edlinger · 2018-10-17T13:23:18Z

doc/man3/OSSL_CRMF_MSG_set_validity.pod

Oops; will be fixed (as well as RAVerfied -> RAVerified).

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. Incremental pull request based on OpenSSL commit 1362190 of 2018-09-26 1st chunk: CRMF API (include/openssl/crmf.h) and its documentation (reviewed)

DDvO · 2018-10-17T14:08:46Z

I've meanwhile provided the code portion of the CRMF implementation as the second commit.

mattcaswell · 2018-10-17T14:12:16Z

I've meanwhile provided the code portion of the CRMF implementation as the second commit.

Personally I would remove that commit until such time as @bernd-edlinger has finished reviewing what is currently there. And then I would create it as a different PR with both the commits from here and the implementation commits so that we have a clear separation of the review history and we know exactly what is in scope for review at any one time.

DDvO · 2018-10-17T14:32:05Z

All right, I will retract the second commit soon.
(I'm just curious to wait and see if besides the successful Travis CI build also the AppVeyor build will succeed).

DDvO · 2018-10-17T17:22:11Z

Also AppVeyor was fine with the code :-)
As suggested by @mattcaswell, I've just removed the second commit (containing the CRMF code) such that this PR is on the CRMF API only.

bernd-edlinger · 2018-10-17T18:28:09Z

doc/man3/OSSL_CRMF_MSG_set_validity.pod

+
+=head1 RETURN VALUES
+
+All functions returns 1 on success, 0 on error.


return 1 on

bernd-edlinger · 2018-10-17T18:33:05Z

doc/man3/OSSL_CRMF_MSG_set1_regCtrl_regToken.pod

+to B<si>. Consumes the B<spi> pointer.
+
+OSSL_CRMF_MSG_set0_SinglePubInfo() sets given B<method> and PubLoction B<nm> to
+B<spi>.  PubLocation is optional, and therfore B<nm> may be NULL. Consumes the


bernd-edlinger · 2018-10-17T19:07:26Z

include/openssl/crmf.h

+int OSSL_CRMF_MSG_set_validity(OSSL_CRMF_MSG *crm, time_t from, time_t to);
+int OSSL_CRMF_MSG_set_certReqId(OSSL_CRMF_MSG *crm, int rid);
+int OSSL_CRMF_MSG_get_certReqId(OSSL_CRMF_MSG *crm);
+int OSSL_CRMF_ASN1_get_int(int *pr, const ASN1_INTEGER *a);


What does this function?

Ah, this is a new function I forgot to document so far. It's basically like ASN1_INTEGER_get_int64(), actually calling it, but with range checks according its result type int. It is used both in the CRMF and CMP sub-libraries. I'll add the doc.

BTW, this rather general function could also be moved (with adapted name) to crypto/asn1/a_int.c. I'll add a TODO.

bernd-edlinger · 2018-10-17T19:24:05Z

include/openssl/crmf.h

+#  define OSSL_CRMF_POPOPRIVKEY_AGREEMAC             3
+#  define OSSL_CRMF_POPOPRIVKEY_ENCRYPTEDKEY         4
+
+#  define OSSL_CRMF_SUBSEQUENTMESSAGE_ENCRCERT       0


funny, the methods are called like encCert, rfc4210 "CertOrEncCert", rfc 4211 has "encrCert"

…edlinger

…edlinger

DDvO · 2018-11-05T14:34:12Z

@bernd-edlinger, thank you for your review of the CRMF API and doc.
My above commit addressed all your improvement hints. Is this now fine for you or do you see anything else that we should improve before we provide the actual CRMF code?

doc/man3/OSSL_CRMF_MSG_get0_tmpl.pod

bernd-edlinger

LGTM

DDvO · 2018-11-16T12:49:35Z

Nice that we meanwhile have both approvals :)

I've just provided the second chunk of our incremental PR in #7646.

mattcaswell · 2018-11-16T12:52:23Z

I've just applied the "hold" label on this, so that no one attempts to merge it until chunk 2 is also ready.

… purposes

mattcaswell · 2019-03-12T12:31:18Z

Now that #7646 is approved, I pushed this one.

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. Incremental pull request based on OpenSSL commit 1362190 of 2018-09-26 1st chunk: CRMF API (include/openssl/crmf.h) and its documentation (reviewed) Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #7328)

DDvO mentioned this pull request Sep 28, 2018

Certificate Management Protocol (CMP, RFC 4210) extension #6811

Closed

2 tasks

mattcaswell requested changes Sep 28, 2018

View reviewed changes

mattcaswell requested changes Oct 5, 2018

View reviewed changes

mattcaswell approved these changes Oct 15, 2018

View reviewed changes

DDvO force-pushed the cmp_incremental branch from f9e2178 to c4d3896 Compare October 17, 2018 11:56

bernd-edlinger reviewed Oct 17, 2018

View reviewed changes

DDvO force-pushed the cmp_incremental branch from c4d3896 to 65ce4e4 Compare October 17, 2018 13:55

DDvO force-pushed the cmp_incremental branch from 65ce4e4 to dee2280 Compare October 17, 2018 17:20

bernd-edlinger reviewed Oct 17, 2018

View reviewed changes

a couple of minor doc improvements according to API review by @bernd-…

0f95fa0

…edlinger

DDvO force-pushed the cmp_incremental branch from f366a0b to 0f95fa0 Compare October 18, 2018 17:19

bernd-edlinger reviewed Nov 5, 2018

View reviewed changes

doc/man3/OSSL_CRMF_MSG_get0_tmpl.pod Outdated Show resolved Hide resolved

remove OSSL_CRMF_ASN1_get_int() due to review comments

117791b

cleanup of '#include'

0ccaed8

bernd-edlinger approved these changes Nov 14, 2018

View reviewed changes

DDvO mentioned this pull request Nov 16, 2018

CMP extension to OpenSSL, incremental PR chunk 2: CRMF code #7646

Closed

2 tasks

DDvO changed the title ~~CMP (RFC 4210) extension to OpenSSL, incremental PR~~ CMP extension to OpenSSL, incremental PR chunk 1 Nov 16, 2018

mattcaswell added the hold label Nov 16, 2018

DDvO changed the title ~~CMP extension to OpenSSL, incremental PR chunk 1~~ CMP extension to OpenSSL, incremental PR chunk 1: CRMF API Nov 16, 2018

re-add explicit #includes not strictly needed, just for documentation…

a31ef1b

… purposes

mattcaswell added approval: done This pull request has the required number of approvals and removed hold labels Mar 12, 2019

mattcaswell closed this Mar 12, 2019

		@@ -0,0 +1,121 @@
		/*
		* Copyright 1995-2018 The OpenSSL Project Authors. All Rights Reserved.


		=head1 COPYRIGHT

		Copyright 2007-2018 The OpenSSL Project Authors. All Rights Reserved.


		=head1 DESCRIPTION

		OSSL_CRMF_MSG_set1_regInfo_utf8Pairs() adds the cutf8Pairs regInfo to the given


		=head1 RETURN VALUES

		All functions returns 1 on success, 0 on error.

Uh oh!

Conversation

DDvO commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

DDvO commented Sep 28, 2018

Uh oh!

DDvO commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpeylo Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO commented Oct 12, 2018

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented Oct 15, 2018

Uh oh!

DDvO commented Sep 28, 2018 •

edited

Loading

DDvO commented Sep 28, 2018 •

edited

Loading

mattcaswell Sep 28, 2018 •

edited

Loading

mpeylo Oct 2, 2018 •

edited

Loading

mspncp Oct 17, 2018 •

edited

Loading

DDvO Oct 17, 2018 •

edited

Loading