Jwt_authn: first draft of Http filter implementation.#3339
Jwt_authn: first draft of Http filter implementation.#3339htuch merged 33 commits intoenvoyproxy:masterfrom
Conversation
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Some initial comments, nice to see this coming together.
There was a problem hiding this comment.
Can you import (or export) these under some prefixed path?
There was a problem hiding this comment.
This seems a general HTTP or core utility to have in Envoy.
There was a problem hiding this comment.
Is there an RFC for canonical URI parsing to reference?
There was a problem hiding this comment.
Is the assumption here that the cluster will be configured for TLS? I'd be interesting in seeing some way to validate or warn if this isn't the case, since it would be a pretty bad misconfiguration to fetch certs or public keys without TLS.
There was a problem hiding this comment.
Normally, public key servers only support TLS. Users have to config cluster with TLS.
There was a problem hiding this comment.
I'm OK with saying this, but I think it would make sense to document with a strong warning here. It would be pretty scary to get this one wrong.
There was a problem hiding this comment.
Is the cache only for preconfigured keys or is there an ability to dynamically add to it? Please clarify in comments, thanks.
qiwzhang
changed the title
qiwzhang
force-pushed
the
jwt_authn_all0
branch
3 times, most recently
from
9cb883e to
e60c317
Compare
There was a problem hiding this comment.
Nit: any chance we can drop the include/ part of this? It's just aesthetics, but yeah..
source/common/http/utility.cc
Outdated
There was a problem hiding this comment.
Nit: make these temporary variables const.
source/common/http/utility.cc
Outdated
There was a problem hiding this comment.
Just a thought; would a regex possibly be clearer?
There was a problem hiding this comment.
I think regex is much slower than string.find()
There was a problem hiding this comment.
Maybe; but if you precompile the regex it could be fairly reasonable, see the ongoing discussion in #3269 (comment).
source/common/http/utility.cc
Outdated
There was a problem hiding this comment.
Needs a bunch of test covering expected and corner cases.
There was a problem hiding this comment.
Given the above cache check is only for pre-configured JWKS, should we have a TODO here to add a cache for the fetchRemoteJwks() result? If we have two back-to-back requests for the same remote JWKS, I imagine we don't want to make two HTTP calls?
There was a problem hiding this comment.
I'm OK with saying this, but I think it would make sense to document with a strong warning here. It would be pretty scary to get this one wrong.
There was a problem hiding this comment.
Nit: You can bring std::string body into here and make it a const std::string
There was a problem hiding this comment.
@saumoh this is similar to what you do in ext_authz. Is there a bug here or is this a simpler way of looking at the side-call state?
There was a problem hiding this comment.
Envoy doesn't allow static non-POD (like Google3), consider using ConstSingleton or the like.
There was a problem hiding this comment.
Prefer constexpr and Envoy style naming such as PubKeyCacheExpirationSec for constant scalars.
There was a problem hiding this comment.
Who is a qualified person to review this JWKS specific logic?
There was a problem hiding this comment.
This is NOT the JWKS specific. This is just our implementation based on the experience from Google cloud endpoint.
Updated the comment.
There was a problem hiding this comment.
Nit: please use explicit sized types, e.g. uint32_t as per Envoy style, or if you're being compatible with an existing API, something like size_t as determined by the STL API.
There was a problem hiding this comment.
Can you confirm that you've verified full test coverage on the new code?
There was a problem hiding this comment.
No, I did not have full test converge for the new code yet. I am still adding new tests into this PR.
There was a problem hiding this comment.
I think you should push almost all of this code to jwt_verifier_lib, it doesn't have to belong to Envoy code base. The only dependency seems to be the configuration, but it is only audiences (vector) and jwks (string) so you should be able to pass by parameter.
There was a problem hiding this comment.
I thought about it. It is NOT just a pure cache. It is a cached Jwks + issuer_config. For each request, we need to access per-issuer config, such as payload_header, and we also need to access cached Jwks. We only need to do one map lookup by issuer if we combine jwks with issuer_config. That is the current JwksCache design.
If we split into a separate pure JwksCache, then, I agree with you, it can be moved into Jwt_verify_lib.
Current design is try to reduce a map lookup by issuer.
There was a problem hiding this comment.
issue config is just audience + jwks, no?
There was a problem hiding this comment.
also "forward_jwt" and "payload_header".
On second throught, I may go with your suggestion, it is cleaner. I will create a config_map to lookup config by issuer, and jwks_cache. If we use unorderred_map, lookup is O(1), it is OK to lookup twice.
There was a problem hiding this comment.
I tried, but decided to keep the current shape as: with new JwtAuthn config change (#3381), audience checking may not be in JwksCache object. Further re-factory is needed to support the config change.
test/common/http/utility_test.cc
Outdated
There was a problem hiding this comment.
You probably want to test more corner cases, e.g. scheme://adf-scheme://adf, :// or /:/adsfetc. How do you handle invalid URIs as user input?
There was a problem hiding this comment.
Added some corner cases. Also updated the header file to say this function will NOT validate URI.
There was a problem hiding this comment.
Should this be bool okToBypass() const; ?
There was a problem hiding this comment.
How is in-progress fetching possible given we only get here via decodeHeaders? Should this just be an ASSERT?
There was a problem hiding this comment.
Updated the comment
There was a problem hiding this comment.
Nit: Be consistent in using : as a separator between these lines.
There was a problem hiding this comment.
Is there a better name than factory here? E.g. ThreadLocalDataStore?
There was a problem hiding this comment.
Rename it to Config
qiwzhang
force-pushed
the
jwt_authn_all0
branch
5 times, most recently
from
a343c49 to
f949bdf
Compare
source/common/http/utility.cc
Outdated
There was a problem hiding this comment.
What about URIs with ports in them, e.g. http://foo.com:1234/foo? I think in general URIs get pretty complicated, for example you can encode usernames and passwords in them. This is kind of why I was in favor of having some standard library for this. If the limitations here are acceptable for your needs, that's fine, just document them.
There was a problem hiding this comment.
returned host and path are used for message->headers() Host() and Path() field for http async client->Send() call. It is OK for host to have port. Updated comment for it
There was a problem hiding this comment.
Nit: remove gratuitous blank line.
There was a problem hiding this comment.
@saumoh can you comment on how this relates to the state machine and handling done for ext authz?
There was a problem hiding this comment.
Sorry, copy & paste error. changed.
There was a problem hiding this comment.
Probably cleaner to work with proto DurationUtil, e.g. DurationUtil::durationToMilliseconds.
There was a problem hiding this comment.
Please add a one-liner // This test validates foo and bar. above each TEST_F.
There was a problem hiding this comment.
Can you recommend someone who can review the JWT business logic in these tests?
There was a problem hiding this comment.
Actually these two tests are covered by jwt_verify_lib. So remove them here.
There was a problem hiding this comment.
I can't spot this on the CI coverage report (https://53860-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.html), do you know why?
There was a problem hiding this comment.
Your coverage maybe from other PR coverage tests. I don't know how to check show coverage from PR coverage test. But from the test log it has jwks_cache.html which should reflect jwks_cache code coverage.
|
@htuch please hold for another round of review. I am going to add filter_test and integration_test. After that will ping you for another round of review. |
qiwzhang
force-pushed
the
jwt_authn_all0
branch
2 times, most recently
from
fdc2506 to
7585069
Compare
source/common/http/utility.cc
Outdated
There was a problem hiding this comment.
Use HeaderMapImpl::appendToHeader?
There was a problem hiding this comment.
Sorry, this is not my change. I may "rebase" wrong change. Will fix it.
There was a problem hiding this comment.
can we make this part an utility function for HttpUri, similar to Grpc::Common::prepareHeaders.
|
@qiwzhang I'm taking another look. FYI for the future, it would be super helpful to reviewers if you avoided doing forced pushes, which it looks like you have been doing, since it's hard to spot the delta since the last round of comments. |
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
3 participants
Signed-off-by: Wayne Zhang qiwzhang@google.com
Description:
This is first draft of porting jwt authentication filter from istio/proxy to Envoy.
Risk Level: Low