Skip to content

authn: revised request authenticator #2952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

authn: revised request authenticator #2952

wants to merge 2 commits into from

Conversation

@Shikugawa
Copy link
Member

@Shikugawa Shikugawa commented Jul 28, 2020

What this PR does / why we need it:
This is a part of replacement of authn wasm implementation. It is using revised request authentication API. istio/api#1536
#2800 is too large and hard to review all codes so that I split all of authn wasm implementation.

@Shikugawa Shikugawa requested review from a team July 28, 2020 14:49
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jul 28, 2020
@istio-testing istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 28, 2020
@lizan lizan self-assigned this Jul 29, 2020
yangminzhu
yangminzhu reviewed Jul 31, 2020
View reviewed changes
Copy link
Contributor

@yangminzhu yangminzhu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the work! this looks great!

I left some comments but I think many of them could be addressed in follow up PRs so feel free to leave a TODO there. One of big benefits of the new authn filter is to reduce the complexity and tech-debt we had today, so before we really switch to use it, we would need to address these TODOs otherwise we're just carrying the tech-debt to the new filter (and also need some test for performance and reliability).

cc @liminw @incfly

extensions/authn/filter_context.cc
return true;
}

bool FilterContext::getJwtPayloadFromIstioJwtFilter(
Copy link
Contributor

@yangminzhu yangminzhu Jul 30, 2020

Choose a reason for hiding this comment

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

I think we no longer need this. We only support Envoy JWT filter now.

extensions/authn/request_authenticator.h
namespace Extensions {
namespace AuthN {

class IRequestAuthenticator {
Copy link
Contributor

@yangminzhu yangminzhu Jul 31, 2020

Choose a reason for hiding this comment

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

nit: I feel it's more common to name the abstract class as RequestAuthentication, and the implementation as RequestAuthenticationImpl in Envoy?

extensions/authn/request_authenticator.cc
}

bool RequestAuthenticator::validateJwt(istio::authn::JwtPayload* jwt) {
for (const auto& jwt_rule : request_authentication_policy_.jwt_rules()) {
Copy link
Contributor

@yangminzhu yangminzhu Jul 31, 2020

Choose a reason for hiding this comment

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

I feel this could be greatly simplified without depending on the request_authentication policy at all.

Istiod already generates the Envoy JWT filter config from consolidated RequestAuthN policies, Envoy JWT filter should validate and output the JWT token to the metadata (via PayloadInMetadata)

can we just pass a list of issuers in the authn filter's config instead of passing the whole RequestAuthN policy? (I want to avoid dependency on the user-facing API in proxy)

extensions/authn/filter_context.cc
// TODO (pitlv2109): Return protobuf Struct instead of string, once Istio jwt
// filter is removed. Also need to change how Istio authn filter processes the
// jwt payload.
MessageToJsonString(entry_it->second.struct_value(), payload);
Copy link
Contributor

@yangminzhu yangminzhu Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

As you can see there is a TODO here, here we parse the struct to string, and later in ExtractOriginalPayload and ProcessJwtPayload we parse the string into JSON object via JsonParse(), is it possible to avoid this and use the struct directly?

kyessenov
kyessenov reviewed Jul 31, 2020
View reviewed changes
extensions/authn/filter_context.h
public:
FilterContext(
const envoy::config::core::v3::Metadata& dynamic_metadata,
const RequestHeaderMap& header_map, const Connection* connection,
Copy link
Contributor

@kyessenov kyessenov Jul 31, 2020

Choose a reason for hiding this comment

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

This seems to be using Envoy interfaces. How are you going to compile this to Wasm?

Copy link
Member Author

@Shikugawa Shikugawa Jul 31, 2020

Choose a reason for hiding this comment

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

I'm not considering about wasm compilation in this PR. I will submit a PR which will include plugin.h/.cc using proxy-wasm interface. I will fix them in that.

Copy link
Contributor

@kyessenov kyessenov Jul 31, 2020

Choose a reason for hiding this comment

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

I'm just wondering that if we're revising the implementation anyways, why not write it once with Wasm ABI.

Copy link
Member Author

@Shikugawa Shikugawa Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This PR doesn't include the build components which will be required to build AuthN filter. I'd like to split AuthN wasm implementations described in #2800. If we consider about wasm build in this PR. It will be complex to review all of codes, and out of request authenticator context. We have had already the implementation to be available not NullVM in #2800. I think that we should focus on simple (not WASM enabled) request authenticator implementation and unit test.

kyessenov
kyessenov reviewed Jul 31, 2020
View reviewed changes
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

I strongly suggest using Wasm interfaces. We cannot upstream this filter so it will need to be dynamically loadable.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 8, 2020
@istio-testing
Copy link
Collaborator

istio-testing commented Nov 8, 2020

@Shikugawa: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kyessenov
Copy link
Contributor

kyessenov commented Nov 16, 2020 via email

@zirain
Copy link
Member

zirain commented Oct 27, 2022

outdated

@zirain zirain closed this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kyessenov kyessenov kyessenov left review comments

+1 more reviewer

@yangminzhu yangminzhu yangminzhu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lizan lizan

@yangminzhu yangminzhu

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Shikugawa @istio-testing @kyessenov @zirain @yangminzhu @lizan @googlebot