Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove mentions of VERIFY_CERTIFICATE_AT_CLIENT and ENABLE_AUTO_SNI #3122

Merged
merged 3 commits into from
Mar 16, 2024

Conversation

leosarra
Copy link
Contributor

@leosarra leosarra commented Mar 12, 2024

Starting with Istio 1.21 VERIFY_CERTIFICATE_AT_CLIENT and ENABLE_AUTO_SNI are true by default.
Some comments had to be updated

@leosarra leosarra requested a review from a team as a code owner March 12, 2024 13:58
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 12, 2024
@leosarra leosarra added release-notes-none Indicates a PR that does not require release notes. cherrypick/release-1.21 Set this label on a PR to auto-merge it to the release-1.21 branch labels Mar 12, 2024
// `VERIFY_CERTIFICATE_AT_CLIENT` is `false` by default in Istio version 1.9 but will
// be `true` by default in a later version where, going forward, it will be
// enabled by default.
// `insecureSkipVerify` is `false` by default while `VERIFY_CERTIFICATE_AT_CLIENT` is `true` by default.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the relationship with VERIFY_CERTIFICATE_AT_CLIENT looks very confusing, would cause usage trouble

Copy link
Contributor Author

@leosarra leosarra Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree. Overall the VERIFY_CERTIFICATE_AT_CLIENT makes the doc a bit more confusing for the user.
On the other end we can look forward to the removal of VERIFY_CERTIFICATE_AT_CLIENT once the deprecation process is completed (given that it is already in the compatibility values starting with Istio 1.21, it is planned for removal in 1.24).

Perhaps this field can be simplified as follows:

// The `insecureSkipVerify` flag determines whether the proxy should skip verifying the
// CA signature and SAN for the server certificate associated
// with a specific host. This field is ignored if the environment variable
// `VERIFY_CERTIFICATE_AT_CLIENT` is set to `false` (default value is `true`),
// as the verification of the CA signature and SAN will be bypassed regardless.

Do you think it is better now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the code, it only collaborate with VERIFY_CERTIFICATE_AT_CLIENT when deciding whether to enable AutoSanValidation.

func (cb *ClusterBuilder) setAutoSniAndAutoSanValidation(mc *clusterWrapper, tls *networking.ClientTLSSettings) {
	if mc == nil || !features.EnableAutoSni {
		return
	}

	setAutoSni := false
	setAutoSanValidation := false
	if len(tls.Sni) == 0 {
		setAutoSni = true
	}
	if features.VerifyCertAtClient && setAutoSni && len(tls.SubjectAltNames) == 0 && !tls.GetInsecureSkipVerify().GetValue() {
		setAutoSanValidation = true
	}

So the saying is not right

Copy link
Contributor Author

@leosarra leosarra Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, insecure skip is not always ignored since it is used also for enabling/disabling auto san validation.
I will update the message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we can just remove any mention of VERIFY_CERTIFICATE_AT_CLIENT. It was a temporary feature flag that is now default-enabled and will eventually be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM without mentioning VERIFY_CERTIFICATE_AT_CLIENT

Copy link
Contributor Author

@leosarra leosarra Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed mentions of both VERIFY_CERTIFICATE_AT_CLIENT and ENABLE_AUTO_SNI.
I think it is fine to remove the mentions of ENABLE_AUTO_SNI since it is going through the same deprecation process as VERIFY_CERTIFICATE_AT_CLIENT.

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 15, 2024
@leosarra leosarra changed the title Update insecureSkipVerify comment Remove mentions of VERIFY_CERTIFICATE_AT_CLIENT and ENABLE_AUTO_SNI Mar 15, 2024
@istio-testing istio-testing merged commit d29365c into istio:master Mar 16, 2024
5 checks passed
@istio-testing
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick/release-1.21 Set this label on a PR to auto-merge it to the release-1.21 branch release-notes-none Indicates a PR that does not require release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants