Native TLS for the gRPC endpoint (REST gated) (#2144)#4334
Open
exzile wants to merge 6 commits into
Open
Conversation
Implements native TLS/HTTPS for the API endpoints (issue openvinotoolkit#2144) so traffic can be encrypted without relying on an external reverse proxy. gRPC TLS (functional): - New per-protocol CLI params (optional, empty = disabled): grpc_certificate_path, grpc_key_path, and grpc_ca_path. Setting cert+key enables server TLS; adding a CA enables mutual TLS (client certificates are required and verified). - grpcservermodule builds grpc::SslServerCredentials from the cert/key (and CA for mTLS via GRPC_SSL_REQUEST_AND_REQUIRE_CLIENT_CERTIFICATE_AND_VERIFY). No insecure fallback: a missing/unreadable cert/key aborts startup rather than serving plaintext. REST TLS (gated, fail-closed): - The same rest_certificate_path/rest_key_path/rest_ca_path params and Drogon SSL wiring are in place, but the bundled Drogon is built without OpenSSL and cannot serve HTTPS. To avoid silently serving plaintext on a port the operator believes is encrypted, Config::validate() rejects any rest TLS parameter at startup. The wiring is retained so REST HTTPS activates once Drogon is built with OpenSSL. Also: - ServerSettings fields, Config accessors, and C-API setters (OVMS_ServerSettingsSet*) for all six params, declared in ovms.h. - Validation: cert/key must be set together, files must exist and be non-empty, CA requires cert+key. Runs on both CLI and C-API start paths. - Private key contents are never logged; only paths appear in logs/errors. Tested: config-validation death tests (cert/key/ca combinations, missing/empty files, REST-gated rejection), C-API setter coverage, and an end-to-end gRPC TLS handshake verified with openssl s_client (TLS 1.2, server cert presented). Reviewed for security (no plaintext fallback, mTLS enforced, no key leakage). Implements openvinotoolkit#2144 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deduplicate the gRPC and REST TLS validation in Config::validate() into a single validateTlsMaterial() helper (cert/key set together, files exist and are non-empty, CA requires cert+key). Removes ~40 lines of duplication and the previously-unreachable REST validation block that sat after the fail-closed REST guard. The REST guard remains (Drogon has no OpenSSL in this build); a comment documents the one-line change to reactivate REST validation once it does. No behavior change: gRPC validation is identical, REST stays fail-closed. All TLS config/C-API unit tests still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-up (minor robustness): - validateTlsMaterial: also confirm each TLS file is readable (open it), and use the error_code overload of std::filesystem::exists, so a permission-blocked or otherwise unreadable cert/key fails validation with a clear message instead of throwing later at TLS setup. - grpcservermodule: capture the bound port from AddListeningPort and, when TLS is enabled, fail with FAILED_TO_START_GRPC_SERVER if the bind returns port 0 (e.g. a non-empty but malformed PEM) instead of silently starting a server that does not listen. Non-TLS bind behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds native TLS configuration for OVMS endpoints, focusing on making gRPC serve securely with optional mTLS, while intentionally gating REST HTTPS (fail-closed) until Drogon is built with OpenSSL. This extends the server configuration surface across CLI, internal Config, and the C API, with accompanying validation, tests, and documentation updates.
Changes:
- Introduces gRPC TLS (server TLS + optional mTLS) via new CLI flags,
Configfields/accessors, and gRPC server credential wiring. - Adds REST TLS parameters and Drogon SSL listener wiring, but rejects REST TLS at startup in this build to avoid accidental plaintext.
- Expands C API server settings/setters and adds unit tests/docs covering the new parameters and validation behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/ovmsconfig_test.cpp | Adds config-validation death tests for TLS flag combinations and a positive gRPC TLS config test. |
| src/test/c_api_tests.cpp | Extends C API config tests to cover new TLS fields/setters and parsing constraints. |
| src/ovms.h | Adds C API setter declarations for gRPC/REST TLS certificate/key/CA paths (with REST gated note). |
| src/http_server.cpp | Passes REST TLS paths into the Drogon server constructor (for future HTTPS enablement). |
| src/grpcservermodule.cpp | Builds grpc::SslServerCredentials from configured cert/key/CA to enable TLS/mTLS on gRPC. |
| src/drogon_http_server.hpp | Extends Drogon server constructor to accept TLS material paths. |
| src/drogon_http_server.cpp | Adds Drogon SSL listener wiring (cert/key + optional CA verification) when TLS paths are set. |
| src/config.hpp | Adds Config accessors for TLS certificate/key/CA paths. |
| src/config.cpp | Implements TLS material validation, gates REST TLS, and adds TLS accessor definitions. |
| src/cli_parser.cpp | Adds new CLI options and wiring into ServerSettings for TLS paths. |
| src/capi_frontend/server_settings.hpp | Extends ServerSettingsImpl with gRPC/REST TLS path fields. |
| src/capi_frontend/capi.cpp | Implements new C API setters to populate TLS path fields. |
| docs/security_considerations.md | Updates security guidance to document native gRPC TLS and REST TLS gating. |
| docs/parameters.md | Documents new TLS-related CLI parameters and the REST HTTPS gating behavior. |
- grpcservermodule: wrap the TLS material reads in try/catch so a file
that becomes unreadable between validation and start() returns a
controlled FAILED_TO_START_GRPC_SERVER instead of std::terminate.
- config: TLS path accessors return const std::string& (was const
std::string by value, which inhibits moves and is redundant).
- drogon_http_server: take TLS path params by value (= {}) instead of
const std::string& = "" (defaulting a reference to a temporary is a
footgun); move into members.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements native TLS/HTTPS for the API endpoints (#2144), so traffic can be encrypted without relying on an external reverse proxy (the NGINX sidecar leaves the NGINX↔OVMS hop in plaintext, which is the gap this issue raised).
gRPC TLS — functional
--grpc_certificate_path,--grpc_key_path,--grpc_ca_path.--grpc_ca_path→ mutual TLS: client certificates are required and verified (GRPC_SSL_REQUEST_AND_REQUIRE_CLIENT_CERTIFICATE_AND_VERIFY).grpcservermodulebuildsgrpc::SslServerCredentialsfrom the cert/key/CA. No insecure fallback — a missing/unreadable/empty cert or key aborts startup rather than serving plaintext.REST TLS — gated (fail-closed) in this build
The same
--rest_certificate_path/--rest_key_path/--rest_ca_pathparams and the DrogonaddListenerSSL wiring are implemented, but the bundled Drogon is built without OpenSSL and cannot serve HTTPS. To avoid silently serving plaintext on a port an operator believes is encrypted,Config::validate()rejects any REST TLS parameter at startup with a clear error. The wiring is retained so REST HTTPS activates once Drogon is built with OpenSSL (a build-system follow-up). Use gRPC TLS or terminate REST TLS with a proxy in the meantime.Also
ServerSettingsfields,Configaccessors, and C-API setters (OVMS_ServerSettingsSet{Grpc,Rest}{Cert,Key,Ca}Path) inovms.h.parameters.md(new params) andsecurity_considerations.md(gRPC TLS now native; REST via proxy).Testing
Unit: config-validation death tests for every cert/key/ca combination, missing/empty files, and the REST-gated rejection; C-API setter coverage; a positive gRPC-TLS config test.
Server-only TLS handshake (CPU): started a server with gRPC TLS and verified a real handshake with
openssl s_client— TLS 1.2, server certificate presented.Live mTLS + real inference over TLS (CPU): with an mTLS-configured server (
src/test/dummymodel), generated a CA, a CA-signed server cert (CN=localhost+ SAN), a CA-signed client cert, and an untrusted client cert (different CA), then ran real gRPC/KFS inference for three cases:Server-side handshake logs confirm the rejection reasons, i.e. that
REQUIRE_AND_VERIFYenforces both the require and the verify:This proves mTLS genuinely enforces at runtime and that a real inference request completes end-to-end over the TLS channel — not just the handshake.
Security review: no plaintext fallback when TLS is requested; mTLS genuinely enforces; no key material in logs;
validate()not bypassable.REST TLS is intentionally untested beyond the startup-rejection unit test, since it is gated/fail-closed in this build pending a Drogon-with-OpenSSL build.
Notes