Do not allow mixing of flags in renew cert (#947)

* Do not allow mixing of flags in renew cert

Signed-off-by: Pravin Pushkar <[email protected]>

* Review comments fixes

Signed-off-by: Pravin Pushkar <[email protected]>

* check cert loaded in mtls expiry

Signed-off-by: Pravin Pushkar <[email protected]>

Author: pravinpushkar

cmd/renew_certificate.go

@@ -16,6 +16,7 @@ package cmd
 import (
 	"fmt"
 	"os"
+	"strings"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -47,17 +48,29 @@ dapr mtls renew-certificate -k --valid-until <no of days> --restart
 dapr mtls renew-certificate -k --private-key myprivatekey.key --valid-until <no of days>
 
 # Rotates certificate of your kubernetes cluster with provided ca.cert, issuer.crt and issuer.key file path
-dapr mtls renew-certificate -k --ca-root-certificate <ca.crt> --issuer-private-key <issuer.key> --issuer-public-certificate <issuer.crt> --restart
+dapr mtls renew-certificate -k --ca-root-certificate <root.pem> --issuer-private-key <issuer.key> --issuer-public-certificate <issuer.pem> --restart
 
 # See more at: https://docs.dapr.io/getting-started/
 `,
 
 		Run: func(cmd *cobra.Command, args []string) {
+			var err error
+			pkFlag := cmd.Flags().Lookup("private-key").Changed
+			rootcertFlag := cmd.Flags().Lookup("ca-root-certificate").Changed
+			issuerKeyFlag := cmd.Flags().Lookup("issuer-private-key").Changed
+			issuerCertFlag := cmd.Flags().Lookup("issuer-public-certificate").Changed
+
 			if kubernetesMode {
 				print.PendingStatusEvent(os.Stdout, "Starting certificate rotation")
-				if caRootCertificateFile != "" && issuerPrivateKeyFile != "" && issuerPublicCertificateFile != "" {
+				if rootcertFlag || issuerKeyFlag || issuerCertFlag {
+					flagArgsEmpty := checkReqFlagArgsEmpty(caRootCertificateFile, issuerPrivateKeyFile, issuerPublicCertificateFile)
+					if flagArgsEmpty {
+						err = fmt.Errorf("all required flags for this certificate rotation path, %q, %q and %q are not present",
+							"ca-root-certificate", "issuer-private-key", "issuer-public-certificate")
+						logErrorAndExit(err)
+					}
 					print.InfoStatusEvent(os.Stdout, "Using provided certificates")
-					err := kubernetes.RenewCertificate(kubernetes.RenewCertificateParams{
+					err = kubernetes.RenewCertificate(kubernetes.RenewCertificateParams{
 						RootCertificateFilePath:   caRootCertificateFile,
 						IssuerCertificateFilePath: issuerPublicCertificateFile,
 						IssuerPrivateKeyFilePath:  issuerPrivateKeyFile,
@@ -66,9 +79,14 @@ dapr mtls renew-certificate -k --ca-root-certificate <ca.crt> --issuer-private-k
 					if err != nil {
 						logErrorAndExit(err)
 					}
-				} else if privateKey != "" {
+				} else if pkFlag {
+					flagArgsEmpty := checkReqFlagArgsEmpty(privateKey)
+					if flagArgsEmpty {
+						err = fmt.Errorf("%q flag has incorrect value", "privateKey")
+						logErrorAndExit(err)
+					}
 					print.InfoStatusEvent(os.Stdout, "Using password file to generate root certificate")
-					err := kubernetes.RenewCertificate(kubernetes.RenewCertificateParams{
+					err = kubernetes.RenewCertificate(kubernetes.RenewCertificateParams{
 						RootPrivateKeyFilePath: privateKey,
 						ValidUntil:             time.Hour * time.Duration(validUntil*24),
 						Timeout:                timeout,
@@ -78,7 +96,7 @@ dapr mtls renew-certificate -k --ca-root-certificate <ca.crt> --issuer-private-k
 					}
 				} else {
 					print.InfoStatusEvent(os.Stdout, "generating fresh certificates")
-					err := kubernetes.RenewCertificate(kubernetes.RenewCertificateParams{
+					err = kubernetes.RenewCertificate(kubernetes.RenewCertificateParams{
 						ValidUntil: time.Hour * time.Duration(validUntil*24),
 						Timeout:    timeout,
 					})
@@ -118,8 +136,17 @@ dapr mtls renew-certificate -k --ca-root-certificate <ca.crt> --issuer-private-k
 	return command
 }
 
+func checkReqFlagArgsEmpty(params ...string) bool {
+	for _, val := range params {
+		if len(strings.TrimSpace(val)) == 0 {
+			return true
+		}
+	}
+	return false
+}
+
 func logErrorAndExit(err error) {
-	err = fmt.Errorf("certificate rotation failed %w", err)
+	err = fmt.Errorf("certificate rotation failed: %w", err)
 	print.FailureStatusEvent(os.Stderr, err.Error())
 	os.Exit(1)
 }

pkg/kubernetes/mtls.go

@@ -165,7 +165,10 @@ func Expiry() (*time.Time, error) {
 		return nil, err
 	}
 
-	caCrt := secret.Data["ca.crt"]
+	caCrt, ok := secret.Data["ca.crt"]
+	if !ok {
+		return nil, errors.New("root certificate not loaded yet, please try again in few minutes")
+	}
 	block, _ := pem.Decode(caCrt)
 	cert, err := x509.ParseCertificate(block.Bytes)
 	if err != nil {

tests/e2e/common/common.go

@@ -486,6 +486,50 @@ func UseProvidedNewCertAndRenew(details VersionDetails, opts TestOptions) func(t
 	}
 }
 
+func NegativeScenarioForCertRenew() func(t *testing.T) {
+	return func(t *testing.T) {
+		daprPath := getDaprPath()
+		args := []string{
+			"mtls", "renew-certificate", "-k",
+			"--ca-root-certificate", "invalid_cert_file.pem",
+		}
+		output, err := spawn.Command(daprPath, args...)
+		t.Log(output)
+		require.Error(t, err, "expected error on certificate renewal")
+		assert.Contains(t, output, "certificate rotation failed: all required flags for this certificate rotation path")
+
+		args = []string{
+			"mtls", "renew-certificate", "-k",
+			"--ca-root-certificate", "invalid_cert_file.pem",
+			"--issuer-private-key", "invalid_cert_key.pem",
+			"--issuer-public-certificate", "invalid_cert_file.pem",
+		}
+		output, err = spawn.Command(daprPath, args...)
+		t.Log(output)
+		require.Error(t, err, "expected error on certificate renewal")
+		assert.Contains(t, output, "certificate rotation failed: open invalid_cert_file.pem: no such file or directory")
+
+		args = []string{
+			"mtls", "renew-certificate", "-k",
+			"--ca-root-certificate", "invalid_cert_file.pem",
+			"--private-key", "invalid_root_key.pem",
+		}
+		output, err = spawn.Command(daprPath, args...)
+		t.Log(output)
+		require.Error(t, err, "expected error on certificate renewal")
+		assert.Contains(t, output, "certificate rotation failed: all required flags for this certificate rotation path")
+
+		args = []string{
+			"mtls", "renew-certificate", "-k",
+			"--private-key", "invalid_root_key.pem",
+		}
+		output, err = spawn.Command(daprPath, args...)
+		t.Log(output)
+		require.Error(t, err, "expected error on certificate renewal")
+		assert.Contains(t, output, "certificate rotation failed: open invalid_root_key.pem: no such file or directory")
+	}
+}
+
 func CheckMTLSStatus(details VersionDetails, opts TestOptions, shouldWarningExist bool) func(t *testing.T) {
 	return func(t *testing.T) {
 		daprPath := getDaprPath()

tests/e2e/kubernetes/kubernetes_test.go

@@ -306,3 +306,39 @@ func TestRenewCertWithPrivateKey(t *testing.T) {
 		t.Run(tc.Name, tc.Callable)
 	}
 }
+
+func TestRenewCertWithIncorrectFlags(t *testing.T) {
+	common.EnsureUninstall(true)
+
+	tests := []common.TestCase{}
+	var installOpts = common.TestOptions{
+		HAEnabled:             false,
+		MTLSEnabled:           true,
+		ApplyComponentChanges: true,
+		CheckResourceExists: map[common.Resource]bool{
+			common.CustomResourceDefs:  true,
+			common.ClusterRoles:        true,
+			common.ClusterRoleBindings: true,
+		},
+	}
+
+	tests = append(tests, common.GetTestsOnInstall(currentVersionDetails, installOpts)...)
+
+	// tests for certifcate renewal with incorrect set of flags provided.
+	tests = append(tests, []common.TestCase{
+		{"Renew certificate with incorrect flags", common.NegativeScenarioForCertRenew()},
+	}...)
+
+	// teardown everything
+	tests = append(tests, common.GetTestsOnUninstall(currentVersionDetails, common.TestOptions{
+		CheckResourceExists: map[common.Resource]bool{
+			common.CustomResourceDefs:  true,
+			common.ClusterRoles:        false,
+			common.ClusterRoleBindings: false,
+		},
+	})...)
+
+	for _, tc := range tests {
+		t.Run(tc.Name, tc.Callable)
+	}
+}