Env Var allowing proxies to utilize OS CA Cert #33472

Kmoneal · 2021-06-16T16:34:20Z

Introduce VERIFY_CERTIFICATE_AT_CLIENT as a bool env var
Select an existing CA cert bundle from a set of known file locations
If bool set and DR does not set CaCertificates, replace the proxies' CA cert with an OS CA cert found in the file system

Address istio/istio #32539

[ ] 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.

istio-testing · 2021-06-16T16:34:27Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Kmoneal · 2021-06-16T16:47:06Z

Described in https://docs.google.com/document/d/100Jvwjr9KkURiF2HtiOtRlAON8TqgskUyLfT17_xoRM/edit#heading=h.xw1gqgyqs5b this is the initial Env Var portion to be expanded on.

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

pilot/pkg/features/pilot.go

pilot/pkg/model/authentication.go

howardjohn · 2021-06-28T23:51:17Z

pilot/pkg/model/authentication.go

+			}
+
+			for _, cert := range certFiles {
+				if _, err := os.Stat(cert); err == nil {


small perf nit: should we wrap this in a once like go does? Its fairly slow to check the filesystem on every call here

+1. And the value seems not neeed to passed around, and parameteize and special casing it. It's fair to assume the proxy image's filesystem stay the same during the pod running time and keep use that value.

* Remove code that is no longer needed with CA file path being generated on init * Move CACertFilePath init outside of shared package so it only needs to be made for sdsservice.go and secretcache.go

* SecretManager uses GenerateSecret and to avoid adding another perameter to the function, SecretManager implementations need to have it in the type. * `file-root:` was missing from secretcache_test.go to pull the correct certificate information

jacob-delgado

Kenan demoed this to me and we walked through the various test cases proposed in the RFC using badssl.com and its various subdomains (self-signed.badssl.com, untrusted-root.badssl.com, expired.badssl.com).

howardjohn · 2021-09-10T21:12:56Z

releasenotes/notes/33472.yaml

+  - 33472
+releaseNotes:
+  - |
+    **Added** `VERTIFY_CERT_AT_CLIENT` environmental variable to istiod. Setting `VERTIFY_CERT_AT_CLIENT` to `true` will verify server certificates using the OS CA certificates when not using a `DestinationRule` `caCertificates` field [Issue #33472](github.com/istio/istio/issues/33472).


can they also set caCertificates: system explicitly?

If so (which I think they should?) we should add a warning to the webhook when we detect an explicit path to use system instead. Definitely should be a followup though, not in this PR. Lets get this in first

They would be able to use system and it will treat it the same.

howardjohn · 2021-09-10T21:13:52Z

security/pkg/nodeagent/cache/secretcache.go

+		sdsFromFile = true
+		if sc.caRootPath != "" {
+			if sitem, err = sc.generateRootCertFromExistingFile(sc.caRootPath, resourceName, false); err == nil {
+				sc.addFileWatcher(sc.caRootPath, resourceName)


note to others: addFileWatcher dedupes so we will have at most 1 watch 👍

* Env Var allowing proxies to utilize OS CA Cert * Introduce VERIFY_CERTIFICATE_AT_CLIENT as a bool env var * Select an existing CA cert bundle from a set of known file locations * If bool set and DR does not set CaCertificates, replace the proxies' CA cert with an OS CA cert found in the file system * Move OS cert replace logic to proxy * Fix typo * Move function in pilot code and check for OS cert once * Move SdsCertificateConfigFromResourceName out of pilot authenticate code and into utils for agent. * Determine OS CA certificate in pilot-agent to pass down instead of searching for the file more often * Linter and change setting off by default * Move OS CA Path find to option generation * Add release notes * Lock pointers when accessing or using them * Remove mutex inside Options * Fix secret resource choice * Add unit tests * Fix tests and linter errors * Improve tests, releasenotes and logging * Add back code that was needed * Fix linter issues * Use string constant in place of file-root:system * Update to remove proxy from ClusterBuilder * Fix auto import location * Refactor code * Refactor SdsCertificateConfig to common package * Move security tests from util.go * Fix releasenotes * Add documentation to public methods * Improve VERIFY_CERT_AT_CLIENT testing * initialize CAFilePath in security instead of main * Refactor and remove dead code * Remove code that is no longer needed with CA file path being generated on init * Move CACertFilePath init outside of shared package so it only needs to be made for sdsservice.go and secretcache.go * Refactor to move cafile package * Refactor toEnvoySecret funciton * Revert changes that were needed for istiod to know the OS CA file path * Fix linter problems * Correct tests and introduce caRootPath to SecretManager * SecretManager uses GenerateSecret and to avoid adding another perameter to the function, SecretManager implementations need to have it in the type. * `file-root:` was missing from secretcache_test.go to pull the correct certificate information * Fix missing argument for mock initializaiton * Reverse mock changes and replace string with const * Fix SdsCertificateConfig to use file-root:system cert. * Fix tests by removing file-root: prefix * Fix empty OS CA cert causing an error * Remove testing print code * Rebase cluster_builder_test update struct * Fix rebase changes made to DestiantionRule

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

istio-policy-bot added the area/networking label Jun 16, 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 Jun 16, 2021

istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 16, 2021

Kmoneal marked this pull request as ready for review June 16, 2021 16:47

Kmoneal requested review from a team as code owners June 16, 2021 16:47

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

howardjohn requested changes Jun 16, 2021

View reviewed changes

pilot/pkg/networking/core/v1alpha3/cluster.go Outdated Show resolved Hide resolved

jacob-delgado reviewed Jun 28, 2021

View reviewed changes

pilot/pkg/features/pilot.go Outdated Show resolved Hide resolved

jacob-delgado reviewed Jun 28, 2021

View reviewed changes

pilot/pkg/model/authentication.go Outdated Show resolved Hide resolved

howardjohn reviewed Jun 28, 2021

View reviewed changes

Kmoneal requested review from a team as code owners July 14, 2021 18:29

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 14, 2021

Kmoneal force-pushed the os-ca-cert branch from b832f51 to d775db4 Compare July 14, 2021 19:48

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 14, 2021

Kmoneal requested review from a team as code owners July 15, 2021 23:02

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 19, 2021

Kmoneal force-pushed the os-ca-cert branch from f61d06c to 5fedbc5 Compare July 19, 2021 17:02

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged and removed needs-rebase Indicates a PR needs to be rebased before being merged labels Jul 19, 2021

Kmoneal force-pushed the os-ca-cert branch from 7bccee0 to cf2e3d4 Compare July 20, 2021 17:19

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 20, 2021

Kmoneal mentioned this pull request Jul 21, 2021

Introduce InsecureSkipVerify to DR istio/api#2040

Merged

Kmoneal added 18 commits September 9, 2021 16:58

Fix releasenotes

05e6850

Add documentation to public methods

2f76981

Improve VERIFY_CERT_AT_CLIENT testing

b3dd4f8

initialize CAFilePath in security instead of main

69755af

Refactor and remove dead code

62b31a7

* Remove code that is no longer needed with CA file path being generated on init * Move CACertFilePath init outside of shared package so it only needs to be made for sdsservice.go and secretcache.go

Refactor to move cafile package

73115d5

Refactor toEnvoySecret funciton

491c46f

Revert changes that were needed for istiod to know the OS CA file path

8105066

Fix linter problems

ea811c0

Fix missing argument for mock initializaiton

ebeaab3

Reverse mock changes and replace string with const

6018a08

Fix SdsCertificateConfig to use file-root:system cert.

9d4b904

Fix tests by removing file-root: prefix

c9f4d28

Fix empty OS CA cert causing an error

c10d2fc

Remove testing print code

6561a24

Rebase cluster_builder_test update struct

b4dd302

Fix rebase changes made to DestiantionRule

c461a76

Kmoneal force-pushed the os-ca-cert branch from fc17396 to c461a76 Compare September 10, 2021 17:26

jacob-delgado approved these changes Sep 10, 2021

View reviewed changes

Kmoneal requested a review from howardjohn September 10, 2021 19:57

howardjohn approved these changes Sep 10, 2021

View reviewed changes

istio-testing merged commit 562cdb8 into istio:master Sep 10, 2021

jacob-delgado mentioned this pull request Sep 10, 2021

Server Identity is not verified if caCertificates are not set when TLS mode is SIMPLE or MUTUAL #25652

Closed

Kmoneal mentioned this pull request Sep 17, 2021

Improve DestinationRule Security Best Practices istio/istio.io#10364

Merged

10 tasks

brian-avery mentioned this pull request Sep 17, 2021

[release-1.11] Env Var allowing proxies to utilize OS CA Cert (#33472) #35188

Closed

Kmoneal mentioned this pull request Sep 27, 2021

Add the release manager for istio 1.12 istio/community#653

Merged

5 tasks

bh-tt mentioned this pull request Mar 7, 2024

DestinationRule caCertificates system option is not documented #49783

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Env Var allowing proxies to utilize OS CA Cert #33472

Env Var allowing proxies to utilize OS CA Cert #33472

Kmoneal commented Jun 16, 2021 •

edited by istio-policy-bot

istio-testing commented Jun 16, 2021

Kmoneal commented Jun 16, 2021

howardjohn Jun 28, 2021

incfly Aug 19, 2021

jacob-delgado left a comment •

edited

howardjohn Sep 10, 2021

Kmoneal Sep 10, 2021

howardjohn Sep 10, 2021

Env Var allowing proxies to utilize OS CA Cert #33472

Env Var allowing proxies to utilize OS CA Cert #33472

Conversation

Kmoneal commented Jun 16, 2021 • edited by istio-policy-bot

istio-testing commented Jun 16, 2021

Kmoneal commented Jun 16, 2021

howardjohn Jun 28, 2021

Choose a reason for hiding this comment

incfly Aug 19, 2021

Choose a reason for hiding this comment

jacob-delgado left a comment • edited

Choose a reason for hiding this comment

howardjohn Sep 10, 2021

Choose a reason for hiding this comment

Kmoneal Sep 10, 2021

Choose a reason for hiding this comment

howardjohn Sep 10, 2021

Choose a reason for hiding this comment

Kmoneal commented Jun 16, 2021 •

edited by istio-policy-bot

jacob-delgado left a comment •

edited