Add subject_alt_names field in ServiceEntry#785
Conversation
googlebot
added
the
cla: yes
| // of namespace names. | ||
| repeated string export_to = 7; | ||
|
|
||
| // The reunion of service accounts associated with workloads |
There was a problem hiding this comment.
It might be helpful to describe what exact form this should take. It came up in the meeting today and I suspect there will be confusion for folks going forward. Would be good to try to make it clear now.
There was a problem hiding this comment.
@smawson - For TOC FYI even though this is not a user-visible API change, it affects contract between Pilot and Galley or other service registries
This should be marked $hidden_from_docs for the moment.
For documentation
"The set of service accounts identities allowed for workloads that implement this service. This information is used to enforce secure-naming (link https://istio.io/docs/concepts/security/#secure-naming)"
There was a problem hiding this comment.
Done, added also an example with SA in SPIFEE format, ptal.
| // serviceAccounts: | ||
| // - "spiffe://cluster.local/ns/httpbin-ns/sa/httpbin-service-account" | ||
| // ``` | ||
| repeated string service_accounts = 8; |
There was a problem hiding this comment.
Usual bike-shedding: service account is a k8s term.
We have the same setting in DestinationRule -> TLSSettings -> SubjectAltNames
The actual use is to indicate what SANs are exposed and/or should be expected by the endpoints.
Technically we can achieve the same by creating a synthetic DestinationRule, but it's getting too complicated.
There was a problem hiding this comment.
Makes sense to use the same nomenclature with existing, let me change it.
There was a problem hiding this comment.
I changed the name.
We need to clarify what is the behavior if the user also configures a DestinationRule for the same service, with a different list of subject_alt_names. Do we want to override or merge? Override may cause auth failures, so merge seems to make more sense. Thoughts?
andraxylia
changed the title
|
Needs to override the synthetic service entry for sure - DR indicates what
user explicitly expects the service to authenticate as.
Or better - Galley could validate the endpoints, and if they don't run as
one of the identities declared in the DR, don't add it to the set/warn.
Also extra care on the namespace - the current scheme of using whatever the
deployment is running as secure identity only works because
in practice all deployments and SA in a namespace are mostly equivalent
from security perspective ( with current K8S implementation of SA).
If the SA in ServiceEntry is not constrained to the same NS - we make a
huge hole. This is super security-sensitive, I'm not even sure having
full spiffe URL is acceptable since it bypasses the namespace boundary.
…On Mon, Feb 4, 2019 at 7:55 PM Andra Cismaru ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In networking/v1alpha3/service_entry.proto
<#785 (comment)>:
> + // spec:
+ // hosts:
+ // - httpbin.com
+ // location: MESH_INTERNAL
+ // ports:
+ // - number: 80
+ // name: http
+ // protocol: HTTP
+ // resolution: STATIC
+ // endpoints:
+ // - address: 2.2.2.2
+ // - address: 3.3.3.3
+ // serviceAccounts:
+ // - "spiffe://cluster.local/ns/httpbin-ns/sa/httpbin-service-account"
+ // ```
+ repeated string service_accounts = 8;
I changed the name.
We need to clarify what is the behavior if the user also configures a
DestinationRule for the same service, with a different list of
subject_alt_names. Do we want to override or merge? Override may cause auth
failures, so merge seems to make more sense. Thoughts?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#785 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6m8tzLwcTK5uyxoj81PxEZp5_-l4ks5vKQCggaJpZM4aiKic>
.
|
| // - address: 3.3.3.3 | ||
| // subjectAltNames: | ||
| // - "spiffe://cluster.local/ns/httpbin-ns/sa/httpbin-service-account" | ||
| // ``` |
There was a problem hiding this comment.
You cannot add yamls and other stuff in the field docs. It will mess up the UI. Move this example and text to the area above message ServiceEntry. And keep the documentation here to a minimum, describing the intent of the field.
|
while in general this is okay, its instructive to understand why the exact same change 6 months ago (2ba6be9) with the same intent (of syncing service model with service entry) was objected to, by messers @costinm and @louiscryan :). |
|
See above - the secure naming scheme is not very secure if anyone who can
run a workload or create a ServiceEntry can modify the list of 'trusted
identities' of a service.
This is a very security-sensitive change - for the synthetic entries
generated by galley it might be fine if Galley is making sure they are
restricted to the namespace.
Allowing them in user-specified SE without equivalent validation - I hope
not.
This should not be a user-visible config.
…On Tue, Feb 5, 2019 at 7:20 AM Shriram Rajagopalan ***@***.***> wrote:
while in general this is okay, its instructive to understand why the exact
same change 6 months ago (2ba6be9
<2ba6be9>)
with the same intent (of syncing service model with service entry) was
objected to, by messers @costinm <https://github.com/costinm> and
@louiscryan <https://github.com/louiscryan> :).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#785 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6jTaGcdO9Ef59w3UgebuvFEbbTLdks5vKaEmgaJpZM4aiKic>
.
|
|
Galley can create the subject_all_names by using the namespace of the service : "spiffe://cluster.local/ns/httpbin-ns/sa/httpbin-service-account". The list of endpoints for that service can only belong in the same namespace as the service in k8s, otherwise the pods would not be selected. Since ServiceEntries can come from other MCP sources than Galley, same restriction applies. |
+1 |
|
/lgtm |
|
@andraxylia: you cannot LGTM your own PR. DetailsIn response to this:
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. |
|
@nmittler: changing LGTM is restricted to assignees, and only istio/api repo collaborators may be assigned issues. DetailsIn response to this:
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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andraxylia, nmittler, ozevren 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 |
|
@wenchenglu This is needed for istio/istio#10589 |
10 participants
As discussed in istio/istio#11293 (comment)
The subject_alt_names field in ServiceEntry contains the reunion of all SAs from all workloads that are backends for that Service. Each MCP source computes the reunion and sends it to Pilot. Pilot merges the service_accounts corresponding to that service from all MCP sources, and maps the result to subject_alt_names field in the Envoy CDS for that ServiceEntry.
cc @nmittler