Add ServicePrincipalProfile.KeyvaultSecretRef for KeyVault secret ref… (#1022)

* Add ServicePrincipalProfile.KeyvaultSecretRef for KeyVault secret reference
- with this change, ServicePrincipalProfile.secret will be added to template parameters as is without being tranformed into Keyvault secret reference.
- to use keyvault secret reference, use ServicePrincipalProfile.KeyvaultSecretRef instead

* refactored to address comments

* fixing the tests

examples/keyvault-params/README.md

@@ -11,7 +11,7 @@ ACS-Engine enables you to retrieve the following k8s deployment parameters from
 *	clientPrivateKey
 *	kubeConfigCertificate
 *	kubeConfigPrivateKey
-*	servicePrincipalClientSecret
+*	servicePrincipalClientKeyvaultSecretRef
 
 The parameters above could still be set as plain text.
 

examples/keyvault-params/kubernetes.json

@@ -35,7 +35,7 @@
     },
     "servicePrincipalProfile": {
       "servicePrincipalClientID": "ServicePrincipalClientID",
-      "servicePrincipalClientSecret": "/subscriptions/my-sub/resourceGroups/my-rg/providers/Microsoft.KeyVault/vaults/my-kv/secrets/my-secret2"
+      "servicePrincipalClientKeyvaultSecretRef": "/subscriptions/my-sub/resourceGroups/my-rg/providers/Microsoft.KeyVault/vaults/my-kv/secrets/my-secret2"
     },
     "certificateProfile": {
       "caCertificate": "<caCertificate>",

pkg/acsengine/engine.go

@@ -423,7 +423,12 @@ func getParameters(cs *api.ContainerService, isClassicMode bool) (map[string]int
 		if properties.OrchestratorProfile.KubernetesConfig != nil &&
 			!properties.OrchestratorProfile.KubernetesConfig.UseManagedIdentity {
 			addValue(parametersMap, "servicePrincipalClientId", properties.ServicePrincipalProfile.ClientID)
-			addSecret(parametersMap, "servicePrincipalClientSecret", properties.ServicePrincipalProfile.Secret, false)
+
+			if properties.ServicePrincipalProfile.KeyvaultSecretRef != "" {
+				addSecret(parametersMap, "servicePrincipalClientSecret", properties.ServicePrincipalProfile.KeyvaultSecretRef, false)
+			} else {
+				addValue(parametersMap, "servicePrincipalClientSecret", properties.ServicePrincipalProfile.Secret)
+			}
 		}
 	}
 

pkg/acsengine/testdata/key-vault-params/kubernetes.json

@@ -40,7 +40,7 @@
     },
     "servicePrincipalProfile": {
       "servicePrincipalClientID": "ServicePrincipalClientID",
-      "servicePrincipalClientSecret": "/subscriptions/my-sub/resourceGroups/my-rg/providers/Microsoft.KeyVault/vaults/my-kv/secrets/my-secret2"
+      "servicePrincipalClientKeyvaultSecretRef": "/subscriptions/my-sub/resourceGroups/my-rg/providers/Microsoft.KeyVault/vaults/my-kv/secrets/my-secret2"
     },
     "certificateProfile": {
       "caCertificate": "caCertificate",

pkg/acsengine/testdata/v20170701/kubernetes-keyvault-secret.json

@@ -0,0 +1,49 @@
+{
+  "apiVersion": "2017-07-01",
+  "properties": {
+    "orchestratorProfile": {
+      "orchestratorType": "Kubernetes",
+      "orchestratorVersion": "1.6.6"
+    },
+    "masterProfile": {
+      "count": 1,
+      "dnsPrefix": "masterdns1",
+      "OSDiskSizeGB": 200,
+      "vmSize": "Standard_D2_v2"
+    },
+    "agentPoolProfiles": [
+      {
+        "name": "agentpool1",
+        "count": 1,
+        "vmSize": "Standard_D2_v2",
+        "OSDiskSizeGB": 200,
+        "availabilityProfile": "AvailabilitySet"
+      },
+      {
+        "name": "agentpool2",
+        "count": 1,
+        "vmSize": "Standard_D3_v2",
+        "availabilityProfile": "AvailabilitySet",
+        "osType": "Windows"
+      }
+    ],
+    "windowsProfile": {
+      "adminUsername": "azureuser",
+      "adminPassword": "replacepassword1234$"
+    },
+    "linuxProfile": {
+      "adminUsername": "azureuser",
+      "ssh": {
+        "publicKeys": [
+          {
+            "keyData": "ssh-rsa PUBLICKEY azureuser@linuxvm"
+          }
+        ]
+      }
+    },
+    "servicePrincipalProfile": {
+      "clientId": "ServicePrincipalClientID",
+      "keyvaultSecretRef": "/subscriptions/my-sub/resourceGroups/my-rg/providers/Microsoft.KeyVault/vaults/my-kv/secrets/my-secret2"
+    }
+  }
+}

pkg/api/converterfromapi.go

@@ -766,11 +766,13 @@ func convertCustomProfileToV20170701(api *CustomProfile, v20170701 *v20170701.Cu
 func convertServicePrincipalProfileToV20170701(api *ServicePrincipalProfile, v20170701 *v20170701.ServicePrincipalProfile) {
 	v20170701.ClientID = api.ClientID
 	v20170701.Secret = api.Secret
+	v20170701.KeyvaultSecretRef = api.KeyvaultSecretRef
 }
 
 func convertServicePrincipalProfileToVLabs(api *ServicePrincipalProfile, vlabs *vlabs.ServicePrincipalProfile) {
 	vlabs.ClientID = api.ClientID
 	vlabs.Secret = api.Secret
+	vlabs.KeyvaultSecretRef = api.KeyvaultSecretRef
 }
 
 func convertCertificateProfileToVLabs(api *CertificateProfile, vlabs *vlabs.CertificateProfile) {

pkg/api/convertertoapi.go

@@ -807,11 +807,13 @@ func convertV20170131ServicePrincipalProfile(v20170131 *v20170131.ServicePrincip
 func convertV20170701ServicePrincipalProfile(v20170701 *v20170701.ServicePrincipalProfile, api *ServicePrincipalProfile) {
 	api.ClientID = v20170701.ClientID
 	api.Secret = v20170701.Secret
+	api.KeyvaultSecretRef = v20170701.KeyvaultSecretRef
 }
 
 func convertVLabsServicePrincipalProfile(vlabs *vlabs.ServicePrincipalProfile, api *ServicePrincipalProfile) {
 	api.ClientID = vlabs.ClientID
 	api.Secret = vlabs.Secret
+	api.KeyvaultSecretRef = vlabs.KeyvaultSecretRef
 }
 
 func convertV20160930CustomProfile(v20160930 *v20160930.CustomProfile, api *CustomProfile) {

pkg/api/types.go

@@ -55,8 +55,9 @@ type Properties struct {
 
 // ServicePrincipalProfile contains the client and secret used by the cluster for Azure Resource CRUD
 type ServicePrincipalProfile struct {
-	ClientID string `json:"servicePrincipalClientID,omitempty"`
-	Secret   string `json:"servicePrincipalClientSecret,omitempty"`
+	ClientID          string `json:"servicePrincipalClientID,omitempty"`
+	Secret            string `json:"servicePrincipalClientSecret,omitempty"`
+	KeyvaultSecretRef string `json:"keyvaultSecretRef,omitempty"`
 }
 
 // CertificateProfile represents the definition of the master cluster

pkg/api/v20170701/types.go

@@ -44,8 +44,9 @@ type Properties struct {
 }
 
 // ServicePrincipalProfile contains the client and secret used by the cluster for Azure Resource CRUD
-// The 'Secret' parameter could be either a plain text, or referenced to a secret in a keyvault.
-// In the latter case, the format of the parameter's value should be
+// The 'Secret' parameter should be a secret in plain text.
+// The 'KeyvaultSecretRef' parameter is a reference to a secret in a keyvault.
+// The format of the parameter's value should be
 // "/subscriptions/<SUB_ID>/resourceGroups/<RG_NAME>/providers/Microsoft.KeyVault/vaults/<KV_NAME>/secrets/<NAME>[/<VERSION>]"
 // where:
 //    <SUB_ID> is the subscription ID of the keyvault
@@ -54,8 +55,9 @@ type Properties struct {
 //    <NAME> is the name of the secret.
 //    <VERSION> (optional) is the version of the secret (default: the latest version)
 type ServicePrincipalProfile struct {
-	ClientID string `json:"clientId,omitempty" validate:"required"`
-	Secret   string `json:"secret,omitempty" validate:"required"`
+	ClientID          string `json:"clientId,omitempty" validate:"required"`
+	Secret            string `json:"secret,omitempty"`
+	KeyvaultSecretRef string `json:"keyvaultSecretRef,omitempty"`
 }
 
 // CustomProfile specifies custom properties that are used for

pkg/api/v20170701/validate.go

@@ -10,10 +10,14 @@ import (
 	validator "gopkg.in/go-playground/validator.v9"
 )
 
-var validate *validator.Validate
+var (
+	validate                *validator.Validate
+	keyvaultSecretPathRegex *regexp.Regexp
+)
 
 func init() {
 	validate = validator.New()
+	keyvaultSecretPathRegex = regexp.MustCompile(`^(/subscriptions/\S+/resourceGroups/\S+/providers/Microsoft.KeyVault/vaults/\S+)/secrets/([^/\s]+)(/(\S+))?$`)
 }
 
 // Validate implements APIObject
@@ -157,9 +161,18 @@ func (a *Properties) Validate() error {
 	if e := validateUniqueProfileNames(a.AgentPoolProfiles); e != nil {
 		return e
 	}
+
 	if a.OrchestratorProfile.OrchestratorType == Kubernetes {
-		if a.ServicePrincipalProfile == nil {
-			return fmt.Errorf("missing ServicePrincipalProfile")
+		if (len(a.ServicePrincipalProfile.Secret) == 0 && len(a.ServicePrincipalProfile.KeyvaultSecretRef) == 0) ||
+			(len(a.ServicePrincipalProfile.Secret) != 0 && len(a.ServicePrincipalProfile.KeyvaultSecretRef) != 0) {
+			return fmt.Errorf("either the service principal client secret or keyvault secret reference must be specified with Orchestrator %s", a.OrchestratorProfile.OrchestratorType)
+		}
+
+		if len(a.ServicePrincipalProfile.KeyvaultSecretRef) != 0 {
+			parts := keyvaultSecretPathRegex.FindStringSubmatch(a.ServicePrincipalProfile.KeyvaultSecretRef)
+			if len(parts) != 5 {
+				return fmt.Errorf("service principal client keyvault secret reference is of incorrect format")
+			}
 		}
 	}
 

pkg/api/v20170701/validate_test.go

@@ -0,0 +1,87 @@
+package v20170701
+
+import "testing"
+
+func Test_ServicePrincipalProfile_ValidateSecretOrKeyvaultSecretRef(t *testing.T) {
+
+	t.Run("ServicePrincipalProfile with secret should pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+
+		if err := p.Validate(); err != nil {
+			t.Errorf("should not error %v", err)
+		}
+	})
+
+	t.Run("ServicePrincipalProfile with KeyvaultSecretRef (with version) should pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+		p.ServicePrincipalProfile.Secret = ""
+		p.ServicePrincipalProfile.KeyvaultSecretRef = "/subscriptions/SUB-ID/resourceGroups/RG-NAME/providers/Microsoft.KeyVault/vaults/KV-NAME/secrets/secret-name/version"
+
+		if err := p.Validate(); err != nil {
+			t.Errorf("should not error %v", err)
+		}
+	})
+
+	t.Run("ServicePrincipalProfile with KeyvaultSecretRef (without version) should pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+		p.ServicePrincipalProfile.Secret = ""
+		p.ServicePrincipalProfile.KeyvaultSecretRef = "/subscriptions/SUB-ID/resourceGroups/RG-NAME/providers/Microsoft.KeyVault/vaults/KV-NAME/secrets/secret-name>"
+
+		if err := p.Validate(); err != nil {
+			t.Errorf("should not error %v", err)
+		}
+	})
+
+	t.Run("ServicePrincipalProfile with Secret and KeyvaultSecretRef should NOT pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+		p.ServicePrincipalProfile.KeyvaultSecretRef = "/subscriptions/SUB-ID/resourceGroups/RG-NAME/providers/Microsoft.KeyVault/vaults/KV-NAME/secrets/secret-name/version"
+
+		if err := p.Validate(); err == nil {
+			t.Error("error should have occurred")
+		}
+	})
+
+	t.Run("ServicePrincipalProfile with incorrect KeyvaultSecretRef format should NOT pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+		p.ServicePrincipalProfile.Secret = ""
+		p.ServicePrincipalProfile.KeyvaultSecretRef = "randomsecret"
+
+		if err := p.Validate(); err == nil || err.Error() != "service principal client keyvault secret reference is of incorrect format" {
+			t.Error("error should have occurred")
+		}
+	})
+}
+
+func getK8sDefaultProperties() *Properties {
+	return &Properties{
+		OrchestratorProfile: &OrchestratorProfile{
+			OrchestratorType: Kubernetes,
+		},
+		MasterProfile: &MasterProfile{
+			Count:     1,
+			DNSPrefix: "foo",
+			VMSize:    "Standard_DS2_v2",
+		},
+		AgentPoolProfiles: []*AgentPoolProfile{
+			&AgentPoolProfile{
+				Name:   "agentpool",
+				VMSize: "Standard_D2_v2",
+				Count:  1,
+			},
+		},
+		LinuxProfile: &LinuxProfile{
+			AdminUsername: "azureuser",
+			SSH: struct {
+				PublicKeys []PublicKey `json:"publicKeys" validate:"required,len=1"`
+			}{
+				PublicKeys: []PublicKey{{
+					KeyData: "publickeydata",
+				}},
+			},
+		},
+		ServicePrincipalProfile: &ServicePrincipalProfile{
+			ClientID: "clientID",
+			Secret:   "clientSecret",
+		},
+	}
+}

pkg/api/vlabs/types.go

@@ -40,8 +40,9 @@ type Properties struct {
 }
 
 // ServicePrincipalProfile contains the client and secret used by the cluster for Azure Resource CRUD
-// The 'Secret' parameter could be either a plain text, or referenced to a secret in a keyvault.
-// In the latter case, the format of the parameter's value should be
+// The 'Secret' parameter should be a secret in plain text.
+// The 'KeyvaultSecretRef' parameter is a reference to a secret in a keyvault.
+// The format of the parameter's value should be
 // "/subscriptions/<SUB_ID>/resourceGroups/<RG_NAME>/providers/Microsoft.KeyVault/vaults/<KV_NAME>/secrets/<NAME>[/<VERSION>]"
 // where:
 //    <SUB_ID> is the subscription ID of the keyvault
@@ -50,8 +51,9 @@ type Properties struct {
 //    <NAME> is the name of the secret.
 //    <VERSION> (optional) is the version of the secret (default: the latest version)
 type ServicePrincipalProfile struct {
-	ClientID string `json:"servicePrincipalClientID,omitempty"`
-	Secret   string `json:"servicePrincipalClientSecret,omitempty"`
+	ClientID          string `json:"servicePrincipalClientID,omitempty"`
+	Secret            string `json:"servicePrincipalClientSecret,omitempty"`
+	KeyvaultSecretRef string `json:"servicePrincipalClientKeyvaultSecretRef,omitempty"`
 }
 
 // CertificateProfile represents the definition of the master cluster

pkg/api/vlabs/validate.go

@@ -9,6 +9,12 @@ import (
 	"time"
 )
 
+var keyvaultSecretPathRegex *regexp.Regexp
+
+func init() {
+	keyvaultSecretPathRegex = regexp.MustCompile(`^(/subscriptions/\S+/resourceGroups/\S+/providers/Microsoft.KeyVault/vaults/\S+)/secrets/([^/\s]+)(/(\S+))?$`)
+}
+
 // Validate implements APIObject
 func (o *OrchestratorProfile) Validate() error {
 	switch o.OrchestratorType {
@@ -229,8 +235,22 @@ func (a *Properties) Validate() error {
 		useManagedIdentity := (a.OrchestratorProfile.KubernetesConfig != nil &&
 			a.OrchestratorProfile.KubernetesConfig.UseManagedIdentity)
 
-		if !useManagedIdentity && (len(a.ServicePrincipalProfile.ClientID) == 0 || len(a.ServicePrincipalProfile.Secret) == 0) {
-			return fmt.Errorf("the service principal clientId and clientSecret must be specified with Orchestrator %s", a.OrchestratorProfile.OrchestratorType)
+		if !useManagedIdentity {
+			if len(a.ServicePrincipalProfile.ClientID) == 0 {
+				return fmt.Errorf("the service principal client ID must be specified with Orchestrator %s", a.OrchestratorProfile.OrchestratorType)
+			}
+
+			if (len(a.ServicePrincipalProfile.Secret) == 0 && len(a.ServicePrincipalProfile.KeyvaultSecretRef) == 0) ||
+				(len(a.ServicePrincipalProfile.Secret) != 0 && len(a.ServicePrincipalProfile.KeyvaultSecretRef) != 0) {
+				return fmt.Errorf("either the service principal client secret or keyvault secret reference must be specified with Orchestrator %s", a.OrchestratorProfile.OrchestratorType)
+			}
+
+			if len(a.ServicePrincipalProfile.KeyvaultSecretRef) != 0 {
+				parts := keyvaultSecretPathRegex.FindStringSubmatch(a.ServicePrincipalProfile.KeyvaultSecretRef)
+				if len(parts) != 5 {
+					return fmt.Errorf("service principal client keyvault secret reference is of incorrect format")
+				}
+			}
 		}
 	}
 

pkg/api/vlabs/validate_test.go

@@ -1,8 +1,6 @@
 package vlabs
 
-import (
-	"testing"
-)
+import "testing"
 
 const (
 	ValidKubernetesNodeStatusUpdateFrequency        = "10s"
@@ -172,3 +170,88 @@ func Test_Properties_ValidateNetworkPolicy(t *testing.T) {
 		)
 	}
 }
+
+func Test_ServicePrincipalProfile_ValidateSecretOrKeyvaultSecretRef(t *testing.T) {
+
+	t.Run("ServicePrincipalProfile with secret should pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+
+		if err := p.Validate(); err != nil {
+			t.Errorf("should not error %v", err)
+		}
+	})
+
+	t.Run("ServicePrincipalProfile with KeyvaultSecretRef (with version) should pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+		p.ServicePrincipalProfile.Secret = ""
+		p.ServicePrincipalProfile.KeyvaultSecretRef = "/subscriptions/SUB-ID/resourceGroups/RG-NAME/providers/Microsoft.KeyVault/vaults/KV-NAME/secrets/secret-name/version"
+
+		if err := p.Validate(); err != nil {
+			t.Errorf("should not error %v", err)
+		}
+	})
+
+	t.Run("ServicePrincipalProfile with KeyvaultSecretRef (without version) should pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+		p.ServicePrincipalProfile.Secret = ""
+		p.ServicePrincipalProfile.KeyvaultSecretRef = "/subscriptions/SUB-ID/resourceGroups/RG-NAME/providers/Microsoft.KeyVault/vaults/KV-NAME/secrets/secret-name>"
+
+		if err := p.Validate(); err != nil {
+			t.Errorf("should not error %v", err)
+		}
+	})
+
+	t.Run("ServicePrincipalProfile with Secret and KeyvaultSecretRef should NOT pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+		p.ServicePrincipalProfile.KeyvaultSecretRef = "/subscriptions/SUB-ID/resourceGroups/RG-NAME/providers/Microsoft.KeyVault/vaults/KV-NAME/secrets/secret-name/version"
+
+		if err := p.Validate(); err == nil {
+			t.Error("error should have occurred")
+		}
+	})
+
+	t.Run("ServicePrincipalProfile with incorrect KeyvaultSecretRef format should NOT pass", func(t *testing.T) {
+		p := getK8sDefaultProperties()
+		p.ServicePrincipalProfile.Secret = ""
+		p.ServicePrincipalProfile.KeyvaultSecretRef = "randomsecret"
+
+		if err := p.Validate(); err == nil || err.Error() != "service principal client keyvault secret reference is of incorrect format" {
+			t.Error("error should have occurred")
+		}
+	})
+}
+
+func getK8sDefaultProperties() *Properties {
+	return &Properties{
+		OrchestratorProfile: &OrchestratorProfile{
+			OrchestratorType: Kubernetes,
+		},
+		MasterProfile: &MasterProfile{
+			Count:     1,
+			DNSPrefix: "foo",
+			VMSize:    "Standard_DS2_v2",
+		},
+		AgentPoolProfiles: []*AgentPoolProfile{
+			&AgentPoolProfile{
+				Name:                "agentpool",
+				VMSize:              "Standard_D2_v2",
+				Count:               1,
+				AvailabilityProfile: AvailabilitySet,
+			},
+		},
+		LinuxProfile: &LinuxProfile{
+			AdminUsername: "azureuser",
+			SSH: struct {
+				PublicKeys []PublicKey `json:"publicKeys"`
+			}{
+				PublicKeys: []PublicKey{{
+					KeyData: "publickeydata",
+				}},
+			},
+		},
+		ServicePrincipalProfile: &ServicePrincipalProfile{
+			ClientID: "clientID",
+			Secret:   "clientSecret",
+		},
+	}
+}

test/common.sh

@@ -47,10 +47,10 @@ function generate_template() {
 	    apiVersion=$(get_api_version)
 		if [[ "$apiVersion" == "vlabs" ]]; then
 			jqi "${FINAL_CLUSTER_DEFINITION}" ".properties.servicePrincipalProfile.servicePrincipalClientID = \"${CLUSTER_SERVICE_PRINCIPAL_CLIENT_ID}\""
-			jqi "${FINAL_CLUSTER_DEFINITION}" ".properties.servicePrincipalProfile.servicePrincipalClientSecret = \"${CLUSTER_SERVICE_PRINCIPAL_CLIENT_SECRET}\""
+			jqi "${FINAL_CLUSTER_DEFINITION}" ".properties.servicePrincipalProfile.servicePrincipalClientKeyvaultSecretRef  = \"${CLUSTER_SERVICE_PRINCIPAL_CLIENT_SECRET}\""
 		else
 			jqi "${FINAL_CLUSTER_DEFINITION}" ".properties.servicePrincipalProfile.clientId = \"${CLUSTER_SERVICE_PRINCIPAL_CLIENT_ID}\""
-			jqi "${FINAL_CLUSTER_DEFINITION}" ".properties.servicePrincipalProfile.secret = \"${CLUSTER_SERVICE_PRINCIPAL_CLIENT_SECRET}\""
+			jqi "${FINAL_CLUSTER_DEFINITION}" ".properties.servicePrincipalProfile.keyvaultSecretRef = \"${CLUSTER_SERVICE_PRINCIPAL_CLIENT_SECRET}\""
 		fi
 	fi