Introducing an application-level library for the CLI and OpenSSL-based applications by DDvO · Pull Request #4992 · openssl/openssl

DDvO · 2017-12-28T11:56:56Z

So far, the OpenSSL code has essentially a three-level structure with a stack of two libraries and a command-line application at the top:

apps/openssl
libssl
libcrypto

The apps/ directory contains various generally useful code like handling crypto-related files and messages, general TLS client/server and CA functionality, support for higher-level protocols like S/MIME, CRL fetching, OCSP, and certainly more to come.
While this code serves as a model for using the crypto and ssl libraries and can be used in a limited way by invoking the openssl application binary (CLI), it cannot be re-used directly. Other applications that need similar functionality need to copy or re-implement portions of that code and then maintain it.
On the other hand, the two libraries contain some code that is actually too high-level for them, for instance the minimal HTTP client as part of the crypto library (crypto/ocsp/ocsp_ht.c). It would be very helpful to introduce a further level in the hierarchy containing a more application-oriented library:

apps/openssl and external OpenSSL-based applications
libsecapp <-- new (with tentative name)
libssl
libcrypto

Then all more high-level and application support functionality can go there. This would make much of the generally useful code that so far resides in the apps/ folder directly accessible to other applications at the programming level, i.e., in the form of a library/API, with all the re-usability advantages that this brings. It would also relieve libcrypto from more high-level topics like HTTP.
This library would also form an ideal condensation point for further higher-level uses of crypto and TLS that may in the future get integrated with OpenSSL.

This pull request contains a fully functional proof-of-concept implementation for the above proposal. It introduces a library with the preliminary name 'secapp'.

For now, the library consists of essentially the code that so far as been in apps/apps.c and apps/app_rand.c, moved to the secapp/ directory. (I had to move and partly rename some functions between these two source files and apps/opt.c because the cut between these sources was not entirely clean. Of course, when this PR is accepted, some further cleanup would make sense.) I moved most of apps/apps.h to include/openssl/apps.h, while the strictly internal declarations in it went in a new file apps/apps_locl.h and the ones related to the command-line option functionality went in a new file apps/opt.h.

The high-level build.info mechanism proved very useful for adding the new library, stating its dependencies etc. Yet it turned out that its implementation in Configure and util/mkdef.pl is not complete - I added respective TODOs there.

I made sure that all this (including tests) builds and runs both in a Linux-like environment (Cygwin) and native Windows (Visual C).

Checklist

tests are added or updated

…expressions

…trace option

…to/mem.c)

DDvO · 2017-12-28T11:58:21Z

This pull request is related to suggestions I made in May 2016 and Sep 2017:
https://mta.openssl.org/pipermail/openssl-dev/2017-September/009712.html

DDvO · 2017-12-28T12:01:20Z

This pull request contains in its first five commits various small corrections for shortcomings in the build machinery of OpenSSL I came across while building the addition of the new library, namely:

a bug fixed in util/mkdef.pl, which caused a loop handling declarations like void fn()
a more practically useful trace option for util/mkdef.pl
a new option for mkdef.pl for warning about incomplete handling of #if expressions
a POSIX/ANSI compatibility warning issue with crypto/mem.c and Visual C fixed in e_os.h
a correction of OPENSSL_EXPORT and OPENSSL_EXTERN in e_os2.h for native Windows builds
a bug fixed in a file open error message in apps/progs.pl

…efile.tmpl

….tmpl

levitte · 2017-12-28T13:16:36Z

Configure

            }
            foreach (@{$ordinals{$dest}}) {
-                my %known_ordinals =
+                my %known_ordinals = # TODO this should be configurable in build.info using ORDINALS


Actually, everything about ORDINALS can be removed, both here and anywhere in Configurations.

Do you mean that also util/lib*.num are not needed anymore, not even for building Windows DLLs?
The better if so, because these often cause(d) extra work.
Hope that someone who is very familiar with this topic takes care of removing the unncessary pieces.

Ah, no, not quite so unnecessary, sorry. What's not needed any more is the build.info keyword ORDINALS. It serves no purpose any more, as we've made the generation of corresponding .map / .def / .opt file explicit.

I will, however, clean out the stuff around ORDINAL, thanks.

levitte · 2017-12-28T13:18:11Z

apps/progs.pl


 foreach my $filename (@openssl_source) {
-    open F, $filename or die "Couldn't open $_: $!\n";
+    open F, $filename or die "Couldn't open $filename: $!\n";


levitte · 2017-12-28T13:19:17Z

build.info

+LIBS=libcrypto libssl libsecapp
+ORDINALS[libcrypto]=crypto
+ORDINALS[libssl]=ssl
+ORDINALS[libsecapp]=secapp


The three ORDINALS lines aren't necessary at all

levitte · 2017-12-28T13:38:22Z

Ok... so here's a few thoughts:

The code that you have put in that new library is internal, i.e. we have not at all designed it for public consumption. It's very likely that the names we use clash with something else if combined with "the wrong library".
the API isn't necessarely stable, i.e. we may introduce changes at any time, and that just doesn't roll well with the idea of making this a public library. We don't promise that it will be stable across minor OpenSSL versions or even releases.

It's true that people do copy that code (hey, I've done the same in previous work), but it's entirely at their own risk.

So in the end, while this might seem like a good idea, this places a maintenance burden on @openssl that I'm unsure we're willing to take.

I'm sorry to deliver such a message, 'cause you have obviously put time and effort into this.
I do take a specific interest into the faults that you have found, however. Would you be willing to move those to a separate PR?

DDvO · 2017-12-28T14:06:37Z

@levitte thanks for your swift comments.

I already anticipated the points you made about internal and unstable API components - that's why I wrote above: "Of course, when this PR is accepted, some further cleanup would make sense."

My main points of this PR are that an application-level library would be of great benefit for several reasons and that it is not too hard to implement.

Of course, the actual contents of the library are debatable - putting essentially all of apps.c and app_rand.c is just an example. Yet at least functions like load_cert, load_key, load_crl, ... and all the code supporting CRL loading and OCSP would fit there perfectly.
As mentioned, further more high-level code possibly added in the future, e.g., on certificate management using CMP, would likely better not go into the core crypto lib, but into the new library.

Sure I'll put the various commits with small general improvements in a separate PR.

DDvO · 2017-12-28T15:44:24Z

How if I reduce the initial (API) contents of the new library to:

X509 *load_cert(const char *file, int format, const char *cert_descrip);
X509_CRL *load_crl(const char *infile, int format);
EVP_PKEY *load_key(const char *file, int format, int maybe_stdin,
                   const char *pass, ENGINE *e, const char *key_descrip);
EVP_PKEY *load_pubkey(const char *file, int format, int maybe_stdin,
                      const char *pass, ENGINE *e, const char *key_descrip);
int load_certs(const char *file, STACK_OF(X509) **certs, int format,
               const char *pass, const char *cert_descrip);
int load_crls(const char *file, STACK_OF(X509_CRL) **crls, int format,
              const char *pass, const char *cert_descrip);
int load_pkcs12(BIO *in, const char *desc, pem_password_cb *pem_cb, void *cb_data,
                EVP_PKEY **pkey, X509 **cert, STACK_OF(X509) **ca);

int load_cert_crl_http(const char *url, X509 **pcert, X509_CRL **pcrl);
X509_CRL *load_crl_crldp(STACK_OF(DIST_POINT) *crldp);
STACK_OF(X509_CRL) *crls_http_cb(X509_STORE_CTX *ctx, X509_NAME *nm);

ENGINE *setup_engine(const char *engine, int debug);
void release_engine(ENGINE *e);

Hope this addresses (all) the concerns mentioned above.

mattcaswell · 2017-12-28T15:54:52Z

I am sceptical about the need for a new library (for this purpose). If we think there is code in the apps that is generally useful, I'm not sure why we wouldn't push that code down into libcrypto or libssl as appropriate. If we were to introduce a new library then I think it should be part of a broader review of the structure of all our codebase. There are other things that have been proposed for splitting out into separate libraries, e.g. we could make all algorithms dynamically loadable at runtime. There is also code in libcrypto which isn't really crypto related at all and probably doesn't belong there at all. Such a wholesale review probably shouldn't happen until 1.2.0

… warning

…on using write(); fix '#' indentation

tmshort · 2018-01-02T18:13:12Z

What about libtls from LibreSSL?

levitte · 2018-01-31T12:50:57Z

So @DDvO, I just wanted to let you know that, which I'm sceptical about making a public library, I've recently come to think that a private apps library (that's made out of all the apps modules that aren't directly CLI stuff) wouldn't be such a bad thing, for practical purposes. So I want to thank you for the inspiration.

DDvO · 2018-02-03T12:24:56Z

@mattcaswell and @levitte, very pleased that you confirmed that having such a lib would have practical advantages. and that you put this as a 1.2.0 milestone.

OpenSSL itself will benefit from this even without the need to sanitize and document the interface of such a library. Yet if this extra effort on the API is spent, also external users of OpenSSL would benefit. This would bring a considerable usability boost because OpenSSL-based developments would not need to deal with various awkward and error-prone details, for instance, on file handling, key management, certificate checking using CRLs and OCSP, and management of TLS connections.

FdaSilvaYY · 2018-02-09T15:45:47Z

This PR needs a rebase.
A smart move would be to split out the opt.h refactoring as as 1st step to this PR.

mattcaswell · 2018-02-09T15:50:52Z

This PR needs a rebase.

This PR is very unlikely to be merged as is, so a rebase would be pointless.

DDvO · 2018-02-09T16:06:01Z

Indeed, no need to rebase since this was essentially a proof-of-concept.

levitte · 2018-02-09T16:50:28Z

I suggest it get closed, even.

DDvO · 2018-02-10T14:07:34Z

If it gets closed I suspect that the milestone tag for 1.2.0 will get neglected.
So in order to keep this PR as a reminder, I'd suggest keeping it open, or maybe it can be transformed into an issue?

DDvO · 2019-06-25T13:53:07Z

Shall we convert this into an issue?

DDvO · 2020-11-18T11:19:49Z

Closing this in favor of the new issue #13440,
which updates the suggestions made and discussed in this (heavily outdated) PR.

DDvO added 6 commits December 27, 2017 14:43

correct file open error message in apps/progs.pl

6e430bc

correct OPENSSL_EXPORT & OPENSSL_EXTERN for native Windows builds

866230d

mkdef.pl: improve matching of 'defined (' and warning on simplifying …

107f153

…expressions

mkdef.pl: bugfix infinite loop, e.g., on 'int fn()'; add non-verbose …

ad3c45e

…trace option

Visual Studio C: replace deprecated 'write' by '_write' (used in cryp…

19e0464

…to/mem.c)

add new application-level library named 'secapp', used by OpenSSL CLI

8c8ab8c

DDvO added 3 commits December 28, 2017 13:14

complete renaming: opt_getprog() -> app_getprog() in apps/rehash.c

94c7ec9

add build activitiy for new libsecapp also in Configurations/unix-Mak…

b120f13

…efile.tmpl

corrected syntax error due to comment in Configurations/unix-Makefile…

7f46acc

….tmpl

levitte reviewed Dec 28, 2017

View reviewed changes

DDvO mentioned this pull request Dec 28, 2017

Various build improvements, e.g., on mkdef.pl and progs.pl and Windows compatibility #4994

Closed

DDvO added 2 commits December 29, 2017 16:51

mkdef.pl: rename 'warn_simp' option to 'verbose', use it for one more…

4f9b6ef

… warning

revert '#define write' in e_os.h; add '# ifndef WIN32' in mem.c secti…

ddc6e99

…on using write(); fix '#' indentation

mattcaswell added this to the 1.2.0 milestone Jan 24, 2018

richsalz modified the milestones: 1.2.0, Post 1.1.0 May 6, 2018

richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

richsalz modified the milestones: Post 1.1.0, Assessed May 6, 2018

mspncp removed the after 1.1.1 label Oct 25, 2019

DDvO mentioned this pull request Dec 23, 2019

Generalize and improve the HTTP client #10667

Merged

DDvO mentioned this pull request Nov 18, 2020

Improve overall OpenSSL library structure #13440

Open

DDvO closed this Nov 18, 2020

DDvO mentioned this pull request Jul 6, 2022

Move protocol implementations such as CMP outside libcrypto #16358

Open

Uh oh!

Comments

Conversation

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

Uh oh!

Checklist

Uh oh!

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

Uh oh!

Uh oh!

DDvO commented Dec 28, 2017

Uh oh!

levitte Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

DDvO Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

levitte Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

levitte Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

levitte Dec 28, 2017

Choose a reason for hiding this comment

Uh oh!

levitte commented Dec 28, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mattcaswell commented Dec 28, 2017

Uh oh!

tmshort commented Jan 2, 2018

Uh oh!

levitte commented Jan 31, 2018

Uh oh!

DDvO commented Feb 3, 2018

Uh oh!

FdaSilvaYY commented Feb 9, 2018

Uh oh!

mattcaswell commented Feb 9, 2018

Uh oh!

DDvO commented Feb 9, 2018

Uh oh!

levitte commented Feb 9, 2018

Uh oh!

DDvO commented Feb 10, 2018

Uh oh!

DDvO commented Jun 25, 2019

Uh oh!

DDvO commented Nov 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DDvO commented Dec 28, 2017 •

edited

Loading

DDvO commented Dec 28, 2017 •

edited

Loading

DDvO commented Dec 28, 2017 •

edited

Loading

DDvO commented Dec 28, 2017 •

edited

Loading