Skip to content

Commit

Permalink
Fix cert update by making certificate fields mutually exclusive (#663)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mpanchajanya
mpanchajanya authored Jan 31, 2024
1 parent 1c3a879 commit 4e35d78
Show file tree
Hide file tree
Showing 2 changed files with 170 additions and 31 deletions.
126 changes: 101 additions & 25 deletions pkg/command/cert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"github.com/vmware-tanzu/tanzu-cli/pkg/utils"
)

const FalseStr = "false"

var (
host, caCertPathForAdd, skipCertVerifyForAdd, insecureForAdd string
caCertPathForUpdate, skipCertVerifyForUpdate, insecureForUpdate string
Expand Down Expand Up @@ -66,10 +68,10 @@ func newCertCmd() *cobra.Command {
// The completion for this flag is simple file completion, which is configured by default
addCertCmd.Flags().StringVarP(&caCertPathForAdd, "ca-cert", "", "", "path to the public certificate")

addCertCmd.Flags().StringVarP(&skipCertVerifyForAdd, "skip-cert-verify", "", "false", "skip server's TLS certificate verification")
addCertCmd.Flags().StringVarP(&skipCertVerifyForAdd, "skip-cert-verify", "", FalseStr, "skip server's TLS certificate verification")
utils.PanicOnErr(addCertCmd.RegisterFlagCompletionFunc("skip-cert-verify", compSkipFlag))

addCertCmd.Flags().StringVarP(&insecureForAdd, "insecure", "", "false", "allow the use of http when interacting with the host")
addCertCmd.Flags().StringVarP(&insecureForAdd, "insecure", "", FalseStr, "allow the use of http when interacting with the host")
utils.PanicOnErr(addCertCmd.RegisterFlagCompletionFunc("insecure", compInsecureFlag))

utils.PanicOnErr(cobra.MarkFlagRequired(addCertCmd.Flags(), "host"))
Expand Down Expand Up @@ -149,19 +151,24 @@ func newAddCertCmd() *cobra.Command {
ValidArgsFunction: completeAddCert,
RunE: func(cmd *cobra.Command, args []string) error {
if skipCertVerifyForAdd != "" {
if !strings.EqualFold(skipCertVerifyForAdd, "true") && !strings.EqualFold(skipCertVerifyForAdd, "false") {
if !strings.EqualFold(skipCertVerifyForAdd, "true") && !strings.EqualFold(skipCertVerifyForAdd, FalseStr) {
return errors.Errorf("incorrect boolean argument for '--skip-cert-verify' option : %q", skipCertVerifyForAdd)
}
}
if insecureForAdd != "" {
if !strings.EqualFold(insecureForAdd, "true") && !strings.EqualFold(insecureForAdd, "false") {
if !strings.EqualFold(insecureForAdd, "true") && !strings.EqualFold(insecureForAdd, FalseStr) {
return errors.Errorf("incorrect boolean argument for '--insecure' option : %q", insecureForAdd)
}
}
if strings.EqualFold(skipCertVerifyForAdd, "false") && strings.EqualFold(insecureForAdd, "false") &&
if strings.EqualFold(skipCertVerifyForAdd, FalseStr) && strings.EqualFold(insecureForAdd, FalseStr) &&
caCertPathForAdd == "" {
return errors.New("please specify at least one additional valid option apart from '--host' ")
return errors.New("please specify at least one additional valid option apart from '--host'")
}

if !strings.EqualFold(skipCertVerifyForAdd, FalseStr) && caCertPathForAdd != "" {
return errors.New("please specify only one valid option either '--skip-cert-verify' or '--ca-cert'")
}

certExistError := fmt.Errorf("certificate configuration for host %q already exist", host)
exits, _ := configlib.CertExists(host)
if exits {
Expand Down Expand Up @@ -203,35 +210,39 @@ func newUpdateCertCmd() *cobra.Command {
tanzu config cert update test.vmware.com --insecure true`,
ValidArgsFunction: completeCertHosts,
RunE: func(cmd *cobra.Command, args []string) error {
if skipCertVerifyForUpdate != "" {
if !strings.EqualFold(skipCertVerifyForUpdate, "true") && !strings.EqualFold(skipCertVerifyForUpdate, "false") {
return errors.Errorf("incorrect boolean argument for '--skip-cert-verify' option : %q", skipCertVerifyForUpdate)
}
}
if insecureForUpdate != "" {
if !strings.EqualFold(insecureForUpdate, "true") && !strings.EqualFold(insecureForUpdate, "false") {
return errors.Errorf("incorrect boolean argument for '--insecure' option : %q", insecureForUpdate)
}
if err := validateUpdateOptions(); err != nil {
return err
}
if skipCertVerifyForUpdate == "" && insecureForUpdate == "" && caCertPathForUpdate == "" {
return errors.New("please specify at least one update option ")

uHost := args[0]
existingCert, _ := configlib.GetCert(uHost)

if existingCert == nil {
return fmt.Errorf("certificate configuration for host %q does not exist", uHost)
}
aHost := args[0]
certNoExistError := fmt.Errorf("certificate configuration for host %q does not exist", aHost)
exits, _ := configlib.CertExists(aHost)
if !exits {
return certNoExistError

updCert := &configtypes.Cert{
Host: uHost,
CACertData: existingCert.CACertData,
SkipCertVerify: existingCert.SkipCertVerify,
Insecure: existingCert.Insecure,
}
cert, err := createCert(aHost, caCertPathForUpdate, skipCertVerifyForUpdate, insecureForUpdate)

updCert, err := updateCert(updCert, caCertPathForUpdate, skipCertVerifyForUpdate, insecureForUpdate)
if err != nil {
return err
}

err = configlib.SetCert(cert)
if err := configlib.DeleteCert(uHost); err != nil {
return err
}

err = configlib.SetCert(updCert)
if err != nil {
return err
}
log.Successf("updated certificate data for host %s", aHost)

log.Successf("updated certificate data for host %s", uHost)
return nil
},
}
Expand Down Expand Up @@ -287,6 +298,71 @@ func createCert(host, caCertPath, skipCertVerify, insecure string) (*configtypes
return cert, nil
}

func updateCert(updateCert *configtypes.Cert, caCertPath, skipCertVerify, insecure string) (*configtypes.Cert, error) {
if caCertPath != "" {
fileBytes, err := os.ReadFile(caCertPath)
if err != nil {
return nil, errors.Wrapf(err, "error reading CA certificate file %s", caCertPath)
}
updateCert.CACertData = base64.StdEncoding.EncodeToString(fileBytes)
// Reset skip cert verify if ca cert is specified
updateCert.SkipCertVerify = FalseStr
}

if skipCertVerify != "" {
updateCert.SkipCertVerify = skipCertVerify
// Reset ca cert data is skip cert is specified
if skipCertVerify != FalseStr {
updateCert.CACertData = ""
}
}

if insecure != "" {
updateCert.Insecure = insecure
}
return updateCert, nil
}

func validateUpdateOptions() error {
if err := validateBooleanOption(skipCertVerifyForUpdate, "--skip-cert-verify"); err != nil {
return err
}

if err := validateBooleanOption(insecureForUpdate, "--insecure"); err != nil {
return err
}

return validateUpdateOptionCombination()
}

func validateBooleanOption(value, option string) error {
if value != "" && !strings.EqualFold(value, "true") && !strings.EqualFold(value, FalseStr) {
return errors.Errorf("incorrect boolean argument for '%s' option: %q", option, value)
}
return nil
}

func validateUpdateOptionCombination() error {
if areNoUpdateOptionsSpecified() {
return errors.New("please specify at least one update option")
}

if areBothSkipCertAndCACertSpecified() {
return errors.New("please specify only one valid option either '--skip-cert-verify' or '--ca-cert'")
}

return nil
}

func areNoUpdateOptionsSpecified() bool {
return (skipCertVerifyForUpdate == "" || strings.EqualFold(skipCertVerifyForUpdate, FalseStr)) &&
insecureForUpdate == "" && caCertPathForUpdate == ""
}

func areBothSkipCertAndCACertSpecified() bool {
return skipCertVerifyForUpdate != "" && !strings.EqualFold(skipCertVerifyForUpdate, FalseStr) && caCertPathForUpdate != ""
}

// ====================================
// Shell completion functions
// ====================================
Expand Down
75 changes: 69 additions & 6 deletions pkg/command/cert_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,33 +57,65 @@ var _ = Describe("config cert command tests", func() {
resetCertCommandFlags()
})
Context("config cert add with all the options", func() {
It("should be success and cert list should return the cert successfully", func() {

It("should error add cert with both ca cert and skip cert verify", func() {
certCmd := newCertCmd()
certCmd.SetArgs([]string{
"add", "--host", testHost, "--ca-cert", caCertFile.Name(),
"--skip-cert-verify", "true", "--insecure", "true"})
err = certCmd.Execute()
Expect(err.Error()).To(Equal("please specify only one valid option either '--skip-cert-verify' or '--ca-cert'"))

certs := listCerts()
Expect(certs).NotTo(ContainElement(certOutputRow{
Host: testHost,
CACertificate: "<REDACTED>",
SkipCertVerification: "true",
Insecure: "true",
}))
})

It("should be success and cert list should return the cert successfully", func() {
certCmd := newCertCmd()
certCmd.SetArgs([]string{"add", "--host", testHost, "--ca-cert", caCertFile.Name(), "--insecure", "true"})
err = certCmd.Execute()
Expect(err).To(BeNil())

certs := listCerts()
Expect(certs).To(ContainElement(certOutputRow{
Host: testHost,
CACertificate: "<REDACTED>",
SkipCertVerification: "true",
SkipCertVerification: "false",
Insecure: "true",
}))
})

It("should be success with skip cert verify false and cert list should return the cert successfully", func() {
certCmd := newCertCmd()
certCmd.SetArgs([]string{"add", "--host", testHost, "--ca-cert", caCertFile.Name(), "--skip-cert-verify", "false", "--insecure", "true"})
err = certCmd.Execute()
Expect(err).To(BeNil())

certs := listCerts()
Expect(certs).To(ContainElement(certOutputRow{
Host: testHost,
CACertificate: "<REDACTED>",
SkipCertVerification: "false",
Insecure: "true",
}))
})

It("should return error if the cert for a host already exists", func() {
certCmd := newCertCmd()
certCmd.SetArgs([]string{
"add", "--host", testHost, "--ca-cert", caCertFile.Name(),
"--skip-cert-verify", "true", "--insecure", "true"})
"--skip-cert-verify", "false", "--insecure", "true"})
err = certCmd.Execute()
Expect(err).To(BeNil())

certCmd.SetArgs([]string{
"add", "--host", testHost, "--ca-cert", caCertFile.Name(),
"--skip-cert-verify", "true", "--insecure", "false"})
"--skip-cert-verify", "false", "--insecure", "false"})
err = certCmd.Execute()
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring(`certificate configuration for host "test.vmware.com" already exist`))
Expand Down Expand Up @@ -174,6 +206,36 @@ var _ = Describe("config cert command tests", func() {
Expect(err).To(BeNil())
Expect(cert.SkipCertVerify).To(Equal("true"))
Expect(cert.Insecure).To(Equal("true"))
Expect(cert.CACertData).To(Equal(""))

})

It("should not update the 'skipCertVerify' and 'insecure' config data successfully", func() {
certCmd := newCertCmd()
certCmd.SetArgs([]string{
"add", "--host", testHost, "--ca-cert", caCertFile.Name(),
"--skip-cert-verify", "false", "--insecure", "false"})
err = certCmd.Execute()
Expect(err).To(BeNil())

gotCAData := getConfigCertData(testHost)
Expect(gotCAData).To(Equal(fakeCACertData))

cert, err := configlib.GetCert(testHost)
Expect(err).To(BeNil())
Expect(cert.SkipCertVerify).To(Equal("false"))
Expect(cert.Insecure).To(Equal("false"))

// update the SkipCertVerify and Insecure configuration
certCmd.SetArgs([]string{
"update", testHost, "--ca-cert", caCertFile.Name(), "--skip-cert-verify", "true", "--insecure", "true"})
err = certCmd.Execute()
Expect(err.Error()).To(Equal("please specify only one valid option either '--skip-cert-verify' or '--ca-cert'"))

cert, err = configlib.GetCert(testHost)
Expect(err).To(BeNil())
Expect(cert.SkipCertVerify).To(Equal("false"))
Expect(cert.Insecure).To(Equal("false"))

})
})
Expand All @@ -182,15 +244,15 @@ var _ = Describe("config cert command tests", func() {
certCmd := newCertCmd()
certCmd.SetArgs([]string{
"add", "--host", testHost, "--ca-cert", caCertFile.Name(),
"--skip-cert-verify", "true", "--insecure", "true"})
"--skip-cert-verify", "false", "--insecure", "true"})
err = certCmd.Execute()
Expect(err).To(BeNil())

certs := listCerts()
Expect(certs).To(ContainElement(certOutputRow{
Host: testHost,
CACertificate: "<REDACTED>",
SkipCertVerification: "true",
SkipCertVerification: "false",
Insecure: "true",
}))

Expand Down Expand Up @@ -234,6 +296,7 @@ func listCerts() []certOutputRow {
return certs
}

//nolint:unparam
func getConfigCertData(host string) string {
cert, err := configlib.GetCert(host)
Expect(err).To(BeNil())
Expand Down

0 comments on commit 4e35d78

Please sign in to comment.