Skip to content

jwt_authn: write JWT payload to dynamic metadata#4698

Closed
qiwzhang wants to merge 3 commits intoenvoyproxy:masterfrom
qiwzhang:jwt_dynamic_metadadta
Closed

jwt_authn: write JWT payload to dynamic metadata#4698
qiwzhang wants to merge 3 commits intoenvoyproxy:masterfrom
qiwzhang:jwt_dynamic_metadadta

Conversation

@qiwzhang
Copy link
Copy Markdown
Contributor

Signed-off-by: Wayne Zhang qiwzhang@google.com

Description:

Use dynamicMetadata in the StreamInfo to pass all successfully verified JWT payloads to other HTTP filters.

Risk Level: Low
Testing: Add unit-tests

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang qiwzhang requested a review from lizan as a code owner October 12, 2018 00:03
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
Copy link
Copy Markdown
Contributor Author

@diemtvu and @potatop could you help review this changes?

void storeAuth(AuthenticatorPtr&& auth) { auths_.emplace_back(std::move(auth)); }

// Add a pair of (issuer, payload), called by Authenticator
void addPaylod(const std::string& issuer, const std::string& payload) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addPayload

curr_token_->removeJwt(*headers_);
}
if (set_payload_cb_ && provider.payload_in_metadata()) {
set_payload_cb_(provider.issuer(), jwt_->payload_str_base64url_);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set the non-decoded version? It save a call to base64 decode in the consumer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually can we parse the JSON payload into a ProtobufWkt::Struct? otherwise you won't be able to match each claim in RBAC.

Also rather than issuer as key, provider_name would be better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For allow_missing_or_fail rule, it doesn't need to specify provider, so there is not provider_name. In theory, there could be two ProviderInfo with same same issuer. It is a useful feature for normal cases. But for allow_missing_or_fail case, we choose the first one matched with the issuer from the JWT token, it is better not to have two providers points to the same issuer in the config.
Overall, writing issuer works for all cases, but not provider name.

curr_token_->removeJwt(*headers_);
}
if (set_payload_cb_ && provider.payload_in_metadata()) {
set_payload_cb_(provider.issuer(), jwt_->payload_str_base64url_);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually can we parse the JSON payload into a ProtobufWkt::Struct? otherwise you won't be able to match each claim in RBAC.

Also rather than issuer as key, provider_name would be better.

Verifier::Callbacks* callback_;
std::unordered_map<const Verifier*, CompletionState> completion_states_;
std::vector<AuthenticatorPtr> auths_;
std::vector<std::pair<std::string, std::string>> payload_pairs_;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use ProbotufWkt::Struct here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking for a utility code to convert JSON to protobuf::Struct. Do you know any?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just use MessageUtil::loadFromJson() to convert it to Struct. Not sure what it will look like?

@lizan
Copy link
Copy Markdown
Member

lizan commented Oct 12, 2018

Please fix build issue.

@lizan
Copy link
Copy Markdown
Member

lizan commented Oct 12, 2018

sorry the build issue might not be due to master failure, fix coming in #4701

@alyssawilk
Copy link
Copy Markdown
Contributor

Merged - mind pulling master to pick up the fix?

@qiwzhang
Copy link
Copy Markdown
Contributor Author

Sure

@qiwzhang qiwzhang closed this Oct 12, 2018
@qiwzhang
Copy link
Copy Markdown
Contributor Author

Sorry, I pushed a wrong button to close it.
@lizan @alyssawilk could you re-open it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants