Add `insecure` property for NutanixDatacenterConfig #4171

thunderboltsid · 2022-11-21T17:00:26Z

To accommodate for the scenario where the user has not replaced the default Certificate that ships with Prism Central, we allow the user to skip TLS verification. Insecure is the optional property to skip TLS verification.

Nutanix Prism Central installation by default ships with a self-signed certificate that will fail TLS verification because of two reasons:

The certificate is not issued by a publicly trusted CA
The certificate does not have the IP SANs for the Prism Central endpoint

As such, in order for these services to establish a connection with Prism Central, client has three options:

Use Legitimate certificates: Replace the default certificate that comes with Prism Central with a certificate signed by a publicly trusted Certificate Authority e.g. Let’s Encrypt.
Use valid Self-Signed certificates: Replace the default certificate that comes with Prism central with a self-signed certificate that has the IP address/domain populated in the Subject Alternative Names section of the certificate. The Root CA Certificate is provided as an additional trust bundle to add to the certificate pool used to establish the connection. This is signaled using the additionalTrustBundle flag.
Skip TLS Verification: The TLS Verification step needs to be skipped when establishing the connection. The benefit of this approach is that it does not require the default certificate to be replaced.

We intend on supporting all three options until Prism Central starts shipping with valid self-signed certificates.
The recommended order from a customer guidance perspective should be:

Use a Legitimate certificate
Use a valid self-signed certificate and provide the CA cert through the trust bundle
Skip TLS verification

From an application evaluation perspective, the order will be:

If a legitimate certificate is present, nothing needs to be done
If an additionalTrustBundle flag is present, the RootCA cert should be added to the client cert pool when establishing a TLS connection.
If an insecure flag is present and set to true, TLS verification should be skipped regardless of what other properties are set.

How was this tested?
Created a EKS-A cluster on a Nutanix setup (@vnephologist lab) with self-signed certificates using the insecure flag set to true.

eks-distro-bot · 2022-11-21T17:00:38Z

Hi @thunderboltsid. Thanks for your PR.

I'm waiting for a aws 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.

codecov · 2022-11-21T17:07:30Z

Codecov Report

Merging #4171 (5002a55) into main (5a6d9da) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4171   +/-   ##
=======================================
  Coverage   68.27%   68.28%           
=======================================
  Files         404      404           
  Lines       32656    32664    +8     
=======================================
+ Hits        22295    22303    +8     
  Misses       8926     8926           
  Partials     1435     1435

Impacted Files	Coverage Δ
pkg/api/v1alpha1/nutanixdatacenterconfig_types.go	`100.00% <ø> (ø)`
internal/pkg/api/nutanix.go	`82.40% <100.00%> (+0.58%)`	⬆️
pkg/dependencies/factory.go	`71.94% <100.00%> (+0.03%)`	⬆️
pkg/providers/nutanix/template.go	`88.80% <100.00%> (ø)`
pkg/providers/nutanix/validator.go	`97.84% <100.00%> (+0.04%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

deepakm-ntnx

pls check my comments before merging

pkg/api/v1alpha1/nutanixdatacenterconfig_types.go

pkg/providers/nutanix/validator.go

eks-distro-bot · 2022-11-21T22:05:13Z

@deepakm-ntnx: changing LGTM is restricted to collaborators

In response to this:

pls check my comments before merging

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.

deepakm-ntnx · 2022-11-21T22:29:15Z

just for completeness, noting that this was added thru https://github.com/aws/eks-anywhere/pull/2229/files#diff-f06dc4da7f8574dbb4b599dc7e0fe78fd2c6698991b7659dd23a730d43ba5435R29 but was removed later thru https://github.com/aws/eks-anywhere/pull/3116/files#diff-f06dc4da7f8574dbb4b599dc7e0fe78fd2c6698991b7659dd23a730d43ba5435 due to concerns from EKS-A Team but since this is a supported way in other providers as well, adding it back to be consistent

thunderboltsid · 2022-11-22T07:19:04Z

just for completeness, noting that this was added thru https://github.com/aws/eks-anywhere/pull/2229/files#diff-f06dc4da7f8574dbb4b599dc7e0fe78fd2c6698991b7659dd23a730d43ba5435R29 but was removed later thru https://github.com/aws/eks-anywhere/pull/3116/files#diff-f06dc4da7f8574dbb4b599dc7e0fe78fd2c6698991b7659dd23a730d43ba5435 due to concerns from EKS-A Team but since this is a supported way in other providers as well, adding it back to be consistent

This was not raised by EKS-A team as a concern but was removed for following better engineering practices by us. However, following the discussion with our product team, and relaying feedback from @abhinavmpandey08 to support both insecure as well as additional trust bundle, we intend on providing support for skipping TLS verification as outlined in the PR description.

thunderboltsid · 2022-11-22T08:26:44Z

/assign @abhay-krishna @pokearu

pokearu · 2022-11-22T19:02:34Z

/ok-to-test

pkg/dependencies/factory.go

chrisdoherty4 · 2022-11-29T13:02:33Z

pkg/api/v1alpha1/nutanixdatacenterconfig_types.go

@@ -26,6 +26,14 @@ type NutanixDatacenterConfigSpec struct {
 	// AdditionalTrustBundle is the optional PEM-encoded certificate bundle for users that
 	// configured their Prism Central with certificates from non-publicly trusted CAs
 	AdditionalTrustBundle string `json:"additionalTrustBundle,omitempty"`
+
+	// Insecure is the optional flag to skip TLS verification. Nutanix Prism Central installation by default ships


Could we break these comment lines consistently with the other comment lines.

@chrisdoherty4 updated the comments. thanks

chrisdoherty4

lgtm with a small comment. Try to limit/have a consistent line length for the reasons outlined in https://github.com/aws/eks-anywhere/blob/main/docs/developer/best-practice.md#line-length.

Insecure is the optional property to skip TLS verification. Nutanix Prism installs by default ship with a self-signed certificate that will fail TLS verification because of two reasons: 1. The certificate is not issued by a public CA 2. The certificate does not have the IP SANs for the Prism Central endpoint To accommodate for the scenario where the user has not changed the default Certificate that ships with Prism Central, we allow the user to skip TLS verification. This is not recommended for production use as skipping TLS verification opens up the user to potential MITM attacks.

abhinavmpandey08

/approve
/lgtm

eks-distro-bot · 2022-11-29T23:09:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavmpandey08

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavmpandey08]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This reverts commit bba5709.

* Add `insecure` property for NutanixDatacenterConfig Insecure is the optional property to skip TLS verification. Nutanix Prism installs by default ship with a self-signed certificate that will fail TLS verification because of two reasons: 1. The certificate is not issued by a public CA 2. The certificate does not have the IP SANs for the Prism Central endpoint To accommodate for the scenario where the user has not changed the default Certificate that ships with Prism Central, we allow the user to skip TLS verification. This is not recommended for production use as skipping TLS verification opens up the user to potential MITM attacks. * Updated the insecure property description based on review feedback * Add a unit test for validating insecure property * updated comment to fit the standard length Co-authored-by: deepakm-ntnx <deepak.muley@nutanix.com>

eks-distro-bot added the needs-ok-to-test label Nov 21, 2022

eks-distro-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 21, 2022

thunderboltsid force-pushed the nutanix-insecure branch 2 times, most recently from e62ca41 to 2bd3ea0 Compare November 21, 2022 17:21

deepakm-ntnx approved these changes Nov 21, 2022

View reviewed changes

pkg/api/v1alpha1/nutanixdatacenterconfig_types.go Outdated Show resolved Hide resolved

pkg/providers/nutanix/validator.go Outdated Show resolved Hide resolved

thunderboltsid requested a review from deepakm-ntnx November 22, 2022 07:36

thunderboltsid force-pushed the nutanix-insecure branch 2 times, most recently from 2edac19 to 2caaafb Compare November 22, 2022 07:38

eks-distro-bot assigned abhay-krishna and pokearu Nov 22, 2022

thunderboltsid force-pushed the nutanix-insecure branch from 2caaafb to 900b3ee Compare November 22, 2022 08:27

thunderboltsid mentioned this pull request Nov 22, 2022

EKS-A Nutanix Provider Action Items #4167

Open

15 tasks

eks-distro-bot added ok-to-test and removed needs-ok-to-test labels Nov 22, 2022

abhinavmpandey08 reviewed Nov 28, 2022

View reviewed changes

pkg/dependencies/factory.go Show resolved Hide resolved

chrisdoherty4 reviewed Nov 29, 2022

View reviewed changes

chrisdoherty4 approved these changes Nov 29, 2022

View reviewed changes

eks-distro-bot assigned chrisdoherty4 Nov 29, 2022

eks-distro-bot added the lgtm label Nov 29, 2022

abhinavmpandey08 added the area/providers/nutanix label Nov 29, 2022

thunderboltsid added 3 commits November 29, 2022 14:58

Updated the insecure property description based on review feedback

a582f96

Add a unit test for validating insecure property

b20333b

updated comment to fit the standard length

5002a55

deepakm-ntnx force-pushed the nutanix-insecure branch from 97ab715 to 5002a55 Compare November 29, 2022 23:00

eks-distro-bot removed the lgtm label Nov 29, 2022

abhinavmpandey08 approved these changes Nov 29, 2022

View reviewed changes

eks-distro-bot assigned abhinavmpandey08 Nov 29, 2022

eks-distro-bot added the lgtm label Nov 29, 2022

eks-distro-bot added the approved label Nov 29, 2022

eks-distro-bot merged commit bba5709 into aws:main Nov 29, 2022

deepakm-ntnx added a commit to nutanix-cloud-native/aws-eks-anywhere that referenced this pull request Nov 30, 2022

Revert "Add insecure property for NutanixDatacenterConfig (aws#4171)"

beeeb04

This reverts commit bba5709.

deepakm-ntnx mentioned this pull request Nov 30, 2022

Revert "Add insecure property for NutanixDatacenterConfig" #4242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `insecure` property for NutanixDatacenterConfig #4171

Add `insecure` property for NutanixDatacenterConfig #4171

thunderboltsid commented Nov 21, 2022 •

edited

Loading

eks-distro-bot commented Nov 21, 2022

codecov bot commented Nov 21, 2022 •

edited

Loading

deepakm-ntnx left a comment

eks-distro-bot commented Nov 21, 2022

deepakm-ntnx commented Nov 21, 2022

thunderboltsid commented Nov 22, 2022 •

edited

Loading

thunderboltsid commented Nov 22, 2022

pokearu commented Nov 22, 2022

chrisdoherty4 Nov 29, 2022

deepakm-ntnx Nov 29, 2022

chrisdoherty4 left a comment

abhinavmpandey08 left a comment

eks-distro-bot commented Nov 29, 2022

Add insecure property for NutanixDatacenterConfig #4171

Add insecure property for NutanixDatacenterConfig #4171

Conversation

thunderboltsid commented Nov 21, 2022 • edited Loading

eks-distro-bot commented Nov 21, 2022

codecov bot commented Nov 21, 2022 • edited Loading

Codecov Report

deepakm-ntnx left a comment

Choose a reason for hiding this comment

eks-distro-bot commented Nov 21, 2022

deepakm-ntnx commented Nov 21, 2022

thunderboltsid commented Nov 22, 2022 • edited Loading

thunderboltsid commented Nov 22, 2022

pokearu commented Nov 22, 2022

chrisdoherty4 Nov 29, 2022

Choose a reason for hiding this comment

deepakm-ntnx Nov 29, 2022

Choose a reason for hiding this comment

chrisdoherty4 left a comment

Choose a reason for hiding this comment

abhinavmpandey08 left a comment

Choose a reason for hiding this comment

eks-distro-bot commented Nov 29, 2022

Add `insecure` property for NutanixDatacenterConfig #4171

Add `insecure` property for NutanixDatacenterConfig #4171

thunderboltsid commented Nov 21, 2022 •

edited

Loading

codecov bot commented Nov 21, 2022 •

edited

Loading

thunderboltsid commented Nov 22, 2022 •

edited

Loading