[Core] Add authentication token logic and related tests

[sampan-s-nayak]

Description

this pr sets up the helper classes and utils to enable token based authentication for ray core rpc calls.

Related issues

NA

Additional information

[cursor]

Bug: Whitespace Handling Causes Undefined Behavior

The TrimWhitespace function relies on undefined behavior when an input string contains only whitespace. If find_last_not_of returns std::string::npos, adding 1 to it causes integer wraparound, leading to erase(0). While this happens to produce the correct empty string, the underlying logic is fragile.

 

[cursor]

Bug: Environment Variables Reference Invalid Memory

On Windows, _putenv is called with temporary strings, such as in set_env_var and the USERPROFILE setup. This results in environment variables pointing to invalid memory after the temporary string is destroyed.

Additional Locations (1)

• src/ray/rpc/tests/authentication_token_loader_test.cc#L112-L113

 

[gemini-code-assist]

Code Review

This pull request introduces token-based authentication for Ray Core RPCs, which is a great security enhancement. The changes include a secure AuthenticationToken class, a loader for tokens from various sources, and related configuration. The implementation is well-structured and includes comprehensive tests.

I've found a critical issue in the test suite on Windows related to environment variable handling that needs to be addressed. I've also left a few other comments for improving code style, efficiency, and robustness. Overall, this is a solid contribution.

[gemini-code-assist]

The Windows implementation of set_env_var has a critical bug. The _putenv function does not copy the string it's given; it only stores the pointer. Here, env_str is a local variable, so the pointer passed to _putenv becomes dangling as soon as this function returns, leading to undefined behavior.

The same issue exists in unset_env_var (lines 119-126) and SetUp (lines 52-64) for the _putenv call.

To fix this, the string buffer must remain valid for the duration of its use in the environment. A good approach for this test class would be to store these environment strings in a std::vector<std::string> member of the AuthenticationTokenLoaderTest fixture.

Example fix for set_env_var:

1. Add std::vector<std::string> managed_env_strings_; to the test fixture.
2. Modify set_env_var like this:

#ifdef _WIN32
    managed_env_strings_.emplace_back(std::string(name) + "=" + std::string(value));
    _putenv(managed_env_strings_.back().c_str());
#else
    setenv(name, value, 1);
#endif

You'll need to apply similar fixes to unset_env_var and SetUp.

[gemini-code-assist]

The implementation for stripping the "Bearer " prefix can be made more robust and readable. Using sizeof(kBearerPrefix) - 1 is a C-style idiom that can be brittle if the constant definition changes. A more modern C++ approach using std::string_view's properties would be safer. Also, the size check can be simplified for better clarity.

Suggested change

	const std::string_view prefix(kBearerPrefix, sizeof(kBearerPrefix) - 1);
	if (metadata_value.size() <= prefix.size() ||
	metadata_value.substr(0, prefix.size()) != prefix) {
	return AuthenticationToken(); // Invalid format, return empty
	const std::string_view prefix(kBearerPrefix);
	if (metadata_value.size() < prefix.size() ||
	metadata_value.substr(0, prefix.size()) != prefix) {
	return AuthenticationToken(); // Invalid format, return empty
	}

[gemini-code-assist]

These headers appear to be unused in this file. It's good practice to remove unnecessary includes to improve compilation times and reduce dependencies.

[gemini-code-assist]

Creating a std::string from a const char* just to check its length is inefficient as it can cause a memory allocation. You can check if the C-style string is non-empty more efficiently.

Suggested change

	if (env_token != nullptr && std::string(env_token).length() > 0) {
	if (env_token != nullptr && *env_token != '\0') {

[gemini-code-assist]

Creating a std::string from a const char* just to check its length is inefficient as it can cause a memory allocation. You can check if the C-style string is non-empty more efficiently.

Suggested change

	if (env_token_path != nullptr && std::string(env_token_path).length() > 0) {
	if (env_token_path != nullptr && *env_token_path != '\0') {

[gemini-code-assist]

Hardcoding temporary paths like /tmp/ and C:\\Windows\\Temp\\ can make tests brittle, as the user running them may not have write permissions. It's better to use a standard temporary directory. You're already using TEST_TMPDIR in SetUp, which is a good practice to follow here as well. Using . as a fallback on Windows is safer than C:\\Windows\\Temp.

  std::string get_temp_token_path() {
    const char *tmp_dir_from_env = std::getenv("TEST_TMPDIR");
#ifdef _WIN32
    const char *tmp_dir = tmp_dir_from_env ? tmp_dir_from_env : ".";
    return std::string(tmp_dir) + "\\ray_test_token_" + std::to_string(_getpid());
#else
    const char *tmp_dir = tmp_dir_from_env ? tmp_dir_from_env : "/tmp";
    return std::string(tmp_dir) + "/ray_test_token_" + std::to_string(getpid());
#endif
  }

[cursor]

Bug: Whitespace Handling Bug in TrimWhitespace Function

The TrimWhitespace function incorrectly handles strings containing only whitespace. When find_last_not_of returns std::string::npos after leading whitespace is removed, adding 1 causes integer wraparound. This leads to an erase call that fails to clear the string, leaving it with whitespace instead of empty.