-
Notifications
You must be signed in to change notification settings - Fork 573
Add AuthenticationPolicy proto #335
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.
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
Conversation
mixer/v1/config/client/auth.proto
Outdated
// - end_user: verify end-user credentials. | ||
// For each part, if it's not empty, at least one of those listed credential | ||
// must be provided and (successfully) verified for the authentication to pass. | ||
message AuthenticationPolicy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For new protos, we'd better follow this proto package guide:
I think it's better to be in package "istio.authentication.v1alpha1" than mixer.client
The message name can be shorten to "Policy", "mechanism".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for defining these authentication policy in a new package. istio.mixer.v1.config.client
is really low-level component level config (i.e. envoy filter) and should be decoupled from policy the user writes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Move to authentication/v1alpha1/policy.proto
mixer/v1/config/client/auth.proto
Outdated
// - doesn't contain namespace/domain. All of these are extracted from metadata | ||
// to construct FQDN service name | ||
// - port is part of the identifiers. | ||
message IstioWorkload { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to leave out service/workload scope for now?
If we only use global and namespace scoping:
- max align with k8s model
- easier policy config
- service level isolation can be achieved by per service per namespace.
- issue with non-k8s identity integration
- cannot control over port level. (do we care?)
If we have service/workload level scope,
- need to invent our "service/workload" scope concept + format
- develop complex scoping conflict resolution/validation -- maybe very hard to use.
- additional RBAC if we want to ACL service/workload scope.
A lot of the above points have been discussed extensively. I feel that the gains to introducing service/level scope is very small comparing to the undertakes.
I wonder if we can just start with policy without explicit scoping. If we have to have it later, we can maybe add it by introducing the "binding" config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this to match the policy to the services on which it should be applied. I rename the field so hope it won't be confused with scoping.
mixer/v1/config/client/auth.proto
Outdated
// the whole authentication should fail). | ||
// The validated credential will be used to extract peer identity (i.e the | ||
// source.user attribute in the request to mixer). | ||
repeated AuthenticationMechanism peer = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeated fields must be in purals.
following https://cloud.google.com/apis/design/naming_convention#repeated_field_names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Interesting style difference with google3 :)
@geeknoid This feels like a prime candidate for the new proto packages/directories ( |
mixer/v1/config/client/auth.proto
Outdated
// This field is specific for Envoy proxy implementation. | ||
// Defines the header location to (re)store the authentiated claims. If not | ||
// set, a default value will be used (as of speaking, sec-istio-auth-userinfo) | ||
string output_header_location = 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a hack. we should not need this if Envoy has a way to share request data between different filters.
For now, we have to use http header to pass data between filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep this with the other mixer client config for now, until the whole thing can be relocated/update in a cohesive way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we let users set this? It is our internal plumbing. Users can easily screw us up with typo or mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Wayne, this is implementation detail and should not be user visible.
mixer/v1/config/client/auth.proto
Outdated
// This field is specific for Envoy proxy implementation. | ||
// Defines the header location to store the authentiated claims. If blank, a | ||
// implementation-specific default value will be used. | ||
string output_header_location = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is clearer to have a specific on/off boolean flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
mixer/v1/config/client/auth.proto
Outdated
// must be provided and (successfully) verified for the authentication to pass. | ||
message AuthenticationPolicy { | ||
// If empty, the policy will be applied on all services in the same namespace as the policy. | ||
repeated IstioWorkload target = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "target" should not be part of Policy. It should be part of "Binding" . Please take a look at EndUserAuthenticationPolicySpecBinding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would the policy CRD look like if we use "binding"? Diem gave some examples of the authentication CRDs that users can provide with the proposed proto.
Agree with @douglas-reid, I think we should introduce a new proto package instead of reusing the current mixer/v1/config/client package. We can call it "authentication/v1alpha1". Alternatively we can add "security" prefix to it ("security/authentication/v1alpha1"), in which case, I will move "rbac" package to "security" directory too. |
mixer/v1/config/client/auth.proto
Outdated
// This field is specific for Envoy proxy implementation. | ||
// Defines the header location to (re)store the authentiated claims. If not | ||
// set, a default value will be used (as of speaking, sec-istio-auth-userinfo) | ||
string output_header_location = 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Wayne, this is implementation detail and should not be user visible.
mixer/v1/config/client/auth.proto
Outdated
// Placer holder for mTLS authentication params. | ||
message MutualTLS { | ||
// This field is specific for Envoy proxy implementation. | ||
// Defines the header location to store the authentiated claims. If blank, a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: authentiated -> authenticated.
mixer/v1/config/client/auth.proto
Outdated
// This field is specific for Envoy proxy implementation. | ||
// Defines the header location to store the authentiated claims. If blank, a | ||
// implementation-specific default value will be used. | ||
string output_header_location = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, the claims are stored in a header is really implementation detail, and should not be set by user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. My previous plan is let's user to bind the output of each authentication themselves, and the 'attributes extraction' can retrieve data accordingly. However, we might not need it yet if the mechanism is defined inline in the policy, so let's leave it out for now.
mixer/v1/config/client/auth.proto
Outdated
} | ||
|
||
// AuthenticationMechanism defines one particular type of authentication (i.e | ||
// mutual TLS, JWT etc). The type can be progammatically determine by checking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: determined
It's been said by others already, but it would be useful to create a new package to hold the user-facing authentication policy. We can keep the existing mixer client policy as an implementation detail between Pilot and mixerclient and deal with refactoring that later. Things like |
authentication/v1alpha1/policy.proto
Outdated
} | ||
|
||
message Destination { | ||
// REQUIRED. The name can be a short name or a fully qualified domain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to apply policy at ingress (i.e. gateway)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be, though some mode (e.g mTLS) maybe not applicable
authentication/v1alpha1/policy.proto
Outdated
// address. | ||
// If short names are used, the FQDN of the service will be resolved in a | ||
// platform specific manner. | ||
string name = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this relate to DestinationRule
's name? We've also moved to accepting wildcard domains in many of these fields; is that allowed here too? cc @rshriram @louiscryan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW feels like we need a common type for this, it's popping up a lot. Previously we had IstioService
for this concept, but it went away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, forgot we renamed IstioService to Destination
api/routing/v1alpha2/route_rule.proto
Line 266 in e419233
message Destination { |
authentication/v1alpha1/policy.proto
Outdated
// the type of the "params" field. | ||
message Mechanism { | ||
oneof params { | ||
bool none = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is none
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not an enum so we don't get a default value. However, I think using Empty
would be better. (E.g. what does none: False
even mean?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, what's the difference between an empty (or null) peers
array and a peers
array with none
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None is one kind of authentication. For example, it can be used to define a service to require EUC, except for request come from specific peers. However, I haven't defined the applied condition in this PR yet.
peer:
- mtls:
end_user: - jwt: .....
- none:
in: "service_account_A"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting - thanks for the explanation. It looks like none should really be a specific type and not a boolean or Empty
so it can be extended in the future, e.g.
message None {
// TODO - add conditions to require EUC?
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
authentication/v1alpha1/policy.proto
Outdated
oneof params { | ||
bool none = 1; | ||
MutualTLS mtls = 2; | ||
istio.mixer.v1.config.client.JWT jwt = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you define a new JWT
type under authentication/v1alpha1
for user-facing config with the envoy specific bits stripped out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, this should be elevated up to a higher level; istio.authentication.v1alpha1.JWT
seems reasonable and can be re-used in Mixer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Not sure if I strip out the right fields though. PTAL
authentication/v1alpha1/policy.proto
Outdated
message Policy { | ||
// List of destinations (workloads) that the policy should be applied on. | ||
// If empty, policy will be used on all destinations in the same namespace. | ||
repeated Destination match = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll also need to be able to match on labels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. I'm just not sure mTLS setting in particular can support that.
On the other hand, I really temp to use the route_rule.Destination. Do you think it's a good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that seems most reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
authentication/v1alpha1/policy.proto
Outdated
@@ -0,0 +1,182 @@ | |||
// Copyright 2017 Istio Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2018
authentication/v1alpha1/policy.proto
Outdated
// jwks_uri: "https://www.googleapis.com/oauth2/v1/certs" | ||
// locations: | ||
// - header: x-goog-iap-jwt-assertion | ||
message Policy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rshriram @diemtvu: one more thought: is this something that can/should be part of DestinationRule? Would the configuration be more natural there? We already have other TLS settings there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way we should rationalize the TLS settings there with the ones defined here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I was once told to stay away from DestinationRule :) (we want to separate networking and security). Not sure if the perspective now shifted.
Regardless, I rather keep them separated for now, and see how they evolve and decide later. What do you think?
authentication/v1alpha1/policy.proto
Outdated
// AuthenticationMechanism defines one particular type of authentication (i.e | ||
// mutual TLS, JWT etc). The type can be progammatically determine by checking | ||
// the type of the "params" field. | ||
message Mechanism { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the comment refers to this as "AuthenticationMechanism" but the message name is "Mechanism". We should use one or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change back to AuthenticationMechanism
authentication/v1alpha1/policy.proto
Outdated
} | ||
} | ||
|
||
// AuthenticationPolicy binds credentials to workload(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it AuthenticationPolicy
or Policy
? We should be consistent. I think this should read:
// Policy binds credentials to workload(s), defining an authentication policy.
// A policy is composed of two-part authentication:
// - peers: ...
Related, I think the examples need to be updated to use the plurals (peers
, end_users
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please use camelCase for YAML field names in examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
authentication/v1alpha1/policy.proto
Outdated
message Policy { | ||
// List of destinations (workloads) that the policy should be applied on. | ||
// If empty, policy will be used on all destinations in the same namespace. | ||
repeated istio.routing.v1alpha2.Destination match = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wora should this be plural? I realize that would be inconsistent with other usages of match
in the API, but it matches the style guidelines, I believe.
What is the recommendation here? If it is to prefer match
to matches
, should we update our Style Guide to mention this exception?
authentication/v1alpha1/policy.proto
Outdated
// Policy to enable mTLS, and use JWT for productpage:9000 | ||
// | ||
// apiVersion: config.istio.io/v1alpha2 | ||
// kind: RouteRule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm confused by the CR examples. Should these be:
apiVersion: config.istio.io/**v1alpha1**
kind: **Policy**
?
And if so, do we need to call these AuthenticationPolicy
so that it isn't a generic Policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, cut-and-paste error.
Regarding the proto name, I think it's not necessary to be the same as 'kind', though it seems to be the convention so far (https://github.com/istio/istio/blob/master/pilot/pkg/model/config.go#L307)
Do you have any strong preference?
authentication/v1alpha1/policy.proto
Outdated
// jwks_uri: https://example.com/.well-known/jwks.json | ||
// | ||
message JWT { | ||
// Identifies the principal that issued the JWT. See |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
principal => issuer
authentication/v1alpha1/policy.proto
Outdated
string query = 2; | ||
} | ||
} | ||
repeated Location locations = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the Location message? I think this works just fine:
repeated string jwt_headers;
repeated string jwt_params;
This part is very unlikely to change, so the extra nesting doesn't seem to add much value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ordering important? The previous version searched location in order of appearance.
authentication/v1alpha1/policy.proto
Outdated
message AuthenticationPolicy { | ||
// List of destinations (workloads) that the policy should be applied on. | ||
// If empty, policy will be used on all destinations in the same namespace. | ||
repeated istio.routing.v1alpha2.Destination match = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wora on API guidance for whether its preferred to reuse types and implications on backwards compatibility. If this is a more core type maybe it belongs in a different package?
|
||
import "routing/v1alpha2/route_rule.proto"; | ||
|
||
// $title: Authentication Policy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right.
$title and $overview and $location should be "floating" comments, not attached to any element. So add a blank line right after these definitions.
Second, you should add a user-friendly comment on top of the package definition which will displayed in the docs on istio.io.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
authentication/v1alpha1/policy.proto
Outdated
|
||
option go_package = "istio.io/api/authentication/v1alpha1"; | ||
|
||
// Place holder for None authentication params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Placeholder is one word
// authentication flow. | ||
// | ||
// Example, | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ```yaml around the code block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
authentication/v1alpha1/policy.proto
Outdated
// | ||
message Jwt { | ||
// Identifies the issuer that issued the JWT. See | ||
// https://tools.ietf.org/html/rfc7519#section-4.1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a markdown link:
// The service name will be accepted if audiences is empty. | ||
// | ||
// Example: | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ```yaml and remove extra spaces at the start of the lines in the code block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// | ||
// Examples: | ||
// Policy to enable mTLS for all services in namespace frod | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ```yaml around code block and remove spaces at the start of each line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
authentication/v1alpha1/policy.proto
Outdated
// | ||
// Policy to enable mTLS, and use JWT for productpage:9000 | ||
// | ||
// apiVersion: authentication.istio.io/v1alpha1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ```yaml, etc.
authentication/v1alpha1/policy.proto
Outdated
// will be validated in sequence, until the first one satisfied. If none of | ||
// the specified mechanism valid, the whole authentication should fail. | ||
// On the other hand, the first valid credential will be used to extract | ||
// peer identity (i.e the source.user attribute in the request to mixer). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mixer -> Mixer
authentication/v1alpha1/policy.proto
Outdated
message Policy { | ||
// List of destinations (workloads) that the policy should be applied on. | ||
// If empty, policy will be used on all destinations in the same namespace. | ||
repeated istio.routing.v1alpha2.Destination destinations = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that Destination should be refactor into a istio.common place. then, both route rule and authentication policy reference to it. We should create that common package and copy the Destination message there. And then, another PR to switch route rule to use the new message.
@wora is there an existing pattern we can follow to do thi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I re-introduce the Destination message specific for authN (which we may want a list of ports instead a single one, and probably without label as it's unclear we are able to use that information yet), and waive the refactor decision till later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would I apply policy at the edge of the mesh with destination (i.e. ingress/gateway) or restrict to specific protocols/paths/methods? RouteRule
has some notion of binding to a gateway or host/service with further protocol specific refinement. Do we need something similar here?
authentication/v1alpha1/policy.proto
Outdated
// | ||
// Example: https://securetoken.google.com | ||
// Example: [email protected] | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remote empty comment line
authentication/v1alpha1/policy.proto
Outdated
// audiences: | ||
// - bookstore_android.apps.googleusercontent.com | ||
// bookstore_web.apps.googleusercontent.com | ||
// jwks_uri: https://example.com/.well-known/jwks.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camel case yaml field names: jwksUri
// Google service account). | ||
// | ||
// Example: https://www.googleapis.com/oauth2/v1/certs | ||
string jwks_uri = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is copied over from the Google API auth protos but do we know why is this jwks
and not jwtks
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik, the name is based on OpenID spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jwks_uri
is also used in OpenID spec (see https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata)
* cluster configuration * nits
authentication/v1alpha1/policy.proto
Outdated
// Policy to enable mTLS, and use JWT for productpage:9000 | ||
// | ||
// ```yaml | ||
// apiVersion: authentication.istio.io/v1alpha1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, only do one or the other: either have ```yaml
blocks, or indent by four spaces, but not both.
Move template and valuetype to mixer/adapter/model/v1beta
Ideally, we do want to use the same notion, but I have a hard time to see
how exactly all pieces fit altogether, given they are all work-in-progress.
I'd like to wait a bit until all implementation details settle down on that
side. Does this make sense?
…On Tue, Feb 6, 2018 at 11:07 AM, Jason Young ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In authentication/v1alpha1/policy.proto
<#335 (comment)>:
> +// port:
+// number: 9000
+// peers:
+// - mtls:
+// endUsers:
+// - jwt:
+// issuer: "https://securetoken.google.com"
+// audiences:
+// - "productpage"
+// jwksUri: "https://www.googleapis.com/oauth2/v1/certs"
+// locations:
+// - header: x-goog-iap-jwt-assertion
+message Policy {
+ // List of destinations (workloads) that the policy should be applied on.
+ // If empty, policy will be used on all destinations in the same namespace.
+ repeated istio.routing.v1alpha2.Destination destinations = 1;
How would I apply policy at the edge of the mesh with destination (i.e.
ingress/gateway) or restrict to specific protocols/paths/methods?
RouteRule has some notion of binding to a gateway or host/service with
further protocol specific refinement. Do we need something similar here?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#335 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AX99cZAa4obn_kf9EqoVwZHa9CGtyd_Cks5tSKL2gaJpZM4RzSWG>
.
--
Diem Vu | Software Engineer | [email protected] | +1 408-215-8127
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
@diemtvu: The following test failed, say
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. I understand the commands that are listed here. |
I did something bad in the last rebase(s) so I create new PR #361 to replace this. |
Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of mixerclient This PR will be merged automatically once checks are successful. ```release-note none ```
Initial API for Authentication policy. In this version, the policy is able to:
The policy has not yet include 'impersonation' rules, and other options to conditionally activate authentication mechanism (e.g enable/disable a particular end-user auth based on peer identity).
Example specs for use cases:
and
Using either mTLS or JWT for service-to-service
Design doc: https://docs.google.com/document/d/1ezP4UuOn3JXEs_cXW4GyPGq-Ppq_XhS9-M-lN6ocOA4/edit?ts=5a70c798#heading=h.vel7gfeap92a