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

Fix cert update by making certificate fields mutually exclusive #663

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

mpanchajanya
Copy link
Contributor

@mpanchajanya mpanchajanya commented Jan 30, 2024

What this PR does / why we need it

  • Implement tanzu config cert update command when ca-cert and skip-cert-verify being mutually exclusive i.e. only one of the option should be set

Examples of valid and invalid usages: -

Invalid usage - tanzu config cert update --ca-cert test.cert --skip-cert-verify true
Valid usage - tanzu config cert update --ca-cert test.cert --skip-cert-verify false
Valid usage - tanzu config cert update --ca-cert test.cert
Valid usage - tanzu config cert update --skip-cert-verify false
Valid usage - tanzu config cert update --skip-cert-verify true

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

-- Added unit tests

mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz version
version: v1.2.0-dev
buildDate: 2024-01-30
sha: d53572db-dirty
arch: arm64
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert list
[i] Refreshing plugin inventory cache for "projects.registry.vmware.com/tanzu_cli/plugins/plugin-inventory:latest", this will take a few seconds.
  HOST            CA-CERTIFICATE  SKIP-CERT-VERIFICATION  INSECURE  
  localhost:9876  <REDACTED>      false                   false     
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert add --host test.vmware.com --ca-cert test.cert --skip-cert-verify true
[x] : please specify only one valid option either '--skip-cert-verify' or '--ca-cert'
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update) [1]> tz config cert add --host test.vmware.com --ca-cert test.cert --skip-cert-verify false
[ok] successfully added certificate data for host test.vmware.com
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert add --host test.vmware.com --ca-cert test.cert
[x] : certificate configuration for host "test.vmware.com" already exist
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update) [1]> tz config cert add --host testv2.vmware.com --ca-cert test.cert
[ok] successfully added certificate data for host testv2.vmware.com
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert list
  HOST               CA-CERTIFICATE  SKIP-CERT-VERIFICATION  INSECURE  
  localhost:9876     <REDACTED>      false                   false     
  test.vmware.com    <REDACTED>      false                   false     
  testv2.vmware.com  <REDACTED>      false                   false     
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert update test.vmware.com --ca-cert test.cert
[ok] updated certificate data for host test.vmware.com
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert list
  HOST               CA-CERTIFICATE  SKIP-CERT-VERIFICATION  INSECURE  
  localhost:9876     <REDACTED>      false                   false     
  testv2.vmware.com  <REDACTED>      false                   false     
  test.vmware.com    <REDACTED>      false                   false     
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert update test.vmware.com --ca-cert test.cert --skip-cert-verify false
[ok] updated certificate data for host test.vmware.com
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert list
  HOST               CA-CERTIFICATE  SKIP-CERT-VERIFICATION  INSECURE  
  localhost:9876     <REDACTED>      false                   false     
  testv2.vmware.com  <REDACTED>      false                   false     
  test.vmware.com    <REDACTED>      false                   false     
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert update test.vmware.com --ca-cert test.cert --skip-cert-verify true
[x] : please specify only one valid option either '--skip-cert-verify' or '--ca-cert'
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update) [1]> tz config cert update test.vmware.com --skip-cert-verify true
[ok] updated certificate data for host test.vmware.com
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert list
  HOST               CA-CERTIFICATE  SKIP-CERT-VERIFICATION  INSECURE  
  localhost:9876     <REDACTED>      false                   false     
  testv2.vmware.com  <REDACTED>      false                   false     
  test.vmware.com    Not configured  true                    false     
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert update test.vmware.com --skip-cert-verify false
[x] : please specify at least one update option
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update) [1]> tz config cert update test.vmware.com --skip-cert-verify false --ca-cert test.cert 
[ok] updated certificate data for host test.vmware.com
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> tz config cert list
  HOST               CA-CERTIFICATE  SKIP-CERT-VERIFICATION  INSECURE  
  localhost:9876     <REDACTED>      false                   false     
  testv2.vmware.com  <REDACTED>      false                   false     
  test.vmware.com    <REDACTED>      false                   false     
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (bug-fix/cert-update)> 



Release note

tanzu config cert options --ca-cert and --skip-cert-verify are mutually exclusive

Additional information

Special notes for your reviewer

@mpanchajanya mpanchajanya self-assigned this Jan 30, 2024
@mpanchajanya mpanchajanya marked this pull request as ready for review January 30, 2024 12:28
@mpanchajanya mpanchajanya requested a review from a team as a code owner January 30, 2024 12:28
@marckhouzam
Copy link
Contributor

@mpanchajanya Do you know if the insecure setting should also be mutually exclusive?
Or is it ok to have both skip-verify/ca-cert configure along with insecure?

@mpanchajanya
Copy link
Contributor Author

@mpanchajanya Do you know if the insecure setting should also be mutually exclusive? Or is it ok to have both skip-verify/ca-cert configure along with insecure?

Yea I'm not sure about that. Tagging @prkalle he might have more insights on this feature

pkg/command/cert.go Outdated Show resolved Hide resolved
@prkalle
Copy link
Contributor

prkalle commented Jan 31, 2024

@mpanchajanya Do you know if the insecure setting should also be mutually exclusive? Or is it ok to have both skip-verify/ca-cert configure along with insecure?

Yea I'm not sure about that. Tagging @prkalle he might have more insights on this feature

If user use insecure, then http scheme would be used to interact with registry, then ca-cert and skip-verify configuration would be irrelavant. The issue that this PR is solving occured because the http transport would give priority to RootCAs(ca-cert) if both RootCAs(ca-cert) and InsecureSkipVerify(skip-verify) options of http.Transport.TLSClientConfig are configured. Hence making ca-cert and skip-verify options of tanzu config cert add and tanzu config cert update command is necessary.

Copy link
Contributor

@prkalle prkalle left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@marckhouzam
Copy link
Contributor

marckhouzam commented Jan 31, 2024

If user use insecure, then http scheme would be used to interact with registry, then ca-cert and skip-verify configuration would be irrelevant.

So should they be removed from the configuration?
If both are configured, the user may be confused as to which takes precedence.

@mpanchajanya
Copy link
Contributor Author

If user use insecure, then http scheme would be used to interact with registry, then ca-cert and skip-verify configuration would be irrelevant.

So should they be removed from the configuration? If both are configured, the user may be confused as to which takes precedence.

@prkalle Any thoughts ?

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Just the comments to fix.
LGTM after that

pkg/command/cert.go Outdated Show resolved Hide resolved
pkg/command/cert.go Outdated Show resolved Hide resolved
@mpanchajanya mpanchajanya merged commit 4e35d78 into vmware-tanzu:main Jan 31, 2024
7 checks passed
@marckhouzam marckhouzam added this to the v1.2.0 milestone Feb 1, 2024
@marckhouzam marckhouzam added the kind/bug PR/issue related to a bug label Feb 1, 2024
@marckhouzam
Copy link
Contributor

If user use insecure, then http scheme would be used to interact with registry, then ca-cert and skip-verify configuration would be irrelevant.

So should they be removed from the configuration? If both are configured, the user may be confused as to which takes precedence.

We have discussed this verbally and it was not clear if the ca-cert could end up being used if an insecure connection was redirected by a proxy to an https one. So, to avoid limiting the user's options, we decided to allow setting insecure and either other two options at the same time. A follow-up PR will document that insecure takes priority.

mpanchajanya added a commit that referenced this pull request Feb 7, 2024
mpanchajanya added a commit that referenced this pull request Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required kind/bug PR/issue related to a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants