HTTP: Implement persistent connections (keep-alive) etc. by DDvO · Pull Request #15053 · openssl/openssl

DDvO · 2021-04-27T22:31:57Z

This PR implements feature request #14004.

The proposed semantics of the new feature implemented here is as follows.
Both at API and at CLI level (for the CMP app only, so far) there is a new parameter/option: keep_alive.

0 means that HTTP connections are not kept open after receiving a response, which is the default behavior for HTTP 1.0.
1 means that persistent connections are requested.
2 means that persistent connections are required, i.e., in case the server does not grant them an error occurs.

For the CMP app the default value is 1, which means preferring to keep the connection open.
For all other internal uses of the HTTP client (fetching an OCSP response, a cert, or a CRL) it does not matter because these operations just take one round trip.

If the client application requested or required a persistent connection and this was granted by the server,
it can keep the OSSL_HTTP_REQ_CTX * as long as it wants to send further requests and OSSL_HTTP_is_alive() returns nonzero,
else it should call OSSL_HTTP_REQ_CTX_free() or OSSL_HTTP_close().
In case the client application keeps the OSSL_HTTP_REQ_CTX * but the connection then dies for any reason at the server side, it will notice this obtaining an I/O error when trying to send the next request .

This feature addition requires extending the HTTP header parsing and rearranging the high-level HTTP client API.
In particular:

Split the monolithic OSSL_HTTP_transfer() into OSSL_HTTP_open(),
OSSL_HTTP_set_request(), a lean OSSL_HTTP_transfer(), and OSSL_HTTP_close().
Split the timeout functionality accordingly and improve default behavior.
Extract part of OSSL_HTTP_REQ_CTX_new() to OSSL_HTTP_REQ_CTX_set_expected().
Extend the local struct ossl_http_req_ctx_st accordingly.

Also extend related diagnostic output for debugging.
Use the new feature for the CMP client,
which requires extending related transaction management of CMP client and test server.

Related further more general improvements (in separate commits) included because they are related:

OSSL_HTTP_get(): Do not close connection if redirect to same server
OSSL_HTTP_REQ_CTX_add1_headers(): Fix use with host == NULL (relative URLs)
OSSL_HTTP_transfer(): Fix error reporting in case rctx->server == NULL.
Also improve doc of OSSL_parse_url() and OSSL_HTTP_parse_url()
Generalize API to arbitrary request and response contents
~~Introducing OSSL_HTTP_i2d() and OSSL_HTTP_d2i() makes the API more clear and modular.~~
Allow streaming of request data (for POST method).
Also clean up OSSL_HTTP_REQ_CTX_nbio() states and make it more efficient.
Allow streaming of response data (with possibly indefinite length).
Also clean up max_resp_len and add OSSL_HTTP_REQ_CTX_get_resp_len()
Rename maxline parameter to buf_size for clarity.
Rename internal fields and functions in http_client.c for consistency.

Checklist

documentation is added or updated
tests are added or updated

DDvO · 2021-04-30T06:04:33Z

When submitting this PR some 2 days ago, I forgot to mention details about the proposed semantics of the new feature. Here they are:

If keep_alive is 0 then HTTP connections are not kept open after receiving a response, which is the default behavior for HTTP 1.0.
If the value is 1 or 2 then persistent connections are requested.
If the value is 2 then persistent connections are required, i.e., in case the server does not grant them an error occurs.

For the CMP app the default value is 1, which means preferring to keep the connection open.
For all other internal uses of the HTTP client (fetching an OCSP response, a cert, or a CRL) it does not matter because these operations just take one round trip.

If the client application requested or required a persistent connection and this was granted by the server,
it can keep the OSSL_HTTP_REQ_CTX * as long as it wants to send further requests and OSSL_HTTP_REQ_CTX_is_alive() returns nonzero,
else it should call OSSL_HTTP_REQ_CTX_free() or OSSL_HTTP_close().
In case the client application keeps the OSSL_HTTP_REQ_CTX * but the connection then dies for any reason at the server side, it will notice this obtaining an I/O error when trying to send the next request.

DDvO · 2021-04-30T08:51:32Z

@openssl/otc, is there already someone willing to review this?
This must get in before feature freeze, also due to the changes to the new high-level HTTP client API introduced with 3.0.

DDvO · 2021-04-30T08:51:44Z

BTW, I also have some high-level HTTP client API streamlining and generalization in the pipeline to be submitted as follow-up;
would be good if this will make it into 3.0 as well...

DDvO · 2021-05-03T06:47:08Z

I meanwhile found that specific tests for the new keep-alive feature can be added in a rather straightforward way, so done this as well.

DDvO · 2021-05-03T06:48:20Z

Also added a couple of related small fixes of the HTTP client implementation,
mostly regarding the special case of relative URLs, which came up due to the new tests.

DDvO · 2021-05-03T07:54:25Z

Added a fixup on the documentation of OSSL_HTTP_REQ_CTX_set_expected(),
more clearly explaining the default behavior if this new low-level HTTP client API function is not used.

DDvO · 2021-05-03T15:04:46Z

Since so far I have not seen any reaction on this PR that would indicate that someone has started reviewing this,
I've used the time to further generalize and actually simplify the high-level HTTP client API by making the OSSL_HTTP_{get,transfer}_asn1() functions independent of the request and response content type.

DDvO · 2021-05-03T15:05:24Z

On this occasion I've updated the NEWS.md and
extended the CHANGES.md entry on the HTTP client as follows:

   It supports arbitrary request and response content types, GET redirection,
   TLS, connections via HTTP(S) proxies, connections and exchange via
   user-defined BIOs (allowing implicit connections), persistent connections,
   and timeout checks.  See L<OSSL_HTTP_transfer(3)> etc. for details.
   The legacy OCSP-focused (and only partly documented) API
   is retained for backward compatibility, while most of it is deprecated.

DDvO · 2021-05-04T05:40:55Z

@openssl/otc, I am getting more and more concerned about how to get this PR approved and merged before the public API / feature freeze, which is going to be in 16 days from now.

This PR, submitted 6 days ago, implements a feature discussed in #14004. It is critical for one of our customers in order to be able to make their product compliant with ERTMS/ECTS requirements by UNISIG.
Unfortunately, so far I did not receive any reaction on it, while OTOH the deadline for getting public API changes included into 3.0 is approaching rapidly.

How to solve this?
Would it make sense for instance if I carve out the pure API changes such that they are reviewed and approved hopefully quicker and are merged safely before the deadline,
and then add the internal feature implementation as well as the related tests and documentation later (hopefully already into beta1, otherwise during the beta phase)?

DDvO · 2021-05-04T16:49:50Z

Rebased again to fix simple merge conflicts

FdaSilvaYY

great work

apps/lib/http_server.c

doc/man3/OSSL_CMP_SRV_CTX_new.pod

include/openssl/http.h

test/http_test.c

doc/man3/OSSL_HTTP_transfer.pod

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

… URLs) Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

Both at API and at CLI level (for the CMP app only, so far) there is a new parameter/option: keep_alive. * 0 means HTTP connections are not kept open after receiving a response, which is the default behavior for HTTP 1.0. * 1 means that persistent connections are requested. * 2 means that persistent connections are required, i.e., in case the server does not grant them an error occurs. For the CMP app the default value is 1, which means preferring to keep the connection open. For all other internal uses of the HTTP client (fetching an OCSP response, a cert, or a CRL) it does not matter because these operations just take one round trip. If the client application requested or required a persistent connection and this was granted by the server, it can keep the OSSL_HTTP_REQ_CTX * as long as it wants to send further requests and OSSL_HTTP_is_alive() returns nonzero, else it should call OSSL_HTTP_REQ_CTX_free() or OSSL_HTTP_close(). In case the client application keeps the OSSL_HTTP_REQ_CTX * but the connection then dies for any reason at the server side, it will notice this obtaining an I/O error when trying to send the next request. This requires extending the HTTP header parsing and rearranging the high-level HTTP client API. In particular: * Split the monolithic OSSL_HTTP_transfer() into OSSL_HTTP_open(), OSSL_HTTP_set_request(), a lean OSSL_HTTP_transfer(), and OSSL_HTTP_close(). * Split the timeout functionality accordingly and improve default behavior. * Extract part of OSSL_HTTP_REQ_CTX_new() to OSSL_HTTP_REQ_CTX_set_expected(). * Extend struct ossl_http_req_ctx_st accordingly. Use the new feature for the CMP client, which requires extending related transaction management of CMP client and test server. Update the documentation and extend the tests accordingly. Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

Also improve doc of OSSL_parse_url() and OSSL_HTTP_parse_url(). Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

Also clean up OSSL_HTTP_REQ_CTX_nbio() states and make it more efficient. Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

…te length) Also clean up max_resp_len and add OSSL_HTTP_REQ_CTX_get_resp_len(). Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

DDvO · 2021-05-14T17:30:23Z

Merged - thanks a lot to all to contributed to being able to merge this PR in time!

This is a minimal version of pull request openssl#15053 including all the proposed improvements to the HTTP client API and its documentation but only those code adaptations strictly needed for it. The proposed new features include * support for persistent connections (keep-alive), * generalization to arbitrary request and response types, and * support for streaming BIOs for request and response data. The related API changes include: * Split the monolithic OSSL_HTTP_transfer() into OSSL_HTTP_open(), OSSL_HTTP_set_request(), a lean OSSL_HTTP_transfer(), and OSSL_HTTP_close(). * Split the timeout functionality accordingly and improve default behavior. * Extract part of OSSL_HTTP_REQ_CTX_new() to OSSL_HTTP_REQ_CTX_set_expected(). Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15147)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

… URLs) Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

Both at API and at CLI level (for the CMP app only, so far) there is a new parameter/option: keep_alive. * 0 means HTTP connections are not kept open after receiving a response, which is the default behavior for HTTP 1.0. * 1 means that persistent connections are requested. * 2 means that persistent connections are required, i.e., in case the server does not grant them an error occurs. For the CMP app the default value is 1, which means preferring to keep the connection open. For all other internal uses of the HTTP client (fetching an OCSP response, a cert, or a CRL) it does not matter because these operations just take one round trip. If the client application requested or required a persistent connection and this was granted by the server, it can keep the OSSL_HTTP_REQ_CTX * as long as it wants to send further requests and OSSL_HTTP_is_alive() returns nonzero, else it should call OSSL_HTTP_REQ_CTX_free() or OSSL_HTTP_close(). In case the client application keeps the OSSL_HTTP_REQ_CTX * but the connection then dies for any reason at the server side, it will notice this obtaining an I/O error when trying to send the next request. This requires extending the HTTP header parsing and rearranging the high-level HTTP client API. In particular: * Split the monolithic OSSL_HTTP_transfer() into OSSL_HTTP_open(), OSSL_HTTP_set_request(), a lean OSSL_HTTP_transfer(), and OSSL_HTTP_close(). * Split the timeout functionality accordingly and improve default behavior. * Extract part of OSSL_HTTP_REQ_CTX_new() to OSSL_HTTP_REQ_CTX_set_expected(). * Extend struct ossl_http_req_ctx_st accordingly. Use the new feature for the CMP client, which requires extending related transaction management of CMP client and test server. Update the documentation and extend the tests accordingly. Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

Also improve doc of OSSL_parse_url() and OSSL_HTTP_parse_url(). Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

Also clean up OSSL_HTTP_REQ_CTX_nbio() states and make it more efficient. Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

…te length) Also clean up max_resp_len and add OSSL_HTTP_REQ_CTX_get_resp_len(). Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

DDvO added the approval: otc review pending label Apr 27, 2021

DDvO added this to the 3.0.0 beta1 milestone Apr 27, 2021

DDvO force-pushed the http_API_keep-alive branch 3 times, most recently from e1ec2af to 6ddf09f Compare April 27, 2021 22:51

DDvO added the triaged: feature The issue/pr requests/adds a feature label Apr 28, 2021

DDvO mentioned this pull request Apr 30, 2021

HTTP: support persistent connections ("keep-alive") #14004

Closed

DDvO removed this from the 3.0.0 beta1 milestone Apr 30, 2021

This was referenced Apr 30, 2021

OCSP: Minor improvements of documentation and header file #15103

Closed

HTTP client: Correct the use of optional proxy URL and its documentation #15104

Closed

DDvO force-pushed the http_API_keep-alive branch from 6ddf09f to 0b550a6 Compare May 3, 2021 06:46

DDvO mentioned this pull request May 3, 2021

testutil/load.c: Add checks for file(name) == NULL #15120

Closed

DDvO force-pushed the http_API_keep-alive branch from 0b550a6 to 856dbf9 Compare May 3, 2021 07:52

DDvO force-pushed the http_API_keep-alive branch from 14d968c to cce3d07 Compare May 3, 2021 15:05

DDvO added this to the 3.0.0 beta1 milestone May 3, 2021

DDvO force-pushed the http_API_keep-alive branch from cce3d07 to 80b0eb0 Compare May 4, 2021 06:12

DDvO mentioned this pull request May 4, 2021

Deprecate X509{,_CRL}_http_nbio() and simplify their definition #15131

Closed

1 task

DDvO force-pushed the http_API_keep-alive branch from 80b0eb0 to 8fc334e Compare May 4, 2021 15:01

DDvO force-pushed the http_API_keep-alive branch from 8fc334e to c958d0c Compare May 4, 2021 16:49

FdaSilvaYY reviewed May 4, 2021

View reviewed changes

openssl-machine pushed a commit that referenced this pull request May 14, 2021

HTTP test server: Improve connection management and logging

cc1af4d

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

openssl-machine pushed a commit that referenced this pull request May 14, 2021

CMP test server: Extend error reporting on cert rejected for revocation

ca8f823

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

openssl-machine pushed a commit that referenced this pull request May 14, 2021

OSSL_HTTP_REQ_CTX_add1_headers(): Fix use with host == NULL (relative…

19a39b2

… URLs) Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

openssl-machine pushed a commit that referenced this pull request May 14, 2021

OSSL_HTTP_get(): Do not close connection if redirect to same server

8801240

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

openssl-machine pushed a commit that referenced this pull request May 14, 2021

OSSL_HTTP_transfer(): Fix error reporting in case rctx->server is NULL

22fe2b1

Also improve doc of OSSL_parse_url() and OSSL_HTTP_parse_url(). Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

openssl-machine pushed a commit that referenced this pull request May 14, 2021

HTTP client API: Generalize to arbitrary request and response contents

8299028

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

openssl-machine pushed a commit that referenced this pull request May 14, 2021

HTTP client: Allow streaming of request data (for POST method)

8b5ca51

Also clean up OSSL_HTTP_REQ_CTX_nbio() states and make it more efficient. Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

openssl-machine pushed a commit that referenced this pull request May 14, 2021

http_client.c: Rename internal fields and functions for consistency

e2c38c1

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

openssl-machine pushed a commit that referenced this pull request May 14, 2021

Add OSSL_ prefix to HTTP_DEFAULT_MAX_{LINE_LENGTH,RESP_LEN}

647a5db

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15053)

DDvO closed this May 14, 2021

This was referenced May 15, 2021

Persistent HTTP connections: use same TCP/TLS connection for all messages of a transaction mpeylo/cmpossl#217

Closed

CMP over HTTPS mpeylo/cmpossl#216

Closed

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

HTTP client: Rename 'maxline' parameter to 'buf_size' for clarity

524ed66

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

OSSL_CMP_SRV_process_request(): Log any error queue entries on response

eeab8d2

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

cmp_server.c: Improve transaction management and logging

a303532

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

HTTP test server: Improve connection management and logging

2916f02

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

CMP test server: Extend error reporting on cert rejected for revocation

0a6019e

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

OSSL_HTTP_REQ_CTX_add1_headers(): Fix use with host == NULL (relative…

a0f435f

… URLs) Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

OSSL_HTTP_get(): Do not close connection if redirect to same server

f655628

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

HTTP client API: Generalize to arbitrary request and response contents

e4e28b1

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

http_client.c: Rename internal fields and functions for consistency

48050f6

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021

Add OSSL_ prefix to HTTP_DEFAULT_MAX_{LINE_LENGTH,RESP_LEN}

1804fc0

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15053)

Uh oh!

Conversation

DDvO commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

DDvO commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 3, 2021

Uh oh!

DDvO commented May 3, 2021

Uh oh!

DDvO commented May 3, 2021

Uh oh!

DDvO commented May 3, 2021

Uh oh!

DDvO commented May 3, 2021

Uh oh!

DDvO commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 4, 2021

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!

Uh oh!

DDvO commented May 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DDvO commented Apr 27, 2021 •

edited

Loading

DDvO commented Apr 30, 2021 •

edited

Loading

DDvO commented Apr 30, 2021 •

edited

Loading

DDvO commented Apr 30, 2021 •

edited

Loading

DDvO commented May 4, 2021 •

edited

Loading