Add JWT generation for API server authentication#170
Conversation
avalerio-tkd
commented
Nov 24, 2025
avalerio-tkd
commented
- Adding module to read credentials on API server side via application keys.
- Adding endpoint on API server to wire the /auth endpoint
- Generates session JWTs for API client calls to the DBPS server.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Overall LGTM. Left a couple of comments, no need for a new PR when/if addressed.
src/server/auth_utils.h
Outdated
|
|
||
| // Note for Protegrity integration: | ||
| // - This is a simplified Authentication module used to complete the client integration. | ||
| // - It is fully functional, but for production deployment a fully pledged Identity Provider can be used instead. |
There was a problem hiding this comment.
typo: I think you mean 'fledged' (instead of 'pledged')
src/server/auth_utils.h
Outdated
| // - It is fully functional, but for production deployment a fully pledged Identity Provider can be used instead. | ||
| // | ||
| // TODO: Expand explanation below. | ||
| // - Note that no Arrow codebase changes would be needed to replace this with a fully pledged Identity Provider |
src/server/dbps_api_server.cpp
Outdated
| const std::string credentials_file_path = "credentials.json"; // Default path | ||
| if (!credential_store.init(credentials_file_path)) { | ||
| std::cout << "Warning: Failed to load credentials file: " << credentials_file_path << std::endl; | ||
| std::cout << "Server will continue to run, but credentials will not be validated." << std::endl; |
There was a problem hiding this comment.
I think this should be a hard fail - instead of a soft fail where all requests may end up being rejected.
There was a problem hiding this comment.
Done. Moved it to a hard fail
src/server/dbps_api_server.cpp
Outdated
| }); | ||
|
|
||
| // Authentication endpoint - POST /auth | ||
| CROW_ROUTE(app, "/auth").methods("POST"_method)([&credential_store](const crow::request& req) { |
There was a problem hiding this comment.
nit: is /auth standard? meaning? from where I sit, /auth may mean "give me permissions" or "please verify my permissions".
I'm OK to keep as is - mainly a question.
There was a problem hiding this comment.
I've used it like that in the past, but we can rename if it's confusing.
There was a problem hiding this comment.
Renamed the endpoint to /token for clarity
- Added additional tests.
|
@argmarco-tkd thanks for the review. Added the JWT verification on the /encrypt and /decrypt endpoints. Could you PTAL? |
src/server/auth_utils.cpp
Outdated
| void ClientCredentialStore::init(const std::map<std::string, std::string>& credentials) { | ||
| credentials_.clear(); | ||
| credentials_ = credentials; | ||
| skip_credential_check_ = false; // Default: check credentials |
There was a problem hiding this comment.
nit: why in negative? :) (again, a nit)
There was a problem hiding this comment.
Thanks. Will update that.
…y users. - Added more tests.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
LGTM - thanks for this!
|
@argmarco-tkd made a bit of rework to hide the details of the authentication params from the library users to make any future changes more clean. There aren't functionality changes, but just in case, if you want to take another look. If not, I'll merge after retesting it. Thanks for the reviews! |
- Add JWT secret key to command line options.
2 participants
