search

mpeylo / cmpossl

An OpenSSL-based implementation of the Certificate Management Protocol (CMP), defined in IETF RFCs 4210, 4211, and 6712. It is being extended according to the emerging RFCs 'CMP Updates' (CMPv3), 'CMP Algorithms', and 'Lightweight CMP Profile'.
https://github.com/mpeylo/cmpossl/wiki
Other
35 stars 13 forks source link

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

Closed Akretsch closed 4 years ago

Akretsch commented 4 years ago

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.

8th chunk: CMP generic server and mock server for testing client in crypto/cmp/cmp_server.c and related files. The testing client and related tests will be contributed in chunk 9.

Checklist
Akretsch commented 4 years ago

@mattcaswell We prepared a preview PR for CMP chunk 8. It shows the deltas to a frozen state of https://github.com/openssl/openssl/pull/10620

Akretsch commented 4 years ago

@mattcaswell Could you please have a first view at this preliminary PR?

DDvO commented 4 years ago

I am struggling to understand why this code is in libcrypto. Is having a mock server considered a generally useful thing to do? I can see why we might want it in the apps - but why would other applications need this? To me this code seems better placed in the apps directory.

A really good and fundamental question here. We recently discussed it internally, too. The idea was - and still is - that although currently the functionality is sufficient only for a test/mock server, it could be improved and extended towards productive code (building blocks) of a real server.

One may wonder if it could make sense in terms of development complexity to implement a full-fledged CMP server in C (or C++) on top of OpenSSL, and it turns out that some folks are actually considering this. If this wasn't the case I'd fully agree that all this code should be moved out of crypto/.

One could try separating code that clearly would make sense for a real server from code that is just good for the mock server, but given the rather small code base it would likely make more trouble (e.g., of defining a stable CMP server sub-API with callbacks for each type of requests etc.) than having all that for the time being in one file, cmp_server.c.

We might move the whole file to apps/ and its header decls from cmp.h to apps/include/ but then we could not directly use it for unit tests in test/. We might instead move them to test/ but then we could likely not use it for CLI-based tests.

mattcaswell commented 4 years ago

IMO a mock server does not belong in libcrypto. If at some point in the future it is fleshed out to be more generally applicable then we can move it at that point.

We might move the whole file to apps/ and its header decls from cmp.h to apps/include/ but then we could not directly use it for unit tests in test/.

I think it should be moved to the apps directory. There's already various instance of code in the apps directory which is used by the test code (for example opt.c in apps/lib is also used by the test code).

DDvO commented 4 years ago

@mattcaswell, all right, @Akretsch will move the server code to a apps/cmp_server.c. This may imply the need to export a couple of further CMP functions, while the tentative OSSL_CMP_SRV_ functions of course won't become part of the libcrypto API. We'll also look for a way to make use of that in both our API-level unit tests and CLI-based tests.

DDvO commented 4 years ago

In the meantime I'll also do a (formal) review of the mock server code.

Akretsch commented 4 years ago

@mattcaswell We introduced a CMP (mock) server. Is there something else we should do?

DDvO commented 4 years ago

Closing this preview PR since the actual PR is out now in https://github.com/openssl/openssl/pull/11142. All comments received so far on it have been addressed. Thanks @mattcaswell!