jwt: make max token size configurable #38461
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
Thanks!
Left minor comments about the API.
Assigning @yanavlasov as codeowner
/assign @yanavlasov
| @@ -380,6 +380,9 @@ message JwtProvider { | |||
| message JwtCacheConfig { | |||
| // The unit is number of JWTs, default to 100. | |||
| uint32 jwt_cache_size = 1; | |||
|
|
|||
| // Max token size in bytes, default to 4096. | |||
There was a problem hiding this comment.
Can you please write the units (I'm assuming "bytes").
Also it is unclear to me if this is the max token size or max cache size (for all tokens). If it is the latter, I suggest renaming the field's name. If the former, this seems like a non-typical cache use, so I suggest adding comments how this cache is working, and making sure the field's comment reflects the correct behavior.
Please also add whether 0 is a valid value, and if so, what is the expected behavior in this case (or consider adding a minimum value constraint for the field).
There was a problem hiding this comment.
Can you please write the units (I'm assuming "bytes").
"Max token size in bytes" - isn't it clear that the unit is byte?``
Also it is unclear to me if this is the max token size
Ok, I changed the comment, I hope it's clear now that it applies to a single token.
| @@ -65,6 +72,7 @@ class JwtCacheImpl : public JwtCache { | |||
| private: | |||
| std::unique_ptr<SimpleLRUCache<std::string, ::google::jwt_verify::Jwt>> jwt_lru_cache_; | |||
| TimeSource& time_source_; | |||
| unsigned int max_jwt_size_for_cache_; | |||
| if (!jwt_lru_cache_ || token.size() > std::numeric_limits<unsigned int>::max()) { | ||
| return; | ||
| } | ||
| if (static_cast<unsigned int>(token.size()) <= max_jwt_size_for_cache_) { |
There was a problem hiding this comment.
This implies that only certain tokens will be cached (not a very intuitive cache use-case). Can you please clarify the comments in the API, and explicitly call that the max value is inclusive.
There was a problem hiding this comment.
Ok, added a comment to the proto file.
Signed-off-by: Jacek Ewertowski <[email protected]>
Signed-off-by: Jacek Ewertowski <[email protected]>
Signed-off-by: Jacek Ewertowski <[email protected]>
Signed-off-by: Jacek Ewertowski <[email protected]>
jewertow
force-pushed
the
jwt-max-token-size
branch
from
356ce23 to
b45fa86
Compare
| @@ -56,7 +60,10 @@ class JwtCacheImpl : public JwtCache { | |||
| } | |||
|
|
|||
| void insert(const std::string& token, std::unique_ptr<::google::jwt_verify::Jwt>&& jwt) override { | |||
| if (jwt_lru_cache_ && token.size() <= kMaxJwtSizeForCache) { | |||
| if (!jwt_lru_cache_ || token.size() > std::numeric_limits<unsigned int>::max()) { | |||
There was a problem hiding this comment.
| if (!jwt_lru_cache_ || token.size() > std::numeric_limits<unsigned int>::max()) { | |
| if (!jwt_lru_cache_ || token.size() > std::numeric_limits<uint32_t>::max()) { |
FWIW, I don't think the right-hand-side can be triggered, as AFAIK size() return type is size_t which should map to the same type.
There was a problem hiding this comment.
Sorry for that leftover, I fixed it.
I don't think the right-hand-side can be triggered, as AFAIK size() return type is size_t
Thanks for this hint. Actually, this check is unrealistic. Theoretically token.size() could be bigger than the max value of int32_t, but I do not believe that it may ever happen - a token would have to be bigger than 4Gb (4294967295 bytes). Do you think this check makes sense? Or is it better to assume that it won't happen and check static_cast<uint32_t>(token.size()) > max_token_size?
Signed-off-by: Jacek Ewertowski <[email protected]>
Commit Message: jwt: make max token size configurable
Additional Description:
Risk Level: low
Testing: unit tests added
Docs Changes: protobuf comment
Release Notes: added
Fixes: #32790