Add Jwt cache.

[mk46]

Signed-off-by: Manish Kumar manish.kumar1@india.nec.com

Commit Message: Add Jwt cache.
Additional Description: Added Jwt cache, which makes Jwt verification faster.
Risk Level:
Testing: Format and unit
Docs Changes: done
Release Notes: done
Fixes #12644

[mk46]

Hi @qiwzhang PTAL, I am not sure where to call addTokenResult and findTokenResult.

[qiwzhang]

The cache should be:

// Map a token to its result in the cache
absl::flat_hash_map<string,  TokenResult> token_cache_;

Struct TokenResult {
     // cache expire time, default cache for 5 minutes
     MonotonicTime expiration_time_;
     // verification status
     Status  status_;
}

In the jwt_authn config, in the JwtProvider config, add

   // enable cache 
   bool enable_token_cache ;

  // specify token cache during,  default to 5 minutes
   Duration  token_cache_duration;

[yuval-k]

should the cache have a (optionally) limited size to cap the amount of memory it can use?

[yuval-k]

ahh nvm, i saw you commented on that.

[mk46]

Done.

[qiwzhang]

My proposed change only skip jwt::verify part with cache. We still need to parse jwt. I am not sure how fast the jwt parsing which is base64url_decode() of 3 sections.

Maybe we should.

Some other issuers we need to work on:

1. should a cache item be expired? Maybe not. But we need to handle "exp" and "nbf" correctly. Since we cache parsed jwt result, even for cache hit, we should always check its time constraint. For a "nbf" failed token, once time comes, it is valid again, we should verify it and cache the verify result.

2. cache size should not keep growing, otherwise we will have OOO. How, and when do we purge cache? Should we use LRU?

[qiwzhang]

BTW, there is a LRU cache implementation here. We can copy the code here

A good example of jwt_cache code using that LRU cache is here

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14341 was synchronize by mk46.

see: more, trace.

[lizan]

BTW, there is a LRU cache implementation here. We can copy the code here

A good example of jwt_cache code using that LRU cache is here

Can we have this LRU as a part of jwt-verify-lib? That makes license and ownership clearer.

[qiwzhang]

@mk46 Please copy simple_lru code to jwt_verify_lib repo and keep their copyright info? Thanks

[qiwzhang]

I think it is quite slow to parsing JWT token (split sections and base64_decode). I like jwt_cache to cache the parsing result too.

So here is how it should work:

struct TokenCacheData {
   // jwt parsing status
   Status  jwt_status;
   // parsed JWT
   std::unique_ptr<Jwt>  jwt_;
   // If valid, the  verification status
   optional<Status> sig_status;
};

We don't need "cache_duration" config. Usually, a token has "exp" field for 1 hour, it is removed from the cache when it is expired. For parsing failed token, it doesn't have expiration time, it will be purged by LRU. If a token doesn't have "exp", it never expire, it is OK to keep it in the cache until purged by LRU.

So the data follow is:

1) for a token, check its cache, if cache miss, do the normal parsing, verification, and cache its result
2) for a cache hit, check its `jwt_status`,  if it is OK,   verifyTimeConstraint of its jwt.   If it is expired, remove it from the cache
3) if sig_status is invalid, verify it, otherwise use sig_sattus

Hi @mk46 what do you think?

[mk46]

SGTM, We should keep cache_duration in the config and will be evaluated like min(exp, cache_duration) if given otherwise exp.

[qiwzhang]

SG for "cache_duration" config. If specified, it can apply to parsing failed token too.

[mk46]

I think we should leave it for a failed token while parsing. Since the probability of hitting the same failed token will be very less. Please LMK if you still want to keep it.

[qiwzhang]

OK, if we don't cache parsing failed tokens, the data flow could be much simpler. Thanks

[wrowe]

/lgtm deps

[wrowe]

@htuch please reconfirm for api and your asks, and I'm happy to merge.

[mk46]

@lizan Please take a look. Thanks!

[htuch]

/lgtm api

[wrowe]

Lizan's changes seem to be addressed, /lgtm deps

[moderation]

/lgtm deps

@wrowe for repokitteh to pick up the deps approval it needs to be on it's own line

All threads resolved, dismissing per @htuch on slack