Generalize and improve the HTTP client by DDvO · Pull Request #10667 · openssl/openssl

DDvO · 2019-12-20T15:39:11Z

This PR solves/implements #10271 as discussed there by
generalizing the HTTP client so far implemented mostly in crypto/ocsp/ocsp_ht.c.

The new HTTP client has become an independent libcrpyto module in crypto/http/ and

can handle any types of requests and responses (both ASN.1-encoded and plain)
does not include potentially busy loops when waiting for responses but
makes use of a new timeout mechanism integrated with socket-based BIO
supports the use of HTTP proxies and TLS, including HTTPS over proxies, via a callback
supports HTTP redirection via codes 301 and 302 for GET requests
returns more useful diagnostics in various error situations

I had to adapt - and was able to simplify and improve - hitherto uses of HTTP in
crypto/ocsp/, crypto/x509/x_all.c, apps/lib/apps.c, and apps/{ocsp,s_client,s_server}.c.
The new features will be used heavily by chunk 10 of the CMP contribution.
The various documentation, header, and meta files are updated accordingly.

DDvO · 2019-12-20T16:00:36Z

I've aimed at not changing the existing OCSP-centric HTTP API, which was a challenge in itself.
I think/hope to have managed to keep the API of those functions documented so far,
yet I took the liberty to adapt the types of those functions that have been undocumented.
See the respective comments towards the end of include/openssl/http.h for details.

crypto/err/openssl.txt

kroeckx · 2019-12-22T21:52:08Z

- supports the use of HTTP proxies and TLS, including HTTPS over proxies

Does that mean libcrypto now depends on libssl? Does that work on all platforms?

FdaSilvaYY

Quick review, impressive work, IMHO. 👍

crypto/http/http_client.c

crypto/http/http_err.c

doc/man3/BIO_socket_wait.pod

doc/man3/OCSP_sendreq_new.pod

doc/man3/OSSL_HTTP_transfer.pod

doc/man3/OSSL_CMP_CTX_new.pod

DDvO · 2019-12-23T07:44:14Z

supports the use of HTTP proxies and TLS, including HTTPS over proxies
Does that mean libcrypto now depends on libssl?

No (and if it did, this would mean a mutual dependency).
I solved this challenge via the callback OSSL_HTTP_bio_cb_t bio_update_fn.
See the new doc (https://github.com/mpeylo/cmpossl/blob/http_client_generalized/doc/man3/OSSL_HTTP_transfer.pod) for details.
An instance of the callback is defined, e.g., in apps/lib/apps.c, where libssl is available.

Does that work on all platforms?

I believe so (but haven't tried this out).
The adapted/extended code does not make (new) use of platform-specific facilities.

DDvO · 2019-12-23T07:47:27Z

Quick review, impressive work, IMHO. +1

Thanks!

This PR is the result of a really large amount of work that stretched over 2+ years.

DDvO · 2019-12-23T07:58:31Z

BTW, part of this work dates back to an issue I brought up in late Aug 2017:
https://mta.openssl.org/pipermail/openssl-dev/2017-August/009629.html

DDvO · 2019-12-23T08:22:00Z

Does that mean libcrypto now depends on libssl?

As a side note: this touches a long-standing structural deficiency in the OpenSSL libs.
The crypto lib includes various modules that do not really belong there but
should be part of a more application-level library that should be above libssl.

See also #4992 and my original report and solution suggestion dating back to May 2016:
https://mta.openssl.org/pipermail/openssl-dev/2017-September/009712.html

This PR does not attempt to solve that fundamental issue but needs to work around it.

DDvO · 2020-01-08T07:06:02Z

Thanks for the initial comments on this generalized HTTP implementation given more than two weeks ago, which I answered.

Ping - can someone please do a formal review?

DDvO · 2020-01-15T17:18:02Z

@levitte, in early November you had given some initial remarks on a preview of this PR and showed interest in having a closer look at this code: #10271 (comment)
When do you think you can do a review?

apps/lib/apps.c

crypto/bio/b_sock.c

doc/man3/OCSP_sendreq_new.pod

doc/man3/OSSL_HTTP_transfer.pod

include/openssl/http.h

util/missingcrypto.txt

DDvO · 2020-01-16T17:16:11Z

@mattcaswell, very pleased to receive your comments!
I've addressed all of them.

crypto/http/http_client.c

DDvO · 2020-01-21T14:05:01Z

I've meanwhile rebased this PR and Travis has recovered from an intermittent timeout issue that occurred yesterday, so it's is up and running again.

DDvO · 2020-01-21T14:05:08Z

@mattcaswell, when can you check if the changes I did on your request are acceptable for you,
and do you plan to provide further comments for this PR?

mattcaswell

A couple of questions and a minor nit. Otherwise this looks good.

doc/man3/OSSL_HTTP_transfer.pod

crypto/http/http_client.c

doc/man3/X509_load_http.pod

DDvO · 2020-01-23T11:49:06Z

A couple of questions and a minor nit. Otherwise this looks good.

@mattcaswell, pleased to hear!
I've fulfilled also the new change request(s).

DDvO · 2020-02-11T10:40:40Z

I'd also have a few additions / corrections to two README files there.

mattcaswell · 2020-02-11T10:44:08Z

Can I do a PR to [email protected]:tools.git instead?

Yes, that's the preferred way to do things.

And how is the review process for adaptations to the tools repo?

At the moment its an OMC owned repo, so reviews/pushes can only be done by OMC members.

mattcaswell · 2020-02-11T10:45:18Z

At the moment its an OMC owned repo, so reviews/pushes can only be done by OMC members.

BTW, I see no reason why tools like ghmerge and addrev shouldn't be owned by OTC rather than OMC...but at the moment they're not.

paulidale · 2020-02-11T10:45:19Z

Anyone can make a pull request against the tools repository.
The review process requires two OMC approvers currently.

paulidale · 2020-02-11T10:45:46Z

BTW, I see no reason why tools like ghmerge and addrev shouldn't be owned by OTC rather than OMC...but at the moment they're not.

Agreed.

DDvO · 2020-02-11T12:10:30Z

I've meanwhile added two respective PRs:

mattcaswell · 2020-02-11T14:00:05Z

Oops. Looks like this broke Travis:

https://travis-ci.org/openssl/openssl/jobs/648882003

This is actually the new travis job introduced by #9982. Since that job went into master slightly after this PR went in, but this PR was committed after the last travis run for #9982 was performed this problem didn't show up in either PRs.

DDvO · 2020-02-11T14:15:00Z

I'm trying to reproduce and fix this...

DDvO · 2020-02-11T14:33:48Z

I found this happens due to configuration options no-cmp and no-ocsp.
The former was trivial to fix, now I'm looking for a good solution for the latter...

DDvO · 2020-02-11T15:22:17Z

Luckily, also the fix for no-ocsp was not difficult.

DDvO · 2020-02-12T12:39:46Z

Fixup PR #11058 has been merged.

…since openssl#10667

…since #10667 Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #11273)

DDvO changed the title ~~Generalize the HTTP client~~ Generalize and extend the HTTP client Dec 20, 2019

DDvO changed the title ~~Generalize and extend the HTTP client~~ Generalize and improve the HTTP client Dec 20, 2019

DDvO force-pushed the http_client_generalized branch from 96f66fa to cc8e700 Compare December 21, 2019 20:03

FdaSilvaYY reviewed Dec 22, 2019

View reviewed changes

crypto/err/openssl.txt Outdated Show resolved Hide resolved

FdaSilvaYY reviewed Dec 22, 2019

View reviewed changes

DDvO force-pushed the http_client_generalized branch from 4de53ed to 81153fa Compare December 23, 2019 07:02

DDvO force-pushed the http_client_generalized branch from 7a70ce6 to abd6ffa Compare January 15, 2020 17:13

mattcaswell requested changes Jan 16, 2020

View reviewed changes

FdaSilvaYY reviewed Jan 19, 2020

View reviewed changes

crypto/http/http_client.c Outdated Show resolved Hide resolved

crypto/http/http_client.c Outdated Show resolved Hide resolved

crypto/http/http_client.c Outdated Show resolved Hide resolved

DDvO force-pushed the http_client_generalized branch 2 times, most recently from 6d71759 to a333fdf Compare January 20, 2020 16:08

mattcaswell requested changes Jan 21, 2020

View reviewed changes

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

crypto/http/http_client.c Outdated Show resolved Hide resolved

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

DDvO mentioned this pull request Feb 11, 2020

OSCP HTTP client and other apps could benefit from improvements in cmp_http.c #10271

Closed

DDvO deleted the http_client_generalized branch February 11, 2020 12:27

mattcaswell mentioned this pull request Feb 11, 2020

X509_PUBKEY_set(): Fix memory leak #11038

Merged

DDvO mentioned this pull request Feb 11, 2020

Fix builds with new HTTP client configured with no-ocsp and no-cmp #11058

Closed

This was referenced Feb 17, 2020

chunk 7 of CMP contribution to OpenSSL #10620

Merged

Don't exclude quite so much in a no-sock build, avoiding tight busy loops #11134

Closed

DDvO added a commit to siemens/openssl that referenced this pull request Mar 23, 2020

update comment on crls_http_cb() as it does support non-blocking I/O …

5303a7d

…since openssl#10667

DDvO mentioned this pull request Mar 30, 2020

Chunk 10 of CMP contribution to OpenSSL: CMP HTTP client and related tests #11404

Closed

2 tasks

openssl-machine pushed a commit that referenced this pull request Apr 20, 2020

Update comment on crls_http_cb() as it does support non-blocking I/O …

0aa87e8

…since #10667 Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #11273)

DDvO mentioned this pull request Apr 22, 2020

add info on the CMP implementation and HTTP client to NEWS and CHANGES #11608

Closed

1 task

DDvO mentioned this pull request May 9, 2020

Generalize HTTP server code from apps/ocsp.c to apps/lib/http_server.c #11736

Closed

DDvO mentioned this pull request Jun 16, 2020

Should the libcrypto HTTP client be disableable at Configure time? #11991

Open

mattcaswell mentioned this pull request Jun 23, 2020

Several OCSP_REQ_CTX-related functions impossible to use from C++ in 3.0.0 #12234

Closed

DDvO mentioned this pull request Dec 13, 2020

remove OSSL_HTTP ->OCSP renaming #13671

Closed

mattcaswell mentioned this pull request Jan 15, 2021

OCSP_REQ_CTX API is quite broken going from 1.0.2 to 3.0.0 #13873

Closed

This was referenced May 12, 2021

ocsp proxy support feature request #6965

Closed

Add explicit HTTP(S) proxy support to ocsp and s_server apps #15245

Closed

Uh oh!

Comments

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

kroeckx commented Dec 22, 2019 via email

Uh oh!

FdaSilvaYY left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

DDvO commented Dec 23, 2019

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

DDvO commented Jan 8, 2020

Uh oh!

DDvO commented Jan 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Jan 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Jan 21, 2020

Uh oh!

DDvO commented Jan 21, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Jan 23, 2020

Uh oh!

DDvO commented Feb 11, 2020

Uh oh!

mattcaswell commented Feb 11, 2020

Uh oh!

mattcaswell commented Feb 11, 2020

Uh oh!

paulidale commented Feb 11, 2020

Uh oh!

paulidale commented Feb 11, 2020

Uh oh!

DDvO commented Feb 11, 2020

Uh oh!

mattcaswell commented Feb 11, 2020

Uh oh!

DDvO commented Feb 11, 2020

Uh oh!

DDvO commented Dec 20, 2019 •

edited

Loading

DDvO commented Dec 20, 2019 •

edited

Loading

DDvO commented Dec 23, 2019 •

edited

Loading

DDvO commented Dec 23, 2019 •

edited

Loading

DDvO commented Dec 23, 2019 •

edited

Loading

DDvO commented Jan 21, 2020 •

edited

Loading