implement periodic JWKs fetcher/updater #150
Conversation
Shikugawa
force-pushed
the
jwk-fetcher
branch
2 times, most recently
from
213939c to
d49a447
Compare
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
| ioc_(io_context), | ||
| duration_(duration), | ||
| timer_(ioc_, duration_) { | ||
| timer_.async_wait( |
There was a problem hiding this comment.
This PR does not include the usage of dynamic jwk resolver yet. but we should be caurefull: initialize the filter/filter chain/authservice and mark it complete only when we get a valid jwks for the first time. otherwise we could receive ExternalAuthz.Check request before jwks is initalized.
wondering how that would be done. hopefully you already got some solution/idea. :)
There was a problem hiding this comment.
I don't think we should support to track existing dynamic JWKs state (such as valid, not valid or not initialized) here. It is because if we don't have valid JWKs, the ext_authz request will be rejected. Envoy will receive kicked request with invalid JWKs no matter when we have invalid (or not initialized) JWKs. Could you show the detailed pitfalls when we won't do that?
There was a problem hiding this comment.
After authservice started, when the first check request comes, it may or may not have jwks fetched ready. So the request may or may not goes through. This won't cause any security problem. but it makes system less predictable from an end user perspective.
Plus this can happen when the authservices are doing a rolling restart. Without this, k8s switched to the new pods right away, even the new pod may not be ready yet (lacking jwks). So the valid requests can fail.
We could add a http endpoint, /readyz, returning 200 only when the jwks is resolved. And configure k8s readiness probe using that. With this, k8s won't swap the new authservice pods util it says it's ready to serve.
No action item on this PR.
|
Sure. Sgtm
…On Thu, Aug 26, 2021, 11:43 PM Rei Shimizu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/filters/oidc/jwks_resolver.h
<#150 (comment)>
:
> +
+#include <memory>
+
+#include "absl/synchronization/mutex.h"
+#include "jwt_verify_lib/jwks.h"
+#include "src/common/http/http.h"
+
+namespace authservice {
+namespace filters {
+namespace oidc {
+
+class JwksResolver {
+ public:
+ virtual ~JwksResolver() = default;
+
+ virtual void updateJwks(const std::string& new_jwks,
I think we can avoid this updateJwks because updateJwks is called by only
JwksFetcher. we can achieve that just to pass DynamicJwksResolverImpl to
the constructor of JwksFetcher :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#150 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMVXB7HQRFPFO3SCHOUMULT64CXNANCNFSM5CH3IOLA>
.
|
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
| setExpectedRemoteJwks(*mock_http, valid_jwt_public_key2_); | ||
| io_context.run_for(std::chrono::seconds(3)); | ||
| EXPECT_EQ(google::jwt_verify::Status::Ok, resolver.jwks()->getStatus()); | ||
| EXPECT_EQ(valid_jwt_public_key2_, resolver.rawStringJwks()); |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: incfly, Shikugawa The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Implement to fetch JWKs periodically and update them.