draft: support passing Secret source over MCP #40509

doujiang24 · 2022-08-17T09:13:04Z

It's a draft for #40325 , and using k8s core.v1.Secret according to @howardjohn 's suggestion.

TODO:

need to authorize secret resources, simply skip for now, since the current authorize way depends on k8s, but the original intention is to bypass k8s; I haven't figured out a way for it yet.
tests and doc.

@howardjohn @hzxuzhonghu Could you please take a look? Just want to make sure it's in the right direction.
Any feedbacks are welcome, thanks!

for istio#40325

istio-policy-bot · 2022-08-17T09:13:11Z

😊 Welcome @doujiang24! This is either your first contribution to the Istio istio repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

istio-testing · 2022-08-17T09:13:22Z

Hi @doujiang24. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

doujiang24 · 2022-08-22T10:22:56Z

Hi team, gentle ping.
Do you think it's the right way to pass v1.Secret through MCP?

cc @howardjohn @hzxuzhonghu

pilot/pkg/xds/sds.go

hzxuzhonghu

Will it influence configcontroller with mcp

pilot/pkg/bootstrap/server.go

pilot/pkg/model/push_context.go

howardjohn · 2022-08-22T21:11:38Z

pilot/pkg/model/push_context.go

@@ -1416,6 +1433,22 @@ func (ps *PushContext) initServiceRegistry(env *Environment) error {
 	return nil
 }

+// pre computes secrets per namespace
+func (ps *PushContext) initSecrets(env *Environment) error {
+	secretConfigs, err := env.List(gvk.Secret, NamespaceAll)


Won't this get all secrets from all components? Including k8s?

I think, no. It get configs from model.ConfigStore. but, seems the secrets from k8s, is on another path, credentials.Controller, which is not a kind of model.ConfigStore.

…the kubeclient is not nil.

doujiang24 · 2022-08-24T02:40:56Z

Will it influence configcontroller with mcp

@hzxuzhonghu Thanks.
I think it depends. when EnableSecretOverMCP = true,

if means only use secrets from MCP, I think they are two independent ways, they won't influence each other,
if means using secrets from k8s and MCP together, then we have to decide the priority, k8s first? or MCP first.

In the current implementation, we don't use secrets from k8s and MCP together, only use one.

hzxuzhonghu · 2022-08-24T02:52:32Z

If we intend to use secret over MCP to provide gateway certs, it is a little bit complex. Even worse in multi cluster, we have to respect both secret namespace and cluster attribute.

From the original issue, I guess you want to use it to provide gateway cert. So it is just a first step for transport, not for usage

doujiang24 · 2022-08-24T03:14:51Z

Thanks. All addressed.

I have two questions, looking forward to your help, thanks.

for the secrets, provide a new option way to bypass k8s totally, is the right way? or do you guys think it is worth maintaining it?

background: we are migrating a large & old north-south ingress, from Nginx to Istio + Envoy. And the old control plane is not in the k8s system. So, we hope the new Istio way does not rely on k8s, at least for the first step.
Also, we are processing well in our internal fork, in the bypass k8s way.

is it worth introducing a new secret controller?

As said in this TODO comment, so that we may register customer secret authorize method in the feature.
8a21eaa#diff-91e0a18a58ea36d2c656d7b2706c52917a74103820c07fc843f48a110c847678R536

We just passing a nil secret controller now, which makes it difficult to add authorize for secrets.

doujiang24 · 2022-08-24T03:49:26Z

If we intend to use secret over MCP to provide gateway certs, it is a little bit complex. Even worse in multi cluster, we have to respect both secret namespace and cluster attribute.

From the original issue, I guess you want to use it to provide gateway cert. So it is just a first step for transport, not for usage

Yeah, we want to pass secrets through MCP, so that the data plane could get the gateway cert from istio, and the related name of the gateway cert is defined in credentialName in Gateway CRD.
https://istio.io/latest/docs/reference/config/networking/gateway/#ServerTLSSettings

For the usage:
the current implementation (in this PR) does only honor the namespace and secret name, for now.
Missing the authorize method, that the biggest left issue for this PR, I think.

istio-testing · 2022-10-01T22:03:45Z

@doujiang24: 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.

costinm · 2022-10-01T23:33:32Z

I am concerned with this - we need a VERY strong security review/discussion first, messing with secrets is dangerous.

Also: Istiod already has a mechanism to distribute secrets using SDS ( which is a first class XDS object ), with a relatively reasonable security model we discussed at length. I don't know if k8s Secret and envoy Secret are a good match - I don't
mind exposing k8s Secret, but I think it must have the same security model, the caller must have some explicit permission for the secret.

costinm · 2022-10-01T23:36:36Z

On a related note - we currently use the special generator for MCP, originally it was intended to keep things clean and it was an experiment, but I think it is very reasonable to allow 'MCP' generators to be used with the default (envoy) and envoy resources to be returned on the MCP generator.

Only 'special' generator is proxyless - mainly because it requires a different format for LDS/RDS, but it also reuses EDS/CDS.

doujiang24 · 2022-10-03T01:36:57Z

Thanks for your reply.

Sorry, I think I made a mistake in the title, "passing" should be "accepting".

For istiod, we still deliver secret resources over SDS, just like it does now.
eg, using credentialName in Gateway CRD,
https://istio.io/latest/docs/reference/config/networking/gateway/#ServerTLSSettings

One thing that changed is istiod also accepts k8s secret from MCP server,
just like it accept k8s secret from k8s server.

Backgroud: we want to get rid of k8s server, in our north-south gateway.

Yeah, I totally agree with you about security.
I think we may introduce a new secret controller to implement the authurize.
But I'm not sure about it, as said in:
#40509 (comment)

Need your help, thanks.

istio-policy-bot · 2022-11-16T05:03:33Z

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-10-01. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

draft: support passing Secret source over MCP

0be3882

for istio#40325

doujiang24 requested review from a team as code owners August 17, 2022 09:13

istio-policy-bot added area/networking area/security labels Aug 17, 2022

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Aug 17, 2022

zirain reviewed Aug 22, 2022

View reviewed changes

pilot/pkg/xds/sds.go Outdated Show resolved Hide resolved

hzxuzhonghu reviewed Aug 22, 2022

View reviewed changes

howardjohn reviewed Aug 22, 2022

View reviewed changes

doujiang24 added 3 commits August 23, 2022 20:49

reformat.

9088c7b

addressed comments.

24e4fcb

bugfix: secret generator should be created in another goroutine when …

4880a81

…the kubeclient is not nil.

minor comment.

8a21eaa

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 23, 2022

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 1, 2022

istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Oct 1, 2022

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Nov 1, 2022

istio-policy-bot closed this Nov 16, 2022

istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Nov 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

draft: support passing Secret source over MCP #40509

draft: support passing Secret source over MCP #40509

doujiang24 commented Aug 17, 2022 •

edited by istio-policy-bot

istio-policy-bot commented Aug 17, 2022

istio-testing commented Aug 17, 2022

doujiang24 commented Aug 22, 2022

hzxuzhonghu left a comment

howardjohn Aug 22, 2022

doujiang24 Aug 24, 2022

doujiang24 commented Aug 24, 2022

hzxuzhonghu commented Aug 24, 2022

doujiang24 commented Aug 24, 2022

doujiang24 commented Aug 24, 2022

istio-testing commented Oct 1, 2022

costinm commented Oct 1, 2022

costinm commented Oct 1, 2022

doujiang24 commented Oct 3, 2022 •

edited

istio-policy-bot commented Nov 16, 2022

draft: support passing Secret source over MCP #40509

draft: support passing Secret source over MCP #40509

Conversation

doujiang24 commented Aug 17, 2022 • edited by istio-policy-bot

istio-policy-bot commented Aug 17, 2022

istio-testing commented Aug 17, 2022

doujiang24 commented Aug 22, 2022

hzxuzhonghu left a comment

Choose a reason for hiding this comment

howardjohn Aug 22, 2022

Choose a reason for hiding this comment

doujiang24 Aug 24, 2022

Choose a reason for hiding this comment

doujiang24 commented Aug 24, 2022

hzxuzhonghu commented Aug 24, 2022

doujiang24 commented Aug 24, 2022

doujiang24 commented Aug 24, 2022

istio-testing commented Oct 1, 2022

costinm commented Oct 1, 2022

costinm commented Oct 1, 2022

doujiang24 commented Oct 3, 2022 • edited

istio-policy-bot commented Nov 16, 2022

doujiang24 commented Aug 17, 2022 •

edited by istio-policy-bot

doujiang24 commented Oct 3, 2022 •

edited