SSL for HTTP server #2578
SSL for HTTP server #2578
Conversation
|
Thanks for your contribution. This PR involves API change, we are discussing ways in which that can be avoided at this moment (for getting web server handle, maybe we can use thread local storage instead). Will keep you posted on this. Also just to give you heads up, |
| * - Bytes : The number of bytes sent successfully | ||
| * - -VE : In case of error | ||
| */ | ||
| typedef int (*httpd_send_func_t)(httpd_handle_t hd, int sockfd, const char *buf, size_t buf_len, int flags); |
There was a problem hiding this comment.
@MightyPork This changes the API, which we would like to avoid
A workaround would be to define a new public function like httpd_get_server_handle(), and use that to extract the handle from inside your custom send/recv function.
Since the send/recv functions will always be invoked from inside the server thread, you could use pvTaskSetThreadLocalStoragePointer() to store the server handle during creation and later retrieve the handle from pvTaskGetThreadLocalStoragePointer().
There was a problem hiding this comment.
this can be done, but it seems needlessly convoluted to me. Is there a reason to worry about API changes, if the http_server component is still unreleased in a stable version? (this is why I'm using master instead of the stable release)
while we're discussing it, what was the original purpose of the send / recv override functions? Because if they are used for SSL handling, they become unavailable for anything else.
There was a problem hiding this comment.
Is there a reason to worry about API changes, if the http_server component is still unreleased in a stable version?
Your are right. But my only concern is that people might have already started using the API. Still let me discuss this and get back to you.
while we're discussing it, what was the original purpose of the send / recv override functions?
The purpose was to allow for extension/encryption/encapsulation of HTTP packets as per requirement, just like you are using it for SSL
Because if they are used for SSL handling, they become unavailable for anything else.
That is alright. They are intended to allow one layer of extension only. Now in case someone requires something fancy with SSL as well as some form of encapsulation then she will have to start implementing on top of http_server.
There was a problem hiding this comment.
@MightyPork After some discussion we have concluded that changing the API should be alright in this case, like you said
| */ | ||
| static int httpdssl_recv(httpd_handle_t server, int sockfd, char *buf, size_t buf_len, int flags) | ||
| { | ||
| SSL *ssl = httpd_sess_get_ctx(server, sockfd); |
There was a problem hiding this comment.
Session context is passed along with the httpd_req_t structure when URI handler is executed. It's useful for session persistence (See the persistent_sockets example).
Could you use a different member variable (and corresponding APIs) for saving and accessing the pointer to SSL object. Maybe call it priv_ctx or internal_ctx?
There was a problem hiding this comment.
sure. in that case, do you want to keep the functions httpd_sess_get_ctx / httpd_sess_set_ctx that I added for this purpose as well as adding new ones for this 'internal_ctx'?
There was a problem hiding this comment.
You can keep them. They may come handy sometime.
|
There seem to be conflicts in Could you please resolve them? |
sure, I plan to have a look at it tonight (GMT+1) and fix this + the other issues |
bbe4115 to
74ded80
Compare
|
@anurag-kar can you please review the changes? it's rebased and should be ready to merge it now uses 'transport_ctx' instead of 'ctx' |
|
@MightyPork I'm in the process of reviewing, but because I am supposed to travel for the upcoming 2-3 days, so it may take some time. I see the github comments you've put and I find them helpful.
There was another We are looking forward to have your pull request so I'll hold of any more changes to Also, could you add a Also, please rename |
d5544f3 to
30ecc88
Compare
|
it's now rebased again, but i'm trying to fix one new bug I found |
|
All good, the bug is fixed, close_fn added and I also added license comments. from my side I'm done with this feature, hope it's good like this |
| * @brief Prototype for freeing context data (if any) | ||
| * @param[in] ctx : object to free | ||
| */ | ||
| typedef void (*httpd_free_sess_ctx_fn_t)(void *ctx); |
There was a problem hiding this comment.
Since we have agreed to modify the APIs, I guess we can also rename this to httpd_free_ctx_fn_t since this is being used for both global and session contexts
There was a problem hiding this comment.
Sure, but it'll take about 12 hours before I can do this. If you want, please feel free to rename it yourself
|
@MightyPork Except the small changes suggested, this looks really good to me. Thanks for putting all the effort in making this feature possible. |
|
One more thing, please squash your commits into two. One commit for changes to esp_http_server. Second commit adds new esp_https_server component along with examples. This way it looks clean during merge. |
Changes:
- renamed `httpd_free_sess_ctx_fn_t` to `httpd_free_ctx_fn_t`
- added a `httpd_handle_t` argument to `httpd_send_func_t` and `httpd_recv_func_t`
- internal function `httpd_sess_get()` is no longer static, as it's used in other
files besides httpd_sess.c
Bug fixes:
- removed a trailing semicolon from `HTTPD_DEFAULT_CONFIG()`
- fixed issue with failed `select()`, now it automatically closes invalid sockets
instead of shutting down the entire server
New features:
- `httpd_resp_send()` and `httpd_resp_send_chunk()` now accept -1 as length to use
`strlen()` internally
- added `httpd_sess_set_ctx()` to accompany `httpd_sess_get_ctx()`
- added a "transport context" to the session structure (next to user context)
- added `httpd_sess_{get,set}_transport_ctx()` to work with this transport context
- added "global user context" and "global transport context" stored in the server
config (and then the handle); supports a user-provided free_fn
- added a "pending func" to e.g. check for data in the transport layer receive
buffer
- added functions `httpd_set_sess_{send,recv,pending}_override()` that target
a session by ID (i.e. not using a request object)
- added `httpd_set_pending_override()`
- added a "open_fn" and "close_fn" - functions called when creating and closing
a session. These may be used to set up transport layer encryption or some other
session-wide feature
f20a42b to
4947ff4
Compare
|
@anurag-kar ready for merge and I hope it's now final. The first commit has a detailed message explaining all the changes for future reference |
|
@MightyPork Thank you for cooperating. Will update you when it gets merged internally |
|
Just a question, how if this supposed to working in practice ? |
|
The main use case for me is to make the node securely accessible over the
network, for example to adjust it's configuration, check it works correctly
etc. I never meant this to be used to run a public server, the esp doesn't
have the horsepower for that anyway. Hence there is no problem for me with
a self signed certificate or one lasting e.g. 10 years. You could, I guess,
set up a dummy CA and preload the root into your browser, if you're
obsessed with having a "green lock". for the record, you could update the
certificate via OTA or in other way, all it takes is to restart the server.
…On Mon, Nov 5, 2018, 4:22 PM X-Ryl669 ***@***.*** wrote:
Just a question, how if this supposed to working in practice ?
I mean, now the industry is moving to short lived certificates, you'll
have a lot of trouble getting a certificate that's valid for, let say, 5
years (even 2 years will be hard in the future). If you make your own
certificate, the browser will never accept it and it'll be a pain for user
to use such server (they'll have a "security warning" most don't
understand, and it's hard to explain to them to add an exception for your
certificate). In the end, I guess the only workable way is to have the
system somehow, download its certificate from some authority, or have a
certbot on the ESP32, but this means always being connected to Internet.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2578 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB8lHo08HTBx-JS0OyX5EFVg-wnH3cqKks5usFfPgaJpZM4XjulV>
.
|
|
I don't want to sound impatient, but what takes so long about accepting an approved merge request? |
Apologies regarding timelines. Approved PR goes through our internal review and testing (functional/integration), so its taking bit longer, in this specific case we also observed few areas for code cleanup. This will reflect in public repository in next few days. |
|
Thanks for contribution. Merged with 61ee1bd |
|
Thanks for merging and the update! one mistake i found - https://github.com/espressif/esp-idf/blob/master/docs/en/api-reference/protocols/esp_https_server.rst (the code changes were done in 9a9d18e) |
|
@MightyPork You are right, we will fix in followup commit. |
This PR adds HTTPS capability to the HTTP server component. It is implemented using the already existing API, with some necessary tweaks. It also fixes a few bugs and issues I encountered, see below.
HTTP server changes:
HTTPS server component
advantage of the new APIs and tweaks