Support inline jwks from config#1476
Conversation
| jwt_config_.local_jwks().specifier_case() == | ||
| ::envoy::api::v2::core::DataSource::kInlineString) { | ||
| Status status = | ||
| SetKey(Base64::decode(jwt_config_.local_jwks().inline_string())); |
There was a problem hiding this comment.
So do you expect the key is base64 encoded?
There was a problem hiding this comment.
does it mean pilot side needs encoding the key ?
There was a problem hiding this comment.
Yes, Envoy is doing a lot of JsonConvert for filter config. We could not just put json as value here
There was a problem hiding this comment.
Please don't do this, if you want inline them with base64 in YAML/JSON, just use inline_bytes, by proto/json spec they are base64 encoded. Don't mix them up.
| audiences_.insert(SanitizeAudience(aud)); | ||
| } | ||
|
|
||
| if (jwt_config_.has_local_jwks() && |
There was a problem hiding this comment.
Instead of the whole block here, just use Envoy::Config::DataSource::read and you're good with all case.
| const auto& duration = jwt_config_.remote_jwks().cache_duration(); | ||
| expiration_time_ += std::chrono::seconds(duration.seconds()) + | ||
| std::chrono::nanoseconds(duration.nanos()); | ||
| if (jwt_config_.has_remote_jwks()) { |
There was a problem hiding this comment.
I'd change SetKey parameter and add a const time_point& expiration, instead of check local/remote here and above.
|
|
||
| auto inline_jwks = Config::DataSource::read(jwt_config_.local_jwks(), true); | ||
| if (!inline_jwks.empty()) { | ||
| Status status = SetKey(Base64::decode(inline_jwks), |
There was a problem hiding this comment.
I think it is still better to base64 encod it. Here is my concern, Envoy is using JsonConvert() to convert Struct or Protobuf to other config proto. If we don't base64 encode jwks, jwks is a JSON, it will break. It is safer to base64 encode the jwks.
There was a problem hiding this comment.
No please dont, it won't break JSON encoding, a string will be escaped correctly. If you want base64 encoded one, just use inline_bytes, which is by proto spec. You don't want to load base64 encoded from DataSource file. please revert.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JimmyCYJ The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Support sending jwks in the config.
Issue: https://github.com/istio/proxy/issues/1373
Release note: