redis: constant-time compare for downstream auth password#45472
Conversation
|
Hi @jmestwa-coder, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
jmestwa-coder
temporarily deployed
to
external-contributors
GitHub Actions
Inactive
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Redis proxy filter to use a constant-time comparison via OpenSSL's CRYPTO_memcmp for checking downstream auth passwords, which helps prevent timing attacks. The review feedback recommends using angle brackets instead of double quotes for the external OpenSSL header inclusion to follow standard C++ style guidelines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| #include "source/common/config/datasource.h" | ||
| #include "source/common/config/utility.h" | ||
|
|
||
| #include "openssl/mem.h" |
There was a problem hiding this comment.
External dependency headers (such as OpenSSL) should be included using angle brackets (<...>) rather than double quotes ("...") to adhere to standard C++ include style guidelines.
#include <openssl/mem.h>
#include <openssl/sha.h>
There was a problem hiding this comment.
done, switched it to angle brackets and pushed.
|
waiting for addressing feedback /wait |
jmestwa-coder
force-pushed
the
redis-auth-consttime
branch
from
a3a066c to
f5fc593
Compare
|
please fix format /wait |
Signed-off-by: Syed Mohammed Nayyar <jmestwa@gmail.com>
jmestwa-coder
force-pushed
the
redis-auth-consttime
branch
from
f5fc593 to
b807607
Compare
|
format fixed and pushed. the openssl include was sorting after the source/ block; moved it up to the angle-bracket .h group so it lands before the stdlib headers, matching gcp_authn/crypto_utils.cc. clang-format is clean now. |
Conflicts were confined to the RESP3-extended client factory lineage and resolved by adopting upstream envoyproxy#45607 std::optional/std::nullopt spelling while keeping the RESP3 additions: - ClientFactory/ClientImpl create + ctor signatures keep the appended upstream_protocol_version and OptRef<Stats::Counter> hello-failure params (OptRef args stay absl::nullopt per OptRef convention) - ClientFactoryImpl::create keeps the unconditional initialize() (init routing moved inside ClientImpl by the RESP3 init state machine) - proxy_filter.cc keeps both new includes: upstream <openssl/mem.h> (envoyproxy#45472 constant-time password compare) and RESP3 <algorithm> - HEXPIRE (envoyproxy#44887) and cluster string_view (envoyproxy#45967) merged cleanly Signed-off-by: Doogie Min <doogie.min@sendbird.com>
2 participants
compare the AUTH password against each configured downstream password with CRYPTO_memcmp in checkPassword instead of std::string ==, which short-circuits on the first mismatch and leaks the configured password by timing over the network.