honour subject alt names from service entry #32878

ramaraochavali · 2021-05-14T08:46:57Z

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[X] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Pull Request Attributes

Please check any characteristics that apply to this pull request.

[] Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

hzxuzhonghu · 2021-05-17T01:43:26Z

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

@@ -775,6 +776,11 @@ func (cb *ClusterBuilder) buildUpstreamClusterTLSContext(opts *buildClusterOpts,
 				CaCertificatePath: tls.CaCertificates,
 			}

+			// Use service accounts in service if TLS settings does not have subject alt names for service entries.
+			if opts.serviceRegistry == string(serviceregistry.External) && len(tls.SubjectAltNames) == 0 {
+				tls.SubjectAltNames = opts.serviceAccounts


Can you move the tls setting to buildAutoMtlsSettings? It belongs there.

I do not think it is autoMTLS related. For user specified TLS settings also for service entries if subject alt names are provided there and not in DR - we should handle it. This actually belongs here.
And also we need to handle for TLS SIMPLE as well.

Actually，in some case the service accounts are already set there. We may need to rename that function

Here it is not service accounts. It is actually subject alt names given in service entry. During conversion it is stored in service https://github.com/istio/istio/blob/master/pilot/pkg/serviceregistry/serviceentry/conversion.go#L220 and we copy it to opts. So opts.serviceAccounts is actually sANs for SEs - which is confusing but not changed in this PR

I think both sa and SubjectAltNames are all used to build same field. They should be set when complete tls.

Also we have passed tls setting to buildUpstreamClusterTLSContext, if we set tls again in some case, this is very coupling with buildAutoMtlsSettings

buildAutoMtlsSettings as is meant today only deals with auto mTLS cases. If we have to do what you are saying we need to change that buildMtlsSettings and take care of SIMPLE also. I can try to refactor that later - but IMO it is not related to this PR.

We do set tls like this - in this method already.
if len(tlsContext.Sni) == 0 {
tlsContext.Sni = c.cluster.Name
}

howardjohn · 2021-05-17T15:39:34Z

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

@@ -775,6 +776,11 @@ func (cb *ClusterBuilder) buildUpstreamClusterTLSContext(opts *buildClusterOpts,
 				CaCertificatePath: tls.CaCertificates,
 			}

+			// Use service accounts in service if TLS settings does not have subject alt names for service entries.


I am a bit confused why we don't need this for the L765 branch?

I did not know that credential name uses the subject alt names. I think it needs to be added

howardjohn · 2021-05-17T15:40:03Z

do we have a test for when SAN is set but cacertificates is not?

ramaraochavali · 2021-05-17T15:48:37Z

do we have a test for when SAN is set but cacertificates is not?

I think we do not validate that case. Do you mean to say we validate that add a test case?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2021-05-18T01:52:37Z

@howardjohn Think about the "SAN is set but cacertificates is not" - we can not validate that in validation webhook because the SAN and cacertificate can come from two different CRDs. So we will have to think about validating the code. While it is related to this issue - but not caused by this fix (it is latent). I am wondering we should get this in and add that validation in a separate PR? WDYT?

@hzxuzhonghu For your point about merging this to buildAutoMTLS - I think if we refactor it to call buildClientTLSSettings and set every thing there - that would work. To simplify backporting of this bug to previous releases, I will do a follow-up once this is merged to refactor that part. WDYT

howardjohn · 2021-05-18T02:04:22Z

I just want to make sure we have a test case covering it, not that user facing validation does.

hzxuzhonghu · 2021-05-18T03:15:39Z

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

@@ -760,7 +762,10 @@ func (cb *ClusterBuilder) buildUpstreamClusterTLSContext(opts *buildClusterOpts,
 			CommonTlsContext: &auth.CommonTlsContext{},
 			Sni:              tls.Sni,
 		}
-
+		// Use service accounts in service if TLS settings does not have subject alt names for service entries.
+		if opts.serviceRegistry == string(serviceregistry.External) && len(tls.SubjectAltNames) == 0 {


TBH, just feels very weird it has to be aware of the serviceregistry when building cluster tls context.

That is because of the API we had in service entry - If it is not there we did not have to use it - And we can not set service accounts as subject alt names in all cases (for example user specified TLS etc...). Do you have a better suggestion to handle this?

Thinking about this - I can think of couple of options

https://github.com/istio/api/blob/master/networking/v1alpha3/destination_rule.proto#L961 - instead of override if we append the service entry SANs and DR SANs it might work and we do not need registry check. This is already being done for workload selector based WLE https://github.com/istio/api/blob/master/networking/v1alpha3/service_entry.proto#L953

Add a specific field in SubjectAltNames which would be populated from ServiceEntry and use it here i.e. do not reuse ServiceAccounts for SubjectAltNames

ramaraochavali · 2021-05-18T04:12:19Z

@howardjohn I thought you are looking for validation. We do have a test that ignores SAN and sets up empty validation context when cacertificate is not specified https://github.com/istio/istio/blob/master/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go#L1774

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2021-05-18T07:41:10Z

/test gencheck_istio

ramaraochavali · 2021-05-19T03:54:31Z

@howardjohn @hzxuzhonghu can you PTAL when you get chance?

hzxuzhonghu · 2021-05-19T07:45:39Z

After a retrospect of the original issue, when the user specifies no SubjectAltNames in tls setting, personally I am more for the user settings (in dr) override the ServiceEntry one.

The fix here is a（dr.ClientTLSSettings） || b(se.SubjectAltNames). , the confusion is when user specifies both a and b simultaneously, only b a takes preference, in this case b is unrespected at all.

Edited: a takes preference

ramaraochavali · 2021-05-19T07:54:36Z

the confusion is when user specifies both a and b simultaneously, only b takes preference, in this case b is unrespected at all.

only DR takes precedence (user specified) right? That is what this PR does

If there are DR settings use them
if there no DR settings, look at service entry and use them if it has
If none of them have we do not.
Are you thinking different should happen?

hzxuzhonghu · 2021-05-19T09:25:33Z

If there are DR settings use them
if there no DR settings, look at service entry and use them if it has
If none of them have we do not.

For the second, user explicitly specifies DestinationRule.TLSSetting（which contains no subjectAltNames, i am thinking it means not to verify server certificate san. I am not sure which is the right behavior, no docs say that.

ramaraochavali · 2021-05-19T09:49:08Z

https://github.com/istio/api/blob/master/networking/v1alpha3/service_entry.proto#L953 - for WLEs that have workload selectors we are actually appending SANs given in SE with service accounts. IMO we should do the same for SE/DR. Combine them and present in the validation context. No need to override one with the other. Then code also becomes simpler :-)

howardjohn · 2021-05-19T15:36:18Z

One small issue may be that if we merge them then there is no way for the DR to say "do not verify any SANs". I suppose they can just not set caCertificates though

howardjohn · 2021-05-19T15:36:26Z

cc @jacob-delgado @Kmoneal

ramaraochavali · 2021-05-20T00:49:06Z

I think specifying empty SAN in DR is not possible even today for service entries. If it is empty we will just bring it from SE. So I think appending does not create that new problem especially for service entries. Are you thinking about regular services? For regular services if it is SIMPLE of MUTUAL we do not add service accounts and for ISTIO_MUTUAL we can combine them. That way we do not have any issue. WDYT? So if you all agree appending is better I will change the PR and update api doc accordingly

jacob-delgado · 2021-05-20T19:28:37Z

Do we intend to fix this in 1.9 and 1.10?

ramaraochavali · 2021-05-21T04:01:10Z

My plan is to fix in Master first and then possibly backport to 1.10? Any preferences?

ramaraochavali · 2021-05-22T06:09:06Z

@howardjohn @hzxuzhonghu How do we move forward with this?

We have couple of options

Get this merged. This implements what is currently documented. I understand there are implementation concerns that it uses serviceregistry but thats unavoidable given how the api is documented today.
We can change the interpretation of api and may be implement properly.

I think we should go with #1 for now and discuss about changing interpretation of api and implementation as a separate issue.

ramaraochavali · 2021-05-23T05:59:41Z

/test all

ramaraochavali · 2021-05-23T06:53:31Z

/test integ-pilot-multicluster-tests_istio

ramaraochavali · 2021-05-25T07:07:51Z

@howardjohn @hzxuzhonghu gentle ping

ramaraochavali · 2021-05-29T11:46:42Z

@howardjohn @hzxuzhonghu gentle ping

ramaraochavali · 2021-06-02T04:28:39Z

/retest

nrjpoddar

Thanks @ramaraochavali for this fix!

Kmoneal · 2021-06-02T20:10:16Z

/cherry-pick release-1.10

istio-testing · 2021-06-02T20:11:48Z

@Kmoneal: #32878 failed to apply on top of branch "release-1.10":

Applying: honour subject alt names from service entry
Using index info to reconstruct a base tree...
M	pilot/pkg/networking/core/v1alpha3/cluster.go
M	pilot/pkg/networking/core/v1alpha3/cluster_builder.go
M	pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go
M	pilot/pkg/networking/core/v1alpha3/cluster_test.go
M	pilot/pkg/serviceregistry/serviceentry/conversion_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceentry/conversion_test.go
Auto-merging pilot/pkg/networking/core/v1alpha3/cluster_test.go
Auto-merging pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go
Auto-merging pilot/pkg/networking/core/v1alpha3/cluster_builder.go
Auto-merging pilot/pkg/networking/core/v1alpha3/cluster.go
CONFLICT (content): Merge conflict in pilot/pkg/networking/core/v1alpha3/cluster.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 honour subject alt names from service entry
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.10

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.

istio-testing · 2021-06-02T20:11:50Z

@Kmoneal: new issue created for failed cherrypick: #33235

In response to this:

/cherry-pick release-1.10

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.

Kmoneal · 2021-06-02T20:12:13Z

/cherry-pick release-1.9

howardjohn · 2021-06-02T20:12:24Z

This feels like a big behavioral change for a patch release. Not sure cherrypick is safe

…

On Wed, Jun 2, 2021 at 1:10 PM Kmoneal ***@***.***> wrote: /cherry-pick release-1.10 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32878 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXKBOHNX7WYDZP4EAN3TQ2F3NANCNFSM444BGW5Q> .

istio-testing · 2021-06-02T20:12:49Z

@Kmoneal: #32878 failed to apply on top of branch "release-1.9":

Applying: honour subject alt names from service entry
Using index info to reconstruct a base tree...
M	pilot/pkg/networking/core/v1alpha3/cluster.go
M	pilot/pkg/networking/core/v1alpha3/cluster_builder.go
M	pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go
M	pilot/pkg/networking/core/v1alpha3/cluster_test.go
M	pilot/pkg/serviceregistry/serviceentry/conversion_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceentry/conversion_test.go
CONFLICT (content): Merge conflict in pilot/pkg/serviceregistry/serviceentry/conversion_test.go
Auto-merging pilot/pkg/networking/core/v1alpha3/cluster_test.go
Auto-merging pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go
CONFLICT (content): Merge conflict in pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go
Auto-merging pilot/pkg/networking/core/v1alpha3/cluster_builder.go
CONFLICT (content): Merge conflict in pilot/pkg/networking/core/v1alpha3/cluster_builder.go
Auto-merging pilot/pkg/networking/core/v1alpha3/cluster.go
CONFLICT (content): Merge conflict in pilot/pkg/networking/core/v1alpha3/cluster.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 honour subject alt names from service entry
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.9

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.

istio-testing · 2021-06-02T20:12:50Z

@Kmoneal: new issue created for failed cherrypick: #33236

In response to this:

/cherry-pick release-1.9

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.

jacob-delgado · 2021-06-02T20:27:54Z

This feels like a big behavioral change for a patch release. Not sure cherrypick is safe
…
On Wed, Jun 2, 2021 at 1:10 PM Kmoneal @.***> wrote: /cherry-pick release-1.10 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32878 (comment)>, or unsubscribe <github.com/notifications/unsubscribe-auth/AAEYGXKBOHNX7WYDZP4EAN3TQ2F3NANCNFSM444BGW5Q> .

I agree with the fact that this is a big behavioral change, but it can also present issues for users upgrading in Istio 1.11; I'm assuming we will include this in the Upgrades notes to mitigate this. We can add an analyzer to assess ServiceEntrys and their use of subjectAltNames in pre Istio 1.11.

If we do not wish to fix this in 1.9/1.10 we should update the docs for 1.10 to reflect that ServiceEntrys currently do not respect subjectAltNames field and that this will be changed in Istio 1.11. We can update https://github.com/istio/api/blob/9a4239731e79ca441e6277e1ac8b38be48f31224/networking/v1beta1/service_entry.proto#L829 if appropriate.

We won't cherry pick this until we get agreement. Sorry for being too hasty.

honour subject alt names from service entry

0a30d20

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested a review from a team as a code owner May 14, 2021 08:46

istio-policy-bot added the area/networking label May 14, 2021

google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label May 14, 2021

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 14, 2021

add release notes

aabaadd

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested review from a team as code owners May 14, 2021 08:49

ramaraochavali added 5 commits May 14, 2021 14:21

fix area'

1a7e310

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix test

3022a8c

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Merge remote-tracking branch 'upstream/master' into fix/se_san

822b12d

fix test

a301cfe

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix test

4a59da8

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

brian-avery approved these changes May 14, 2021

View reviewed changes

hzxuzhonghu reviewed May 17, 2021

View reviewed changes

howardjohn reviewed May 17, 2021

View reviewed changes

move condition out

05d03c7

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

hzxuzhonghu reviewed May 18, 2021

View reviewed changes

comment

6b3c3ee

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn approved these changes Jun 1, 2021

View reviewed changes

istio-testing merged commit a5194cf into istio:master Jun 2, 2021

ramaraochavali deleted the fix/se_san branch June 2, 2021 05:33

nrjpoddar reviewed Jun 2, 2021

View reviewed changes

istio-testing mentioned this pull request Jun 2, 2021

[release-1.10] honour subject alt names from service entry #33235

Closed

istio-testing mentioned this pull request Jun 2, 2021

[release-1.9] honour subject alt names from service entry #33236

Closed

honour subject alt names from service entry #32878

honour subject alt names from service entry #32878

Uh oh!

Conversation

ramaraochavali commented May 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howardjohn commented May 17, 2021

Uh oh!

ramaraochavali commented May 17, 2021

Uh oh!

ramaraochavali commented May 18, 2021

Uh oh!

howardjohn commented May 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented May 18, 2021

Uh oh!

ramaraochavali commented May 18, 2021

Uh oh!

ramaraochavali commented May 19, 2021

Uh oh!

hzxuzhonghu commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramaraochavali commented May 19, 2021

Uh oh!

hzxuzhonghu commented May 19, 2021

Uh oh!

ramaraochavali commented May 19, 2021

Uh oh!

howardjohn commented May 19, 2021

Uh oh!

howardjohn commented May 19, 2021

Uh oh!

ramaraochavali commented May 20, 2021 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacob-delgado commented May 20, 2021

Uh oh!

ramaraochavali commented May 21, 2021

Uh oh!

ramaraochavali commented May 22, 2021

Uh oh!

ramaraochavali commented May 23, 2021

Uh oh!

ramaraochavali commented May 23, 2021

Uh oh!

ramaraochavali commented May 25, 2021

Uh oh!

ramaraochavali commented May 29, 2021

Uh oh!

ramaraochavali commented Jun 2, 2021

Uh oh!

nrjpoddar left a comment

Choose a reason for hiding this comment

ramaraochavali May 17, 2021 •

edited

Loading

hzxuzhonghu May 17, 2021 •

edited

Loading

hzxuzhonghu commented May 19, 2021 •

edited

Loading

ramaraochavali commented May 20, 2021 via email •

edited

Loading