-
Notifications
You must be signed in to change notification settings - Fork 14.1k
server: SSL Support #5926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
server: SSL Support #5926
Conversation
Signed-off-by: Gabe Goodhart <[email protected]>
All SSL setup is hidden behind CPPHTTPLIB_OPENSSL_SUPPORT in the same way that the base httlib hides the SSL support Signed-off-by: Gabe Goodhart <[email protected]>
| key_file.close(); | ||
| } else if (arg == "--timeout" || arg == "-to") { | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of hate that this breaks the brace formatting of the rest of the if/else if chain, but I figured it was better than duplicating the else if (arg == "--timeout"....
|
Note that the tests are written in Python, but there is no binding, just plain old http calls to the server. Anyway, these are mainly functional tests, and I don't expect to add an ssl handcheck against the server as it will require a trusted root certificate. If it compiles with the good libopenssl version, it will work. |
phymbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the README.md with the proposed changes
Signed-off-by: Gabe Goodhart <[email protected]>
Signed-off-by: Gabe Goodhart <[email protected]>
|
Thanks for the review! The README changes are pushed as well as a first pass at top-level |
For a few libraries we are using pkg-config. It's probably not necessary here though. |
phymbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really useful, thanks
* add cmake build toggle to enable ssl support in server Signed-off-by: Gabe Goodhart <[email protected]> * add flags for ssl key/cert files and use SSLServer if set All SSL setup is hidden behind CPPHTTPLIB_OPENSSL_SUPPORT in the same way that the base httlib hides the SSL support Signed-off-by: Gabe Goodhart <[email protected]> * Update readme for SSL support in server Signed-off-by: Gabe Goodhart <[email protected]> * Add LLAMA_SERVER_SSL variable setup to top-level Makefile Signed-off-by: Gabe Goodhart <[email protected]> --------- Signed-off-by: Gabe Goodhart <[email protected]>
* add cmake build toggle to enable ssl support in server Signed-off-by: Gabe Goodhart <[email protected]> * add flags for ssl key/cert files and use SSLServer if set All SSL setup is hidden behind CPPHTTPLIB_OPENSSL_SUPPORT in the same way that the base httlib hides the SSL support Signed-off-by: Gabe Goodhart <[email protected]> * Update readme for SSL support in server Signed-off-by: Gabe Goodhart <[email protected]> * Add LLAMA_SERVER_SSL variable setup to top-level Makefile Signed-off-by: Gabe Goodhart <[email protected]> --------- Signed-off-by: Gabe Goodhart <[email protected]>
* add cmake build toggle to enable ssl support in server Signed-off-by: Gabe Goodhart <[email protected]> * add flags for ssl key/cert files and use SSLServer if set All SSL setup is hidden behind CPPHTTPLIB_OPENSSL_SUPPORT in the same way that the base httlib hides the SSL support Signed-off-by: Gabe Goodhart <[email protected]> * Update readme for SSL support in server Signed-off-by: Gabe Goodhart <[email protected]> * Add LLAMA_SERVER_SSL variable setup to top-level Makefile Signed-off-by: Gabe Goodhart <[email protected]> --------- Signed-off-by: Gabe Goodhart <[email protected]>
|
If anyone is struggling to get this to work, please note that there does not appear to be error handliing for --ssl-key-file and --ssl-cert-file. So if you fat-finger the filename, you won't get an error that the filename doesn't exist. You'll get |
Description
This PR adds SSL support to the
examples/serverusing the built-in SSL support in the vendoredhttlib.hheader library.Changes
CMakebuild optionLLAMA_SERVER_SSL(defaultOFF) to toggle building with/without SSL supportlibssl/libcrypto, so this adds a dependency onopenssland requires version >= 3 (see note here on thecpp-httplibrepo)#define CPPHTTPLIB_OPENSSL_SUPPORTfollowing the pattern inhttplibssl_key_fileandssl_cert_fileto theserver_paramsstruct--ssl-key-fileand--ssl-cert-filehttplibdoes not support inline PEM strings, so these must point to filessvrinstance variable to be aunique_ptr<httplib::Server>and instantiate it as either ahttplib::Serverorhttplib::SslServerdepending on the values ofserver_params.ssl_key_fileandserver_params.ssl_cert_file.svrto move from.to->, so touched a lot of lines!Testing
I noticed the tests in examples/server/tests, but these all appear to be testing against the python bindings. Since building with SSL support requires additional dependencies, I did not want to add these as dependencies in the python bindings and did not attempt to make the default build for python include SSL support, therefore I did not update these tests.
I performed manual testing using locally-generated SSL files (see my utility script for generating them below). With the generated
server.key.pemandserver.cert.pem, I ran the following:gen_mtls_test_files.sh
Open Questions
nginxproxy is a more robust way to go. That said, having basic SSL support in the native server is a much simpler deployment topology, so may be worth the maintenance effort.--ssl-key-fileand--ssl-cert-filearguments only work if both are specified. Currently, if either is not specified, they are both ignored. Should they instead cause an error if one is specified without the other?cpp-httlibdoes not support mutual TLS, this change also does not support it. Is it worth having non-mutual support without going all the way to mTLS?