Make sure aud from SDS JWT is Citadel #16666

pitlv2109 · 2019-08-29T17:54:14Z

Please provide a description for what this PR is for.
#16637

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

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

myidpt

/lgtm
Please test with both valid and invalid audiences. Thanks :)

security/pkg/nodeagent/cache/secretcache.go

install/kubernetes/helm/istio/values.yaml

security/pkg/nodeagent/cache/secretcache.go

pitlv2109 · 2019-08-29T19:45:01Z

@howardjohn Could you take a quick look. Trying to get this in before Friday noon for the rc-1.

howardjohn · 2019-08-29T20:05:02Z

install/kubernetes/helm/istio/charts/gateways/templates/deployment.yaml

@@ -296,7 +296,7 @@ spec:
          - serviceAccountToken:
              path: istio-token
              expirationSeconds: 43200
-              audience: {{ $.Values.global.trustDomain }}
+              audience: {{ $.Values.global.sds.token.aud }}


this is a backwards incompatible change it seems?

hmm there shouldn't be any problem, but regardless of this, sds is an alpha feature and not many people are using it seriously

And it's clear that trustDomain is not the real audience of this token, but the CA service that provides the certificates.

install/kubernetes/helm/istio/charts/mixer/templates/deployment.yaml

howardjohn · 2019-08-29T20:06:21Z

security/pkg/k8s/tokenreview/k8sauthn.go

 			// If the audiences are not specified, the api server will use the audience of api server,
 			// which is also the issuer of the jwt.
+			// This feature is only available on Kubernetes v1.13 and above.
+			Audiences: []string{defaultAudience},


when is this ever overrided? Seems like default is always used?

For 1.3, it's hard-coded. We will revisit it post 1.3 (maybe a patch in 1.3.x) to support configurable trust domain. The audience should be Citadel. We won't support Vault with trustworthy JWT for now

Ok to have it hard-coded - but hard-code it in the template as well.

And I don't think it matters if Vault or something else is used - as long as they check the same constant for audience it'll probably work.

The intent of audience is to make sure a server doesn't accept a token intended for a different server.

I'm not entirely sure the spifee URL is a proper use for audience - it's intended to identify workloads, not audiences. You could have any SA or workload implementing the cert signing service.

Maybe a simple string "istio-ca" as hardcoded audience would be better ?

@myidpt wdyt? I'm okay with either option (istio-ca or the spiffee one)

I'm OK with "istio-ca" since it's hard-coded anyway.

I've updated the PR to use "istio-ca". PTAL

why not hard code in the template then?

It's not always "istio-ca". For example, for GoogleCA, this value is different and Istio cannot control it

howardjohn · 2019-08-29T20:06:54Z

@costinm recommend you look at SDS values changes

costinm

The default audience seems to be hardcoded - so I don't think we need to add more config UX.

The agreement is that new runtime configs will go to istio/api and reviewed as API - I suggest just using the same hardcoded version - since obviously setting it to any other value will fail the check in citadel, so we're adding a setting that can't be changed by user.

costinm · 2019-08-29T22:39:31Z

I mean - there is no need to go trough istio/api review since this is not a setting that we need - but a constant that doesn't need to be exposed to the user.

pitlv2109 · 2019-08-29T23:18:11Z

I mean - there is no need to go trough istio/api review since this is not a setting that we need - but a constant that doesn't need to be exposed to the user.

Just to be clear, the aud needs to be configurable in Helm because different CAs expect different value for the aud. In GoogleCA, it's the trust domain. In Citadel, I'm not sure if it makes sense to set it to "cluster.local" @myidpt

install/kubernetes/helm/istio/values.yaml

pitlv2109 · 2019-08-30T16:06:39Z

@pitlv2109 It's a good practice to document the defaults. Setting the default value istio-ca in values.yaml would achieve that, right?

Well the default is not to use sds, so it should be empty. Same with some other sds attributes.

If I run helm template I will get token.aud=""

Isn't this intended? You'll also get sds.udsPath="", because by default sds is not enabled, and its attributes should be empty. If they want to enable sds, they will pass the sds yaml file. This is what we're using and recommending the users now. Why change the structure? If there is a strong reason that we need to move all default values to values.yaml (which I don't see why, unless the new installer really needs it this way), then this work deserves another PR to change the structure of the whole thing.

rlenglet · 2019-08-30T16:07:02Z

Well the default is not to use sds, so it should be empty. Same with some other sds attributes.

Does it harm to leave it non-empty?

rlenglet · 2019-08-30T16:07:34Z

The important point for a default value is that it always works. istio-ca always works, right?

rlenglet · 2019-08-30T16:08:15Z

If they want to enable sds, they will pass the sds yaml file.

That's a good point.

howardjohn · 2019-08-30T16:15:27Z

What other feature requests the user to pass a full new values-sds.yaml file? You shouldn't need to set 20 values in order to use a feature.

There is no reason a user should ever care about token.aud unless the are using GoogleCA, so they should never have to configure it.

Then you can remove it from values-istio-sds-auth.yaml, and all the other 'profiles'

Co-Authored-By: John Howard <howardjohn@google.com>

howardjohn · 2019-08-30T16:19:53Z

Opened #16705 to track making SDS easier to enable

rlenglet · 2019-08-30T16:20:36Z

I filed #16706 so we can track the issue of SDS default values later.

rlenglet · 2019-08-30T16:21:04Z

Opened #16705 to track making SDS easier to enable

@howardjohn beat me to it. ;)

pitlv2109 · 2019-08-30T16:21:41Z

What other feature requests the user to pass a full new values-sds.yaml file? You shouldn't need to set 20 values in order to use a feature.

Well the users do not need to set it, they only need to pass in the sds yaml file. Whether this modular model is good, that's another discussion.

There is no reason a user should ever care about token.aud unless the are using GoogleCA, so they should never have to configure it.

So that we're on the same page, Istio users never have to configure this.

rlenglet

Thanks @pitlv2109!

howardjohn · 2019-08-30T16:23:29Z

Passing --values sds.yaml or whatever is configuring it. Now with your change they don't. As mentioned in the issue I opened, ideally we would be able to just do helm install --set sds.enabled=true and things just work out of the box.

Thanks for making the change

istio-testing · 2019-08-30T17:06:44Z

In response to a cherrypick label: new pull request created: #16711

costinm · 2019-08-30T20:36:24Z

security/pkg/k8s/tokenreview/k8sauthn.go

@@ -83,10 +89,10 @@ func (authn *K8sSvcAcctAuthn) reviewServiceAccountAtK8sAPIServer(targetToken str
 		Kind:       "TokenReview",
 		Spec: specForSaValidationRequest{
 			Token: targetToken,


To confirm: for 1.12 we are still subject to 'confused deputy' ? My understanding was that we will check the audience in citadel ( by decoding the token and comparing ) so it works on 1.12 as well.

If in 1.12 audience check doesn't work and we don't add the fix - it should probably be in release notes, so users avoid using SDS ( or don't use any JWT tokens with any other servers that may create a 'confused deputy' problem)

If adding release notes, please do it here: https://github.com/istio/istio/wiki/Release-Note-1.3-Draft

I've updated the release notes.

Did we agree to completely break SDS for k8s 1.12 in TOC? My understanding is we just agreed to get rid of legacy jwt?

Yes, we agreed to get rid of legacy jwt.

GKE now has 1.13.7, so I think it's fine to ask customers to use 1.13? Also, Istio claims to support 1.13-1.15 https://preliminary.istio.io/faq/general/#what-deployment-environment for 1.3.

Nope, it's a breaking change. K8s below 1.13 fails to check if aud is anything other than api server.
Technically, we could still support 1.12 if in Citadel we parse the jwt and check if the audience is istio-ca, and send the TokenReview without the audience. I think you suggested this but I thought just bump the support to 1.13 and let k8s api does all the work.

I'm happy to let Citadel does the dirty work so that we can support k8s 1.12 or encourage users to upgrade to a newer k8s verion for other benefits, including security, that k8s > 1.12 offers.

Sorry, I didn't know this is a breaking change to 1.12. Given that SDS is an Alpha feature, I think it's acceptable to ask users to upgrade to 1.13 to use SDS (Also technically this doesn't break our policy of supporting the latest 3 K8s versions). We will update the release notes, blog post and the SDS task.

Having Citadel to check the audience isn't nice IMO. If we decide to check the audience in Citadel, we must not check it on apiserver.

Sounds good, so I'll keep things as they are. I've updated all the relevant docs.

howardjohn · 2019-08-30T21:14:06Z

My understanding is that it will not work, which is why the Kubernetes 1.12 tests fail now https://prow.istio.io/view/gcs/istio-prow/logs/integ-k8s-112-postsubmit-master/308 https://prow.istio.io/view/gcs/istio-prow/logs/integ-k8s-112-postsubmit-master/307 https://prow.istio.io/view/gcs/istio-prow/logs/integ-k8s-112-postsubmit-master/306

…

On Fri, Aug 30, 2019 at 2:09 PM Oliver Liu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In security/pkg/k8s/tokenreview/k8sauthn.go <#16666 (comment)>: > @@ -83,10 +89,10 @@ func (authn *K8sSvcAcctAuthn) reviewServiceAccountAtK8sAPIServer(targetToken str Kind: "TokenReview", Spec: specForSaValidationRequest{ Token: targetToken, I think we are not breaking SDS in K8s 1.12. SDS will still work, it's just K8s doesn't check the JWT "aud" field in 1.12, right? @pitlv2109 <https://github.com/pitlv2109> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16666?email_source=notifications&email_token=AAEYGXOWDCF5SQTSGBKMV43QHGEAVA5CNFSM4ISFEOO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDJL77I#discussion_r319675583>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEYGXL6S2CMTZP2AMI4IVDQHGEAVANCNFSM4ISFEOOQ> .

howardjohn · 2019-08-30T21:48:05Z

sgtm It would be nice if we could skip the SDS tests on the integ-k8s-112-postsubmit since otherwise we will basically need to disable them as they fail 100% now

…

On Fri, Aug 30, 2019 at 2:37 PM Phillip Quy Le ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In security/pkg/k8s/tokenreview/k8sauthn.go <#16666 (comment)>: > @@ -83,10 +89,10 @@ func (authn *K8sSvcAcctAuthn) reviewServiceAccountAtK8sAPIServer(targetToken str Kind: "TokenReview", Spec: specForSaValidationRequest{ Token: targetToken, Sounds good, so I'll keep things as they are. I've updated all the relevant docs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16666?email_source=notifications&email_token=AAEYGXOYH5U4CUAI2RQRWTLQHGHIZA5CNFSM4ISFEOO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDJN7PY#discussion_r319681625>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEYGXLTVG6KJ5OGMUUIWATQHGHIZANCNFSM4ISFEOOQ> .

Support for this version has both been dropped and broken by istio/istio#16666. We can re-enable this if we get back support for 1.12 (note - this is not officially supported version, so low priority) or disable just the test that is broken.

* istio#16223 * istio#16272 * istio#16187 * istio#16466 * istio#16634 * istio#16594 * istio#16666 * istio#16483 * istio#16820 * istio#16842 * istio#16852 * istio#16835 * istio#16863 * istio#16892 * istio#16991 * istio#16957 * istio#17013 * istio#17134 * istio#17155 * istio#17235 * istio#17342 * istio#17477 * istio#17615 * istio#17334 * istio#17708 * istio#17737 * Fix injection template * Fix quoting * Fix test values * Add accidentally deleted affinity

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 29, 2019

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Aug 29, 2019

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 29, 2019

pitlv2109 force-pushed the enforce-aud-field branch from 84e7564 to fdf64fe Compare August 29, 2019 18:01

myidpt added the cherrypick/release-1.3 label Aug 29, 2019

myidpt added this to the 1.3 milestone Aug 29, 2019

myidpt approved these changes Aug 29, 2019

View reviewed changes

myidpt requested a review from howardjohn August 29, 2019 18:19

rlenglet suggested changes Aug 29, 2019

View reviewed changes

security/pkg/nodeagent/cache/secretcache.go Outdated Show resolved Hide resolved

howardjohn reviewed Aug 29, 2019

View reviewed changes

install/kubernetes/helm/istio/values.yaml Outdated Show resolved Hide resolved

security/pkg/nodeagent/cache/secretcache.go Outdated Show resolved Hide resolved

rlenglet approved these changes Aug 29, 2019

View reviewed changes

pitlv2109 marked this pull request as ready for review August 29, 2019 19:26

pitlv2109 requested review from costinm, diemtvu, incfly, JimmyCYJ, linsun, mandarjog, morvencao, ostromart, quanjielin and sdake as code owners August 29, 2019 19:26

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 29, 2019

howardjohn reviewed Aug 29, 2019

View reviewed changes

howardjohn assigned costinm Aug 29, 2019

costinm reviewed Aug 29, 2019

View reviewed changes

howardjohn reviewed Aug 30, 2019

View reviewed changes

install/kubernetes/helm/istio/values.yaml Outdated Show resolved Hide resolved

Set token.aud to istio-ca

2f03c65

Co-Authored-By: John Howard <howardjohn@google.com>

rlenglet approved these changes Aug 30, 2019

View reviewed changes

howardjohn approved these changes Aug 30, 2019

View reviewed changes

istio-testing merged commit e849e69 into istio:master Aug 30, 2019

istio-testing mentioned this pull request Aug 30, 2019

[release-1.3] Make sure aud from SDS JWT is Citadel #16711

Merged

howardjohn mentioned this pull request Aug 30, 2019

Check the JWT audience in Citadel #16637

Closed

pitlv2109 mentioned this pull request Aug 30, 2019

Sds trustworthy jwt doc istio/istio.io#4701

Merged

costinm reviewed Aug 30, 2019

View reviewed changes

This was referenced Sep 2, 2019

SDS no longer works on Kubernetes 1.12, breaking tests #16754

Closed

Remove k8s 1.12 tests istio/test-infra#1753

Merged

howardjohn mentioned this pull request Oct 15, 2019

Backport changes istio/installer#433

Merged

Make sure aud from SDS JWT is Citadel #16666

Make sure aud from SDS JWT is Citadel #16666

Conversation

pitlv2109 commented Aug 29, 2019 • edited Loading

myidpt left a comment

Choose a reason for hiding this comment

pitlv2109 commented Aug 29, 2019

Choose a reason for hiding this comment

pitlv2109 Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myidpt Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Aug 29, 2019

costinm left a comment

Choose a reason for hiding this comment

costinm commented Aug 29, 2019

pitlv2109 commented Aug 29, 2019

pitlv2109 commented Aug 30, 2019

rlenglet commented Aug 30, 2019

rlenglet commented Aug 30, 2019

rlenglet commented Aug 30, 2019

howardjohn commented Aug 30, 2019

howardjohn commented Aug 30, 2019

rlenglet commented Aug 30, 2019

rlenglet commented Aug 30, 2019

pitlv2109 commented Aug 30, 2019

rlenglet left a comment

Choose a reason for hiding this comment

howardjohn commented Aug 30, 2019

istio-testing commented Aug 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Aug 30, 2019 via email

howardjohn commented Aug 30, 2019 via email

pitlv2109 commented Aug 29, 2019 •

edited

Loading

pitlv2109 Aug 29, 2019 •

edited

Loading

myidpt Aug 29, 2019 •

edited

Loading