add api-key parameter #3690

dtrawins · 2025-10-08T11:07:54Z

🛠 Summary

JIRA/Issue if applicable.
Describe the changes.

🧪 Checklist

Unit tests added.
The documentation updated.
Change follows security best practices.
``

mzegla · 2025-10-08T14:07:40Z

src/http_rest_api_handler.cpp

    registerHandler(V3, [this](const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, HttpResponseComponents& response_components, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser) -> Status {
        OVMS_PROFILE_FUNCTION();
-        return processV3(uri, request_components, response, request_body, std::move(serverReaderWriter), std::move(multiPartParser));
+        return processV3(uri, request_components, response, request_body, std::move(serverReaderWriter), std::move(multiPartParser), api_key);


Suggested change

return processV3(uri, request_components, response, request_body, std::move(serverReaderWriter), std::move(multiPartParser), api_key);

return processV3(uri, request_components, response, request_body, std::move(serverReaderWriter), std::move(multiPartParser), apiKey);

mzegla · 2025-10-08T14:07:51Z

src/http_rest_api_handler.cpp

 }

-Status HttpRestApiHandler::processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser) {
+Status HttpRestApiHandler::processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& api_key) {


Suggested change

Status HttpRestApiHandler::processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& api_key) {

Status HttpRestApiHandler::processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& apiKey) {

mzegla · 2025-10-08T14:08:40Z

src/http_rest_api_handler.cpp

+        std::transform(lowercaseKey.begin(), lowercaseKey.end(), lowercaseKey.begin(), 
+                        [](unsigned char c){ return std::tolower(c); });
+    }
+    if (!api_key.empty()) {


Suggested change

if (!api_key.empty()) {

if (!apiKey.empty()) {

mzegla · 2025-10-08T14:09:28Z

src/http_rest_api_handler.cpp

+    }
+    if (!api_key.empty()) {
+        if (request_components.headers.count("authorization")) {
+            if (request_components.headers.at("authorization") != "Bearer " + api_key) {


Suggested change

if (request_components.headers.at("authorization") != "Bearer " + api_key) {

if (request_components.headers.at("authorization") != "Bearer " + apiKey) {

Is exact match safe here? No risk of some additional whitespaces?

mzegla · 2025-10-08T14:09:39Z

src/http_rest_api_handler.cpp

+    if (!api_key.empty()) {
+        if (request_components.headers.count("authorization")) {
+            if (request_components.headers.at("authorization") != "Bearer " + api_key) {
+                SPDLOG_DEBUG("Unauthorized request - invalid API key {} instead of {}", request_components.headers.at("authorization"), api_key);


Suggested change

SPDLOG_DEBUG("Unauthorized request - invalid API key {} instead of {}", request_components.headers.at("authorization"), api_key);

SPDLOG_DEBUG("Unauthorized request - invalid API key {} instead of {}", request_components.headers.at("authorization"), apiKey);

mzegla · 2025-10-08T14:09:52Z

src/http_rest_api_handler.hpp

    Status processServerMetadataKFSRequest(const HttpRequestComponents& request_components, std::string& response, const std::string& request_body);

-    Status processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser);
+    Status processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& api_key);


Suggested change

Status processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& api_key);

Status processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& apiKey);

mzegla · 2025-10-08T14:10:00Z

src/http_rest_api_handler.hpp

+    Status processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& api_key);
    Status processListModelsRequest(std::string& response);
    Status processRetrieveModelRequest(const std::string& name, std::string& response);
+    const std::string api_key;


Suggested change

const std::string api_key;

const std::string apiKey;

mzegla · 2025-10-08T14:10:19Z

src/server.cpp


+std::string Server::getAPIKey() const {
+    return apiKey;
+    }


Suggested change

}

}

Copilot

Pull Request Overview

This PR adds API key authentication support for generative endpoints in the HTTP REST API. The implementation introduces an optional authentication mechanism that validates Bearer tokens against a configured API key.

Adds API key configuration parameter and server-level storage
Implements Bearer token validation in V3 HTTP endpoints
Adds unauthorized status code for invalid authentication attempts

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/status.hpp	Adds UNAUTHORIZED status code enum
src/status.cpp	Adds status message mapping for unauthorized requests
src/server.hpp	Adds API key storage and getter method
src/server.cpp	Implements API key initialization and getter
src/http_rest_api_handler.hpp	Updates processV3 signature to accept API key parameter
src/http_rest_api_handler.cpp	Implements Bearer token validation logic
src/config.hpp	Adds API key getter method to Config class
src/config.cpp	Implements API key getter method
src/cli_parser.cpp	Adds API key command line parameter parsing
src/capi_frontend/server_settings.hpp	Adds API key field to server settings

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-08T14:11:52Z

src/server.hpp

    Status startModules(ovms::Config& config);
    void shutdownModules();
+    std::string getAPIKey() const;
+    std::string setAPIKey(const std::string& newApiKey);


The setAPIKey method is declared but never implemented. Either implement this method or remove the declaration if API key modification after initialization is not intended.

Suggested change

std::string setAPIKey(const std::string& newApiKey);

std::string setAPIKey(const std::string& newApiKey) {

std::unique_lock<std::shared_mutex> lock(modulesMtx);

std::string oldApiKey = apiKey;

apiKey = newApiKey;

return oldApiKey;

}

HttpServerModule has access to ovms::config in HTTPServerModule::start(ovms::Config)
Either extend ovms::createAndStartDrogonHttpServer() with additional parameter or wrap
apiKey together with restBindAddress,restPort, workersCount in helper struct nested inside server settings (HTTPServerConfig?)

Copilot · 2025-10-08T14:11:52Z

src/http_rest_api_handler.cpp

+    // convert headers to lowercase because http headers are case insensitive
+    for (const auto& [key, value] : request_components.headers) {
+        std::string lowercaseKey = key;
+        std::transform(lowercaseKey.begin(), lowercaseKey.end(), lowercaseKey.begin(),
+            [](unsigned char c) { return std::tolower(c); });
+    }


The loop converts header keys to lowercase but doesn't store the results anywhere. The lowercaseKey variable is not used, so the header lookup will still be case-sensitive. Either store the lowercase headers in a new map or perform case-insensitive lookup directly.

Copilot · 2025-10-08T14:11:52Z

src/http_rest_api_handler.cpp

+        if (request_components.headers.count("authorization")) {
+            if (request_components.headers.at("authorization") != "Bearer " + api_key) {


Header lookup uses exact case 'authorization' but HTTP headers are case-insensitive. This will fail if the client sends 'Authorization' (capitalized). Use case-insensitive header lookup or convert the headers map to lowercase keys.

atobiszei · 2025-10-13T08:27:21Z

docs/security_considerations.md

-docker run --rm -d --user $(id -u):$(id -g) --read-only --tmpfs /tmp -v ${PWD}/models/:/models -p 9178:9178 openvino/model_server:latest \
--model_path /models/resnet/ --model_name resnet --port 9178
+```
+docker run --rm -d --user $(id -u):$(id -g) --read-only --tmpfs /tmp -v ${PWD}/models/:/models -p 9000:9000 openvino/model_server:latest \


Suggested change

docker run --rm -d --user $(id -u):$(id -g) --read-only --tmpfs /tmp -v ${PWD}/models/:/models -p 9000:9000 openvino/model_server:latest \

docker run --rm -d --user $(id -u):$(id -g) --read-only --tmpfs /tmp -p 9000:9000 openvino/model_server:latest \

Mount is not needed at all here?

atobiszei · 2025-10-13T08:31:24Z

src/test/http_openai_handler_test.cpp

+        std::string absoluteApiKeyPath = std::filesystem::absolute(apiKeyFile).string();
+        ofs << "1234";
+        ofs.close();
+        ::SetUpServer(this->t, this->server, this->port, configPath, 10, absoluteApiKeyPath);


Suggested change

::SetUpServer(this->t, this->server, this->port, configPath, 10, absoluteApiKeyPath);

randomizeAndEnsureFree(this->port);

::SetUpServer(this->t, this->server, this->port, configPath, 10, absoluteApiKeyPath);

atobiszei · 2025-10-13T08:45:36Z

src/cli_parser.cpp

+            if (file.is_open()) {
+                std::getline(file, serverSettings.apiKey);
+                serverSettings.apiKey.erase(serverSettings.apiKey.find_last_not_of(" \n\r\t") + 1);
+                file.close();


Suggested change

file.close();

done automatically in destructor.

atobiszei · 2025-10-13T08:46:05Z

src/cli_parser.cpp

+                serverSettings.apiKey.erase(serverSettings.apiKey.find_last_not_of(" \n\r\t") + 1);
+                file.close();
+            } else {
+                throw std::runtime_error("Unable to open API key file: " + api_key_file.string());


Can't we just cerr here & exit, instead of throw & instant catch?

atobiszei · 2025-10-13T08:49:17Z

src/cli_parser.cpp

+        if (envApiKey != nullptr) {
+            serverSettings.apiKey = envApiKey;
+        }
+        if (serverSettings.apiKey.empty()) {


Do we intent to print this message in both cases (empty file with api key & empty env)?

atobiszei · 2025-10-13T09:00:05Z

src/http_rest_api_handler.cpp

+    std::unordered_map<std::string, std::string> lowercaseHeaders;
+    lowercaseHeaders = toLowerCaseHeaders(request_components.headers);
+    if (!api_key.empty()) {
+        if (lowercaseHeaders.count("authorization")) {
+            if (lowercaseHeaders.at("authorization") != "Bearer " + api_key) {
+                SPDLOG_DEBUG("Unauthorized request - invalid API key {} instead of {}", lowercaseHeaders.at("authorization"), api_key);
+                return StatusCode::UNAUTHORIZED;
+            }
+        } else {
+            SPDLOG_DEBUG("Unauthorized request - missing API key");
+            return StatusCode::UNAUTHORIZED;
+        }
+    }


Wrap into Status checkIfAuthorized()

atobiszei · 2025-10-13T09:02:04Z

src/http_rest_api_handler.cpp

+    lowercaseHeaders = toLowerCaseHeaders(request_components.headers);
+    if (!api_key.empty()) {
+        if (lowercaseHeaders.count("authorization")) {
+            if (lowercaseHeaders.at("authorization") != "Bearer " + api_key) {


at() could throw. Use find & check instead

add api-key parameter

4e9df48

dtrawins requested a review from atobiszei October 8, 2025 11:08

style

9bb4cac

mzegla reviewed Oct 8, 2025

View reviewed changes

mzegla requested a review from Copilot October 8, 2025 14:10

Copilot AI reviewed Oct 8, 2025

View reviewed changes

dtrawins added 5 commits October 10, 2025 17:59

update

197109f

style

137a7bd

windows

c335a5a

unit tests

d9d22b0

fix test windows

3af935a

dtrawins marked this pull request as ready for review October 11, 2025 21:05

atobiszei reviewed Oct 13, 2025

View reviewed changes

	return processV3(uri, request_components, response, request_body, std::move(serverReaderWriter), std::move(multiPartParser), api_key);
	return processV3(uri, request_components, response, request_body, std::move(serverReaderWriter), std::move(multiPartParser), apiKey);

	Status HttpRestApiHandler::processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& api_key) {
	Status HttpRestApiHandler::processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& apiKey) {

	if (request_components.headers.at("authorization") != "Bearer " + api_key) {
	if (request_components.headers.at("authorization") != "Bearer " + apiKey) {

	SPDLOG_DEBUG("Unauthorized request - invalid API key {} instead of {}", request_components.headers.at("authorization"), api_key);
	SPDLOG_DEBUG("Unauthorized request - invalid API key {} instead of {}", request_components.headers.at("authorization"), apiKey);

	Status processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& api_key);
	Status processV3(const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser, const std::string& apiKey);

-    std::string setAPIKey(const std::string& newApiKey);
+    std::string setAPIKey(const std::string& newApiKey) {
+        std::unique_lock<std::shared_mutex> lock(modulesMtx);
+        std::string oldApiKey = apiKey;
+        apiKey = newApiKey;
+        return oldApiKey;
+    }

		if (request_components.headers.count("authorization")) {
		if (request_components.headers.at("authorization") != "Bearer " + api_key) {

	docker run --rm -d --user $(id -u):$(id -g) --read-only --tmpfs /tmp -v ${PWD}/models/:/models -p 9000:9000 openvino/model_server:latest \
	docker run --rm -d --user $(id -u):$(id -g) --read-only --tmpfs /tmp -p 9000:9000 openvino/model_server:latest \

	::SetUpServer(this->t, this->server, this->port, configPath, 10, absoluteApiKeyPath);
	randomizeAndEnsureFree(this->port);
	::SetUpServer(this->t, this->server, this->port, configPath, 10, absoluteApiKeyPath);

add api-key parameter #3690

Are you sure you want to change the base?

add api-key parameter #3690

Conversation

dtrawins commented Oct 8, 2025

🛠 Summary

🧪 Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants