Introduce InsecureSkipVerify to DR #2040

Kmoneal · 2021-06-29T20:14:59Z

hide VerifyCertificateAtClient in ProxyConfig to eventually be
removed.
Add InsecureSkipVerify bool to allow users to prevent any certificate
validation on desired external host.

istio-testing · 2021-06-29T20:15:05Z

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

jacob-delgado · 2021-06-30T19:43:25Z

cc @ericvn can you go over the documentation for this PR? Thanks!

brian-avery · 2021-06-30T21:14:41Z

This is part of https://docs.google.com/document/d/100Jvwjr9KkURiF2HtiOtRlAON8TqgskUyLfT17_xoRM/edit#

craigbox · 2021-07-01T18:07:42Z

releasenotes/notes/25652.yaml

+
+releaseNotes:
+  - |
+    **Added** InsecureSkipVerify to DestinationRule. Users can choose to opt-out of validating external site certificates on hosts basis if a user has enabled certificate validation.


on a per-host basis?

craigbox · 2021-07-01T18:08:29Z

releasenotes/notes/25652.yaml

+releaseNotes:
+  - |
+    **Added** InsecureSkipVerify to DestinationRule. Users can choose to opt-out of validating external site certificates on hosts basis if a user has enabled certificate validation.
+    **Modified** VerifyCertificateAtClient is now deprecated in meshconfig. This will be removed in a future release and was never implemented.


meshConfig?

"This feature was not implemented, and will be removed in a future release" ?

craigbox · 2021-07-01T18:09:23Z

while Eric is out, Brian asked me to look at this; the only thing I think that is relevant to docs is

// $hide_from_docs

so LGTM with some comments on the release note

craigbox · 2021-07-01T18:11:05Z

networking/v1alpha3/destination_rule.proto

+
+  // Boolean specifying if proxy should not check the CA signature for the host
+  // certificate. `InsecureSkipVerify` is `false` by default. If set to true,
+  // the proxy will skip verification of the host's certificate regardless of


Suggested change

// the proxy will skip verification of the host's certificate regardless of

// the proxy will skip verification of the host's certificate, regardless of

craigbox · 2021-07-01T18:11:29Z

networking/v1alpha3/destination_rule.proto

@@ -964,6 +964,12 @@ message ClientTLSSettings {

  // SNI string to present to the server during TLS handshake.
  string sni = 6;
+
+  // Boolean specifying if proxy should not check the CA signature for the host


if the proxy?

Kmoneal · 2021-07-02T05:59:51Z

/retest

hzxuzhonghu · 2021-07-02T08:20:41Z

lg, need to run make gen

nrjpoddar

You need to sync v1alpha3 to v1beta1 APIs. @howardjohn recently wrote a tool for it so you can just run it and ensure the proto files are in sync.

nrjpoddar · 2021-07-02T09:08:28Z

networking/v1alpha3/destination_rule.proto

+  // Boolean specifying if the proxy should not check the CA signature for the host
+  // certificate. `InsecureSkipVerify` is `false` by default. If set to true,
+  // the proxy will skip verification of the host's certificate, regardless of
+  // any CA certificate specified elsewhere.


Suggested change

// Boolean specifying if the proxy should not check the CA signature for the host

// certificate. `InsecureSkipVerify` is `false` by default. If set to true,

// the proxy will skip verification of the host's certificate, regardless of

// any CA certificate specified elsewhere.

// InsecureSkipverify specifies whether the proxy should skip verifying the CA signature for the server

// certificate. `InsecureSkipVerify` is `false` by default. If set to true,

// the proxy will skip verification of the server's certificate, regardless of

// the CA certificate path specified elsewhere.

Does it also not verify the SAN field?

This would also not verify the SAN past the cert and host name match

Ok basically certificate signature and Host in SAN is validated. If that's correct we should not just mention signature here.

* hide VerifyCertificateAtClient in ProxyConfig to eventually be removed. * Add InsecureSkipVerify bool to allow users to prevent any certificate validation on desired external host.

* Update release-notes to specify changes and purpose for adding InsecureSkipVerify and deprecating VerifyCertificateAtClient * VerifyCertificateAtClient gets deprecated instead of only hidden

nrjpoddar · 2021-07-14T15:58:01Z

networking/v1alpha3/destination_rule.proto

+  // Boolean specifying if the proxy should not check the CA signature for the host
+  // certificate. `InsecureSkipVerify` is `false` by default. If set to true,
+  // the proxy will skip verification of the host's certificate, regardless of
+  // any CA certificate specified elsewhere.


Ok basically certificate signature and Host in SAN is validated. If that's correct we should not just mention signature here.

releasenotes/notes/25652.yaml

networking/v1alpha3/destination_rule.proto

nrjpoddar · 2021-07-14T17:09:42Z

Few more minor suggestions, rest look good!

Kmoneal · 2021-07-15T16:36:47Z

@brian-avery if you have time, I would appreciate any input!

networking/v1beta1/destination_rule.proto

linsun · 2021-07-19T16:51:38Z

mesh/v1alpha1/config.proto

@@ -436,7 +437,7 @@ message MeshConfig {
  // For wildcard host name in DestinationRule, client-side proxy will do a suffix match. For example,
  // if host is `*.x.y.com`, client-side proxy will verify the presented server certificate SAN matches
  // ``.x.y.com` suffix.
-  google.protobuf.BoolValue verify_certificate_at_client = 54;
+  google.protobuf.BoolValue verify_certificate_at_client = 54 [deprecated=true];


Can you clarify default here given this is not configurable?

Is the scenario that this is set to true by default globally but certain service wants to skip the validation by setting insecure_skip_verify?

By default, we don't verify server certificates unless you specify caCertificates in DR which is the bug (CVE) we are trying to address. @Kmoneal is adding an ENV var in istio/istio so that going forward CA verification can be turned on globally (it will still be default off for backwards compatibility). Once you have it enabled globally, the new config option in DR insecureSkipVerify disables it on a per host (service) basis.

Neeraj summed it up well. This was an attempt to add in enabling mesh-wide external server certificate validation. By planning to default verification to on, we wanted to make sure that we can allow users to disable it mesh-wide.

networking/v1alpha3/destination_rule.proto

linsun · 2021-07-19T16:55:50Z

networking/v1alpha3/destination_rule.proto

+  // is `false` by default. If set to true, the proxy will skip verification of
+  // the server certificate, regardless of any certificate validation
+  // configureation provided elsewhere.
+  google.protobuf.BoolValue insecure_skip_verify = 8;


what about skip_verify_cert or skip_verify_certificate? seems easier to understand and consistent with VerifyCertificateAtClient

Negation makes more sense as it is consistent with tools like curl that provide "-k" flag. We expect most users to have verification globally turned on (and hopefully in few releases default on) so in that light this naming is more clear. WDYT?

We wanted to make sure it was explicit that it was an insecure flag to enable and I prefer to keep that. If insecure_skip_verify_cert is agreeable I would be happy to change it.

I like InsecureSkipVerify because it has a wellknown meaning already

louiscryan · 2021-07-19T17:01:41Z

networking/v1alpha3/destination_rule.proto

+  // is `false` by default. If set to true, the proxy will skip verification of
+  // the server certificate, regardless of any certificate validation
+  // configureation provided elsewhere.
+  google.protobuf.BoolValue insecure_skip_verify = 8;


Is the certificate expiration checked?

@nrjpoddar If we're confident that we won't need more control then I'm OK with not being an enum.

I guess so? I'm assuming Envoy uses the openssl call to verify most expected attributes like signature, SAN, expiration, not valid before etc. We can refine the comment to not explicitly list out just signature and SAN.

I think starting out with boolean is good here. I haven't seen many requests for signature verification but no SAN or skip expiration etc.

This is what curl documents for -k:

The server connection is verified by making sure the server's certificate contains the right name and verifies successfully using the cert store.

The certificate would be checked that it is a usable cert (expiration, not malformed, etc) but it will not check against a CA.

curl -k doesn't verify expiration (you can check with https://expired.badssl.com/) fwiw

nrjpoddar

If folks questions are answered can we approve this? @howardjohn @linsun @louiscryan? Thanks!

howardjohn · 2021-07-21T16:56:42Z

networking/v1beta1/destination_rule.proto

+
+  // InsecureSkipverify specifies whether the proxy should skip verifying the
+  // CA signature and SAN for the server certificate corresponding to the host.
+  // `InsecureSkipVerify` is `false` by default. If set to true, the proxy will


is "false by default" accurate?

Please, correct me if I am missing something, but InsecureSkipVerify would disable any other added CA Root Certs or SAN specified which means it should be false. Defaulting to false would have no changes to current environments once the istio modifications are made. This is a new feature to disable security checks per host and the Env Var portion is adding the additional security setting which will be disabled by default as well.

This seems misleading. If someone told me InsecureSkipVerify is false by default, and I configure a DR for https://google.com (with no caCertificate config), I think the vast majority of users would expect that we are verifying the TLS.
Even without this comment, today most people (including most Istio developers), thought it was verifying. This furthers the confusion IMO.

I agree, it is misleading and may provide a false sense of security for the standard settings. I took a second look and wanted to know if any of these solutions would be able to fix any concerns?

Hide InsecureSkipVerify from docs until OS CA Certificates verification is enabled by default.

Switch to an enum like the one below. This could also give a false sense of security unless VERIFY is added only when it is available. It will be a change from RFC but considering it has been talked about a couple of time I think it would be worth it

enum InsecureSkipVerifyMode { // Keep the legacy behavior, no change UNSPECIFIED = 0; // Do not verify certificates against a CA but verify that the certificate is valid // with basic checks (i.e. expiration check, issued to the expected host, etc.) NO_VERIFY = 1; // Allow any verification enabled to be unchanged VERIFY = 2; }

Change wording to specify that this just disables added DestinationRule and ServiceEntry CA/SAN settings verification.

If there is anything else you would recommend or any modifications I would be happy to hear them.

I would suggest adding clarification around how this flag works in relationship with the environment variable i.e. this flag should only be used when the env var is enabled and you have verification turned on globally but want it disabled for a specific host. I would even add which Istio releases have the env var default off and later when we toggle it add from the release it is by default on. WDYT?

+1 to documenting correlation to the flag.

howardjohn · 2021-07-21T16:57:59Z

Put a hold for small comment

nrjpoddar · 2021-07-26T13:49:01Z

Put a hold for small comment

John, @Kmoneal responses to your comment so PTAL and remove hold so this can collapse.

howardjohn · 2021-07-27T21:29:00Z

networking/v1alpha3/destination_rule.proto

+  //
+  // `InsecureSkipVerify` is `false` by default.
+  // `VerifyCertAtClient` is `false` by default in Istio version 1.9 until
+  // Istio version 1.13 where, going forward, it will be enabled by default.


Nit: would avoid exact date, historically we don't hit our targets a lot 🙂

nrjpoddar

@Kmoneal I missed updates to this PR so added few suggestions. PTAL.

nrjpoddar · 2021-07-30T10:25:25Z

networking/v1alpha3/destination_rule.proto

+  // This flag should only be set if global CA signature verifcation is
+  // enabled, `VerifyCertAtClient` environmental variable is set to `true`,


// This flag should only be used if server certificate validation has been globally enabled
// via setting the environment variable VERIFY_CERTIFICATE_AT_CLIENT to true in istiod
// but no verification is desired for a specific host.

nrjpoddar · 2021-07-30T10:27:32Z

networking/v1alpha3/destination_rule.proto

+  // SAN will be skipped.
+  //
+  // `InsecureSkipVerify` is `false` by default.
+  // `VerifyCertAtClient` is `false` by default in Istio version 1.9 but will


// Environment variable VERIFY_CERTIFICATE_AT_CLIENT is false by default in Istio versions 1.9 - 1.11
// for backwards compatibility.
// This variable will be set to true by default in a later version where, going forward, it will be enabled by default.
// It is strongly recommended to turn on server certificate validation globally by setting this environment
// variable to true and selectively disabling verification on a per-host basis by using the
// InsecureSkipVerify flag in DestinationRule.

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 29, 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 29, 2021

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

Kmoneal marked this pull request as ready for review June 30, 2021 18:49

Kmoneal requested review from brian-avery, howardjohn, linsun, louiscryan, nrjpoddar and smawson as code owners June 30, 2021 18:49

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 30, 2021

craigbox reviewed Jul 1, 2021

View reviewed changes

nrjpoddar reviewed Jul 2, 2021

View reviewed changes

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

Kmoneal added 5 commits July 8, 2021 11:00

Introduce InsecureSkipVerify to DR

064c7a6

* hide VerifyCertificateAtClient in ProxyConfig to eventually be removed. * Add InsecureSkipVerify bool to allow users to prevent any certificate validation on desired external host.

Updated release-notes and depricate VerifyCertificateAtClient

8bf2c89

* Update release-notes to specify changes and purpose for adding InsecureSkipVerify and deprecating VerifyCertificateAtClient * VerifyCertificateAtClient gets deprecated instead of only hidden

Update release-notes area to security

9a1da34

Make grammatical corrections

5963376

Rebased and ran make gen

fc930dc

Kmoneal force-pushed the insecure-skip-verify branch from 9576ab4 to fc930dc Compare July 9, 2021 19:07

nrjpoddar reviewed Jul 14, 2021

View reviewed changes

Clarify release notes and documentation

2a89d28

nrjpoddar reviewed Jul 14, 2021

View reviewed changes

Improve documentation clarity

4507a47

nrjpoddar approved these changes Jul 14, 2021

View reviewed changes

howardjohn reviewed Jul 19, 2021

View reviewed changes

networking/v1beta1/destination_rule.proto Outdated Show resolved Hide resolved

linsun reviewed Jul 19, 2021

View reviewed changes

networking/v1alpha3/destination_rule.proto Outdated Show resolved Hide resolved

linsun reviewed Jul 19, 2021

View reviewed changes

louiscryan reviewed Jul 19, 2021

View reviewed changes

Fix typo

325d590

Kmoneal requested review from howardjohn, linsun and louiscryan July 20, 2021 15:24

Make clarification edits

6b6f4b4

nrjpoddar reviewed Jul 21, 2021

View reviewed changes

howardjohn approved these changes Jul 21, 2021

View reviewed changes

howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Jul 21, 2021

Improve description of InsecureSkipVerify

fd66530

howardjohn approved these changes Jul 27, 2021

View reviewed changes

Correct documentation

c298f40

Kmoneal requested a review from howardjohn July 29, 2021 15:16

howardjohn removed the do-not-merge/hold Block automatic merging of a PR. label Jul 29, 2021

istio-testing merged commit 0412822 into istio:master Jul 29, 2021

nrjpoddar reviewed Jul 30, 2021

View reviewed changes

howardjohn mentioned this pull request Jun 22, 2023

Implement DestinationRule.InsecureSkipVerify istio/istio#45611

Closed

	// the proxy will skip verification of the host's certificate regardless of
	// the proxy will skip verification of the host's certificate, regardless of

		// This flag should only be set if global CA signature verifcation is
		// enabled, `VerifyCertAtClient` environmental variable is set to `true`,

Introduce InsecureSkipVerify to DR #2040

Introduce InsecureSkipVerify to DR #2040

Conversation

Kmoneal commented Jun 29, 2021

istio-testing commented Jun 29, 2021

jacob-delgado commented Jun 30, 2021

brian-avery commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigbox commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kmoneal commented Jul 2, 2021

hzxuzhonghu commented Jul 2, 2021

nrjpoddar left a comment

Choose a reason for hiding this comment

nrjpoddar Jul 2, 2021 • edited

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

nrjpoddar commented Jul 14, 2021

Kmoneal commented Jul 15, 2021

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

nrjpoddar Jul 19, 2021 • edited

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

nrjpoddar Jul 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrjpoddar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kmoneal Jul 21, 2021 • edited

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 Jul 21, 2021

nrjpoddar commented Jul 26, 2021

Choose a reason for hiding this comment

nrjpoddar left a comment

Choose a reason for hiding this comment

nrjpoddar Jul 30, 2021 • edited

Choose a reason for hiding this comment

nrjpoddar Jul 30, 2021 • edited

Choose a reason for hiding this comment

nrjpoddar Jul 2, 2021 •

edited

nrjpoddar Jul 19, 2021 •

edited

nrjpoddar Jul 19, 2021 •

edited

Kmoneal Jul 21, 2021 •

edited

nrjpoddar Jul 30, 2021 •

edited

nrjpoddar Jul 30, 2021 •

edited