Handle PrivateKeyProvider configuration by rveerama1 · Pull Request #37681 · istio/istio

rveerama1 · 2022-03-03T12:05:25Z

Please provide a description of this PR:

When the PrivateKeyProvider configuration information is provided through Istio operator yaml file, parse and pass it to gateway or sidecars. Envoy will act based on the information provided by the configuration.

To set the mesh wide defaults, configure the defaultConfig section of meshConfig.
For example:

meshConfig:
  defaultConfig:
    privateKeyProvider:
      cryptomb:
        pollDelay: 0.01s

This can also be configured on a per-workload basis by configuring the proxy.istio.io/config
annotation on the pod.
For example:

annotations:
  proxy.istio.io/config: |
    privateKeyProvider:
      cryptomb:
        pollDelay: 0.01s

This requires istio/api#2261

istio-policy-bot · 2022-03-03T12:05:28Z

😊 Welcome @rveerama1! 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-03-03T12:05:51Z

Hi @rveerama1. 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.

Details

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.

Arhell

/ok-to-test

rveerama1 · 2022-03-04T10:49:27Z

As it depends upon api changes, build fails. I will move to WIP.

rveerama1 · 2022-03-16T08:25:36Z

@howardjohn @jacob-delgado @ericvn PTAL about implementation of istio/api#2261 private key provider addition.

releasenotes/notes/37681.yaml

rveerama1 · 2022-03-17T20:30:26Z

/retest

security/pkg/nodeagent/sds/sdsservice.go

yangminzhu · 2022-03-17T20:47:19Z

pilot/pkg/xds/sds.go

this has quite some duplicate code as the other toEnvoyKeyCertSecretWithPrivateKeyProvider(), could we refactor and try to share the common logic

It has some, but not completely. Anyway I am working on QAT private key provider extension support. I will fine tune in that PR.

yangminzhu · 2022-03-17T20:53:55Z

security/pkg/nodeagent/sds/server.go

Just some general questions

Regarding the PrivateKeyProvider extension, is it actually providing any private key? The name is a bit confused to me because based on the PR and my understanding, it does not provide any private key but is just a new type (cryptomb) to wrap the TLS config that tells Envoy to process it with some hardware assisted acceleration?

How are you going to e2e test the feature if it requires some special hardware support?

Regarding the PrivateKeyProvider extension, is it actually providing any private key? The name is a bit confused to me because based on the PR and my understanding, it does not provide any private key but is just a new type (cryptomb) to wrap the TLS config that tells Envoy to process it with some hardware assisted acceleration?

Name is bit confusing, but I tried to sync with Envoy naming pattern https://github.com/envoyproxy/envoy/tree/main/api/contrib/envoy/extensions/private_key_providers

I think there are two things which can be e2e tested. One is the functionality of setting a private key provider configuration to Envoy, and the other is a specific private key provider such as CryptoMB.

The first case is probably not too bad -- for example Envoy uses a software-only private key provider for testing the interfaces (https://github.com/envoyproxy/envoy/blob/main/test/extensions/transport_sockets/tls/test_private_key_method_provider.cc) and we could do something similar. The test private key provider isn't exposed by Envoy though, but that code could probably be used or reimplemented by istio/proxy test/envoye2e directory.

The second case (CryptoMB) is tricky because that will indeed require a very modern Xeon processor. An e2e test could be written but it should be behind a switch, and we need to understand if it's possible to make the CI tests run in an environment which supports AVX-512. @yangminzhu do you have any suggestions how this could be accomplished?

Thanks for the detail explanation, the naming part is a bit confusing but I think it is fine if you could document it clearly in the code and documentation so that other users does not confuse it with things like Plug in certificate that is really providing its own private/public key to Istio.

Regarding the e2e tests, it would be nice if you can somehow mock the CryptoMB functionality in Envoy then run it like a real e2e test but I have no idea how to do it easily so you probably will end up with tests verifying the Envoy TLS configs generated correctly (but not executing them). Also I have no idea how to require a very modern Xeon processor in the CI environment (it gives you some flexibility in machine type but probably not to the level for custom CPU), @howardjohn might know about about the CI configuration.

pilot/pkg/bootstrap/server.go

security/pkg/nodeagent/sds/sdsservice.go

howardjohn

LGTM once we fix the gateway part, should be small fix

pilot/pkg/xds/sds.go

howardjohn · 2022-03-30T22:05:45Z

/retest

jacob-delgado · 2022-03-31T18:35:47Z

cc @rveerama1 Looks like you need to re-base, but have the appropriate approvals.

When the PrivateKeyProvider configuration information is provided through Istio operator yaml file, parse and pass it to gateway or sidecars. Envoy will act based on the information provided by the configuration. To set the mesh wide defaults, configure the `defaultConfig` section of `meshConfig`. For example: meshConfig: defaultConfig: privateKeyProvider: cryptomb: pollDelay: 0.01s This can also be configured on a per-workload basis by configuring the `proxy.istio.io/config` annotation on the pod. For example: annotations: proxy.istio.io/config: | privateKeyProvider: cryptomb: pollDelay: 0.01s

rveerama1 · 2022-03-31T20:29:24Z

thank you, @jacob-delgado @howardjohn fixed the conflicts. can you remove the do-not-merge/hold label

howardjohn

Thanks for the great work here!

* Handle PrivateKeyProvider configuration When the PrivateKeyProvider configuration information is provided through Istio operator yaml file, parse and pass it to gateway or sidecars. Envoy will act based on the information provided by the configuration. To set the mesh wide defaults, configure the `defaultConfig` section of `meshConfig`. For example: meshConfig: defaultConfig: privateKeyProvider: cryptomb: pollDelay: 0.01s This can also be configured on a per-workload basis by configuring the `proxy.istio.io/config` annotation on the pod. For example: annotations: proxy.istio.io/config: | privateKeyProvider: cryptomb: pollDelay: 0.01s * add release notes * add validation and tests * modify according to api change * address comments * Fix lint issues * address comments * address jacob-delgado and howardjohn comments * add tests * use default config * fix rebase conflicts * fix config for gateway cert * fix rebase conflicts

rveerama1 · 2022-04-12T14:04:39Z

How we do this with other custom integrations is OSS has unit level tests that check the generated XDS is correct. The owner of the platform runs tests on their own system with their own platform to verify it works in the real world.

For example we do this with things like Stackdriver, etc

@howardjohn I am looking into write e2e test setup for this. Need some help.

Are there any existing e2e tests for XDS generation (where it involves envoy also)? If yes I can extend it with this feature.
You mentioned about Stackdriver as an example for custom tests. I searched about telemetry/stackdriver tests about it. Couldn't get complete picture. Can you give some pointers where I can look about e2e tests which require custom hardware?
or anybody to contact?

rveerama1 requested review from a team as code owners March 3, 2022 12:05

istio-policy-bot added area/networking area/security labels Mar 3, 2022

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

This was referenced Mar 4, 2022

Enable configuring Envoy private key provider through ProxyConfig istio/api#2261

Merged

Proposal: enable configuring Envoy private key providers over xDS protocol from Istiod #35809

Closed

Arhell reviewed Mar 4, 2022

View reviewed changes

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Mar 4, 2022

rveerama1 mentioned this pull request Mar 4, 2022

Enable CryptoMB PrivateKeyProvider extension istio/proxy#3752

Merged

rveerama1 requested review from a team as code owners March 4, 2022 10:47

rveerama1 changed the title ~~Handle PrivateKeyProvider configuration~~ [WIP] Handle PrivateKeyProvider configuration Mar 4, 2022

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

rveerama1 force-pushed the private-key-provider branch 2 times, most recently from 3ad09e4 to 5adfa5d Compare March 16, 2022 06:45

rveerama1 changed the title ~~[WIP] Handle PrivateKeyProvider configuration~~ Handle PrivateKeyProvider configuration Mar 16, 2022

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

jacob-delgado reviewed Mar 17, 2022

View reviewed changes

releasenotes/notes/37681.yaml Outdated Show resolved Hide resolved

yangminzhu reviewed Mar 17, 2022

View reviewed changes

jacob-delgado reviewed Mar 18, 2022

View reviewed changes

pilot/pkg/bootstrap/server.go Outdated Show resolved Hide resolved

jacob-delgado reviewed Mar 18, 2022

View reviewed changes

security/pkg/nodeagent/sds/sdsservice.go Outdated Show resolved Hide resolved

rveerama1 force-pushed the private-key-provider branch from 65390da to 858ea4f Compare March 18, 2022 10:12

howardjohn approved these changes Mar 30, 2022

View reviewed changes

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

howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Mar 30, 2022

howardjohn reviewed Mar 30, 2022

View reviewed changes

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

rveerama1 force-pushed the private-key-provider branch from 776ffcb to 0d7e4ac Compare March 30, 2022 21:25

howardjohn approved these changes Mar 30, 2022

View reviewed changes

jacob-delgado approved these changes Mar 31, 2022

View reviewed changes

rveerama1 added 13 commits March 31, 2022 22:09

add release notes

25f9267

add validation and tests

2407f4f

modify according to api change

0995ca1

address comments

ef14efe

Fix lint issues

3833f40

address comments

e50e68e

address jacob-delgado and howardjohn comments

d314f64

add tests

a11fb6f

use default config

836e50a

fix rebase conflicts

2452e0c

fix config for gateway cert

58f0986

fix rebase conflicts

8a1e306

rveerama1 force-pushed the private-key-provider branch from 0d7e4ac to 8a1e306 Compare March 31, 2022 19:48

howardjohn reviewed Mar 31, 2022

View reviewed changes

howardjohn removed the do-not-merge/hold Block automatic merging of a PR. label Mar 31, 2022

istio-testing merged commit 1647479 into istio:master Mar 31, 2022

rveerama1 mentioned this pull request Jun 10, 2022

Add rveerama1 to Istio member list istio/community#746

Merged

5 tasks

Conversation

rveerama1 commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-policy-bot commented Mar 3, 2022

Uh oh!

istio-testing commented Mar 3, 2022

Uh oh!

Arhell left a comment

Choose a reason for hiding this comment

Uh oh!

rveerama1 commented Mar 4, 2022

Uh oh!

rveerama1 commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rveerama1 commented Mar 17, 2022

Uh oh!

Uh oh!

yangminzhu Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

rveerama1 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

yangminzhu Mar 17, 2022 • 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!

rveerama1 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

ipuustin Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

yangminzhu Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

howardjohn commented Mar 30, 2022

Uh oh!

jacob-delgado commented Mar 31, 2022

Uh oh!

rveerama1 commented Mar 31, 2022

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

rveerama1 commented Apr 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

rveerama1 commented Mar 3, 2022 •

edited

Loading

rveerama1 commented Mar 16, 2022 •

edited

Loading

yangminzhu Mar 17, 2022 •

edited

Loading