HTTP client: Minimal changes that include the improved API by DDvO · Pull Request #15147 · openssl/openssl

DDvO · 2021-05-04T22:17:22Z

This is a minimal version of #15053
including all the proposed improvements to the HTTP client API
but only those code adaptations strictly needed for it.

Is this helpful for reviewing the API and preliminary inclusion in the upcoming 3.0 beta1?

In case this gets merged, #15053 can then be based on it to provide the meat,
i.e., the implementation of the keep-alive feature etc., extended tests, and updated documentation.

Checklist

documentation is added or updated
tests are added or updated

t8m · 2021-05-05T06:43:48Z

I think both this API PR and the "meat" PR should be merged before beta1. And I am in favor of the split mainly because it keeps the size of the PRs more reasonable.

DDvO · 2021-05-05T06:56:35Z

Thanks a lot @t8m for your response!

I think both this API PR and the "meat" PR should be merged before beta1.

Pleased to hear. Yes, this should make most sense.

And I am in favor of the split mainly because it keeps the size of the PRs more reasonable.

I'm glad that the extra effort I spent in tentatively providing this split is going to pay off.

Who can do the review(s) and start when, since unfortunately this is pretty urgent?

DDvO · 2021-05-05T06:59:48Z

I'll take care of the remaining CI failures (due to mem leaks) later today...

DDvO · 2021-05-05T20:03:31Z

Mem leaks fixed.
check_update CI failure is unrelated.
Ready for review.

DDvO · 2021-05-05T20:57:54Z

In order to get rid of the unrelated CI failure on check_update
I had to add an artificial commit for updating the FIPS checksums
since apparently this meanwhile required and someone forgot to do so.

DDvO · 2021-05-06T14:08:16Z

@t8m, are you going to review this PR?
Its core are just some 130 lines changed in header files.

t8m

In general this looks good except for a few things.

doc/man3/OSSL_HTTP_REQ_CTX.pod

test/http_test.c

crypto/http/http_client.c

t8m · 2021-05-06T16:15:38Z

You should be able to drop the fips.checksum update commit now.

DDvO · 2021-05-06T19:26:14Z

In general this looks good except for a few things.

Pleased to hear - thanks for your swift review; I've handled all comments.

DDvO · 2021-05-06T19:26:38Z

You should be able to drop the fips.checksum update commit now.

Yes 👍

DDvO · 2021-05-08T04:42:12Z

CI failures are unrelated - currently credential loading appears broken.

crypto/asn1/a_d2i_fp.c

DDvO · 2021-05-10T10:38:04Z

I'm still stuck with trying to dissect where the various new test failures come from, part of which are related to a strange regression in private key loading:
In case a key is not encrypted so far it did not hurt if a (useless) password for decryption was provided; now loading via OSSL_STORE fails.
Does anyone have a hint/idea why?

DDvO · 2021-05-10T10:39:24Z

@t8m, I hope that, taking aside the test failures, you can still continue reviewing this PR, which meanwhile includes the updated and extended documentation.

DDvO · 2021-05-10T12:55:57Z

I'm still stuck with trying to dissect where the various new test failures come from

I think I've managed to sufficiently trace them down and handle them.
Found and fixed an instability of the HTTP test server instability when it returns an error message.

part of which are related to a strange regression in private key loading:
In case a key is not encrypted so far it did not hurt if a (useless) password for decryption was provided; now loading via OSSL_STORE fails.

~~Since this issue is unrelated I've decoupled it from this PR by disabling the respective (unimportant) test case for now.~~
Update: Meanwhile found the root cause - see 15219.
So reverted the planned improvement on asn1_d2i_read_bio(), and no need to adapt the tests.

…bio() ASN1_item_d2i_bio(): Do not report errors in queue on BIO input being NULL

rename OSSL_HTTP_d2i_free_bio to OSSL_HTTP_d2i_consume_bio

…proved API

t8m

Just some small stuff remaining to fix and I'll approve. Please do not rebase.

include/openssl/http.h

crypto/http/http_client.c

doc/man3/OCSP_sendreq_new.pod

doc/man3/OSSL_HTTP_REQ_CTX.pod

doc/man3/OSSL_HTTP_transfer.pod

doc/man3/ASN1_item_d2i_bio.pod

… the improved API

t8m

Good work!

DDvO · 2021-05-11T13:06:51Z

Good work!

Thank you 😄

openssl-machine · 2021-05-12T13:00:17Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

…bio() ASN1_item_d2i_bio(): Do not report errors in queue on BIO input being NULL Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15147)

This is a minimal version of pull request #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 #15147)

DDvO · 2021-05-12T13:13:46Z

Merged - thanks a lot @t8m for the timely handling of this non-trivial PR!

…bio() ASN1_item_d2i_bio(): Do not report errors in queue on BIO input being NULL Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15147)

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)

DDvO added approval: otc review pending triaged: feature The issue/pr requests/adds a feature labels May 4, 2021

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

DDvO mentioned this pull request May 4, 2021

HTTP: Implement persistent connections (keep-alive) etc. #15053

Closed

2 tasks

openssl-machine force-pushed the master branch from d5315e1 to 029875d Compare May 4, 2021 23:31

DDvO force-pushed the http_API_only branch from a354f7f to dff4d88 Compare May 5, 2021 06:33

DDvO changed the title ~~WIP: HTTP client: Minimal changes that include the improved API~~ HTTP client: Minimal changes that include the improved API May 5, 2021

DDvO marked this pull request as ready for review May 5, 2021 20:03

DDvO force-pushed the http_API_only branch from a38b5f3 to 18792f7 Compare May 5, 2021 20:57

t8m requested changes May 6, 2021

View reviewed changes

DDvO force-pushed the http_API_only branch from 18792f7 to 8e0c36c Compare May 6, 2021 19:19

DDvO force-pushed the http_API_only branch from 651378c to 31ff16a Compare May 7, 2021 20:24

DDvO force-pushed the http_API_only branch from 31ff16a to 4bd69e3 Compare May 8, 2021 13:30

levitte reviewed May 9, 2021

View reviewed changes

crypto/asn1/a_d2i_fp.c Outdated Show resolved Hide resolved

crypto/asn1/a_d2i_fp.c Outdated Show resolved Hide resolved

DDvO force-pushed the http_API_only branch from 4bd69e3 to 4f10541 Compare May 9, 2021 08:47

DDvO force-pushed the http_API_only branch 2 times, most recently from b7514be to cb8bc29 Compare May 10, 2021 12:53

DDvO force-pushed the http_API_only branch from cb8bc29 to 1274692 Compare May 10, 2021 14:07

DDvO mentioned this pull request May 10, 2021

asn1_d2i_read_bio() has brittle design depending on the error queue #15219

Closed

DDvO added 5 commits May 11, 2021 12:07

Add ASN1_item_i2d_mem_bio(); document and improve also ASN1_item_d2i_…

c297f0d

…bio() ASN1_item_d2i_bio(): Do not report errors in queue on BIO input being NULL

HTTP client: Minimal changes that include the improved API

f53b8a2

fixup! HTTP client: Minimal changes that include the improved API

65fe5bd

rename OSSL_HTTP_d2i_free_bio to OSSL_HTTP_d2i_consume_bio

fixup! fixup! HTTP client: Minimal changes that include the improved API

63fd5ac

fixup! fixup! fixup! HTTP client: Minimal changes that include the im…

4cb445c

…proved API

DDvO force-pushed the http_API_only branch from 1274692 to 4cb445c Compare May 11, 2021 10:09

DDvO requested a review from t8m May 11, 2021 10:16

t8m requested changes May 11, 2021

View reviewed changes

fixup! fixup! fixup! fixup! HTTP client: Minimal changes that include…

731d246

… the improved API

t8m approved these changes May 11, 2021

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending labels May 11, 2021

DDvO added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels May 12, 2021

DDvO closed this May 12, 2021

Uh oh!

Conversation

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

Uh oh!

Checklist

Uh oh!

t8m commented May 5, 2021

Uh oh!

DDvO commented May 5, 2021

Uh oh!

DDvO commented May 5, 2021

Uh oh!

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

Uh oh!

Uh oh!

DDvO commented May 5, 2021

Uh oh!

DDvO commented May 6, 2021

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t8m commented May 6, 2021

Uh oh!

DDvO commented May 6, 2021

Uh oh!

DDvO commented May 6, 2021

Uh oh!

DDvO commented May 8, 2021

Uh oh!

Uh oh!

Uh oh!

DDvO commented May 10, 2021

Uh oh!

DDvO commented May 10, 2021

Uh oh!

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

Uh oh!

Uh oh!

t8m 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!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented May 11, 2021

Uh oh!

openssl-machine commented May 12, 2021

Uh oh!

DDvO commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DDvO commented May 4, 2021 •

edited

Loading

DDvO commented May 5, 2021 •

edited

Loading

DDvO commented May 10, 2021 •

edited

Loading