Generalize HTTP server code from apps/ocsp.c to apps/lib/http_server.c by DDvO · Pull Request #11736 · openssl/openssl

DDvO · 2020-05-05T14:25:27Z

This is a further spin-off PR of #11470 as requested by @mattcaswell: #11470 (comment).

This adds apps/lib/http_server.c and apps/include/http_server.h
simplifying apps/ocsp.c. This going to be used also for apps/cmp.c
and will be helpful as well for any future app acting as an HTTP server.

This PR also contains a couple of related fixes as separate commits:

Fix bio_wait() in crypto/bio/bio_lib.c in case OPENSSL_NO_SOCK
Constify 'req' parameter of OSSL_HTTP_post_asn1()
Fix bug in OSSL_CMP_SRV_process_request() on transaction renewal

mattcaswell

This looks good to me. A couple of comments below.

I wonder if @vdukhovni might have some cycles to look at it?

apps/ocsp.c

crypto/cmp/cmp_server.c

apps/lib/http_server.c

apps/include/http_server.h

DDvO · 2020-05-08T09:59:23Z

Thanks @mattcaswell and @vdukhovni for your reviews and comments.
I've addressed all of them.

mattcaswell

Approved assuming the minor nit is fixed.

apps/include/http_server.h

Also adds apps/include/http_server.h. This is used so far by apps/ocsp.c and is going to be used for apps/cmp.c and will be helpful also for any future app acting as HTTP server.

DDvO · 2020-05-08T12:51:43Z

Approved assuming the minor nit is fixed.

Thanks for your swift reaction!
I've fixed the doc nit and partly squashed the commit structure for merging tomorrow.

vdukhovni

Thanks, looks good.

vdukhovni · 2020-05-08T22:03:40Z

It would have been cool to leverage a ubiquitous light-weight HTTP server library in C, with a suitable license instead of rolling our own, but the only one that comes to mind (libmicrohttpd) is LGPL. Mind you, we only use the HTTP server in "apps", it wouldn't be quite so bad to have it be runtime linked to an LGPL library, but we'd still need to something on systems where that's not available. Anyone know of suitable alternatives?

vdukhovni · 2020-05-08T22:06:42Z

It would have been cool to leverage a ubiquitous light-weight HTTP server library in C, with a suitable license instead of rolling our own, but the only one that comes to mind (libmicrohttpd) is LGPL. Mind you, we only use the HTTP server in "apps", it wouldn't be quite so bad to have it be runtime linked to an LGPL library, but we'd still need to something on systems where that's not available. Anyone know of suitable alternatives?

Anyone have any experience good/bad with libhttp? The license is MIT, so not a problem...

openssl-machine · 2020-05-09T12:00:11Z

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.

DDvO · 2020-05-09T14:59:02Z

Merged - thanks @mattcaswell and @vdukhovni!

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Viktor Dukhovni <[email protected]> (Merged from #11736)

Also adds apps/include/http_server.h. This is used so far by apps/ocsp.c and is going to be used for apps/cmp.c and will be helpful also for any future app acting as HTTP server. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Viktor Dukhovni <[email protected]> (Merged from #11736)

DDvO · 2020-05-09T15:05:49Z

It would have been cool to leverage a ubiquitous light-weight HTTP server library in C, with a suitable license instead of rolling our own

I agree this would make sense at least instead of trying to extend this very rudimentary HTTP server implementation, which would be a mayor endeavor.
The same I'd say holds for the HTTP client merged in #10667, which is somewhat more advanced but also far from being complete and already took quite some effort.

DDvO mentioned this pull request May 5, 2020

Chunk 11 of CMP contribution to OpenSSL: CMP command-line interface #11470

Closed

mattcaswell requested changes May 6, 2020

View reviewed changes

apps/ocsp.c Outdated Show resolved Hide resolved

crypto/cmp/cmp_server.c Outdated Show resolved Hide resolved

vdukhovni requested changes May 7, 2020

View reviewed changes

apps/lib/http_server.c Outdated Show resolved Hide resolved

apps/lib/http_server.c Outdated Show resolved Hide resolved

apps/lib/http_server.c Outdated Show resolved Hide resolved

apps/include/http_server.h Outdated Show resolved Hide resolved

DDvO added this to the 3.0.0 milestone May 8, 2020

mattcaswell approved these changes May 8, 2020

View reviewed changes

apps/include/http_server.h Outdated Show resolved Hide resolved

mattcaswell added approval: done This pull request has the required number of approvals branch: master Applies to master branch labels May 8, 2020

DDvO added 4 commits May 8, 2020 14:47

Fix bug in OSSL_CMP_SRV_process_request() on transaction renewal

15afbe7

Constify 'req' parameter of OSSL_HTTP_post_asn1()

a10a588

Fix bio_wait() in crypto/bio/bio_lib.c in case OPENSSL_NO_SOCK

139f938

Extract HTTP server code from apps/ocsp.c to apps/lib/http_server.c

b32b30b

Also adds apps/include/http_server.h. This is used so far by apps/ocsp.c and is going to be used for apps/cmp.c and will be helpful also for any future app acting as HTTP server.

DDvO force-pushed the extract_http_server branch from c19fd14 to b32b30b Compare May 8, 2020 12:50

vdukhovni approved these changes May 8, 2020

View reviewed changes

DDvO added the approval: ready to merge The 24 hour grace period has passed, ready to merge label May 9, 2020

openssl-machine pushed a commit that referenced this pull request May 9, 2020

Fix bug in OSSL_CMP_SRV_process_request() on transaction renewal

045229c

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Viktor Dukhovni <[email protected]> (Merged from #11736)

openssl-machine pushed a commit that referenced this pull request May 9, 2020

Constify 'req' parameter of OSSL_HTTP_post_asn1()

9253f83

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Viktor Dukhovni <[email protected]> (Merged from #11736)

openssl-machine pushed a commit that referenced this pull request May 9, 2020

Fix bio_wait() in crypto/bio/bio_lib.c in case OPENSSL_NO_SOCK

d8c78e5

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Viktor Dukhovni <[email protected]> (Merged from #11736)

mattcaswell closed this May 12, 2020

richsalz mentioned this pull request Apr 20, 2021

Read a REQUEST not RESPONSE in ocsp responder #14946

Closed

Uh oh!

Comments

Conversation

DDvO commented May 5, 2020

Uh oh!

mattcaswell 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 May 8, 2020

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DDvO commented May 8, 2020

Uh oh!

vdukhovni left a comment

Choose a reason for hiding this comment

Uh oh!

vdukhovni commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdukhovni commented May 8, 2020

Uh oh!

openssl-machine commented May 9, 2020

Uh oh!

DDvO commented May 9, 2020

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vdukhovni commented May 8, 2020 •

edited

Loading

DDvO commented May 9, 2020 •

edited

Loading