[core] update k8s token cache hashing algorithm to Blake3#58593
[core] update k8s token cache hashing algorithm to Blake3#58593andrewsykim wants to merge 1 commit intoray-project:masterfrom
Conversation
src/ray/thirdparty/sha256.h
Outdated
| unsigned long long bitlen; | ||
| unsigned int state[8]; | ||
| } SHA256_CTX; | ||
| } ray_SHA256_CTX; |
There was a problem hiding this comment.
Rename is requried due to definition conflict w/ openssl libs:
In file included from external/boringssl/src/include/openssl/conf.h:60,
from external/boost/boost/asio/ssl/detail/openssl_types.hpp:23,
from external/boost/boost/asio/ssl/error.hpp:20,
from src/ray/rpc/authentication/k8s_util.cc:19:
external/boringssl/src/include/openssl/base.h:353:32: error: conflicting declaration 'typedef struct sha256_state_st SHA256_CTX'
andrewsykim
force-pushed
the
k8s-auth-sha256
branch
from
c3a8ea3 to
f460909
Compare
andrewsykim
changed the title
andrewsykim
force-pushed
the
k8s-auth-sha256
branch
3 times, most recently
from
94b0d39 to
eb7affc
Compare
andrewsykim
force-pushed
the
k8s-auth-sha256
branch
from
eb7affc to
abd31ab
Compare
Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
andrewsykim
force-pushed
the
k8s-auth-sha256
branch
from
abd31ab to
9d19c55
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ":blake3_avx512", | ||
| ], | ||
| visibility=["//visibility:public"], | ||
| ) |
There was a problem hiding this comment.
Bug: BLAKE3: ARM Builds Lack Performance Optimizations
The BLAKE3 build configuration only includes x86-specific SIMD optimizations (blake3_sse2, blake3_sse41, blake3_avx2, blake3_avx512) with x86-specific compiler flags (-msse2, -msse4.1, -mavx2, -mavx512f). ARM platforms like Apple Silicon or AWS Graviton won't get optimized NEON implementations, resulting in significantly degraded hashing performance. BLAKE3 supports ARM NEON but those implementations aren't included in this build file.
There was a problem hiding this comment.
if this is true then it may lead to errors, we will probably need to support all env's
| @@ -110,9 +114,17 @@ class AuthenticationToken { | |||
| /// Get token hash | |||
| /// @return Hash of the token value | |||
| std::size_t ToHash() const { | |||
There was a problem hiding this comment.
we are using just the first 8 bytes of BLAKE3's 32byte output, this will work fine but now I am wondering if using BLAKE3 is an overkill for this usecase (I thought we wanted to use the entire hash not just a std::size_t so sorry about the confusion)
on reading a bit more on this topic it looks like using BLAKE3 like this can still be a good choice as we can avoid collision attacks, but I am not sure if we want that level of security.
There was a problem hiding this comment.
@richo-anyscale let me know your thoughts on this, if the benefits are not worth the effort I feel we can skip this for now. I am not sure about the effort required for fixing #58593 (comment) so would prefer not to spend time on this if possible
There was a problem hiding this comment.
So I've spent a while thinking about it and I'm just not really worried about this as a sidechannel.
There was a problem hiding this comment.
I tend to agree, will close this PR, we can revisit this in the future
ray-gardener
bot
added
core
| std::size_t hash_value; | ||
| std::memcpy(&hash_value, output, sizeof(hash_value)); | ||
|
|
||
| return hash_value; |
There was a problem hiding this comment.
Should we consider caching this? How frequently will ToHash() get called? The original function likely performs quite a bit faster.
4 participants
Description
Follow-up PR from #58497 to update the hashing algorithm to Blake3. See #58497 (comment) for more details.
Related issues
#58497 (comment)
Additional information