Merge pull request juju#16111 from wallyworld/merge-2.9-20230817

juju#16111 Merge 2.9, no conflicts juju#16110 [from wallyworld/azure-vault-sp](juju@de64baa)
jack-w-shaw · Aug 17, 2023 · 87b82f7 · 87b82f7
2 parents d48908d + cfd8739
commit 87b82f7
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 36 deletions.
diff --git a/provider/azure/credentials.go b/provider/azure/credentials.go
@@ -161,6 +161,19 @@ func (c environProviderCredentials) FinalizeCredential(
 	switch authType := args.Credential.AuthType(); authType {
 	case deviceCodeAuthType:
 		subscriptionId := args.Credential.Attributes()[credAttrSubscriptionId]
+
+		var azCloudName string
+		switch args.CloudName {
+		case "azure":
+			azCloudName = azureauth.AzureCloud
+		case "azure-china":
+			azCloudName = azureauth.AzureChinaCloud
+		case "azure-gov":
+			azCloudName = azureauth.AzureUSGovernment
+		default:
+			return nil, errors.Errorf("unknown Azure cloud name %q", args.CloudName)
+		}
+
 		if subscriptionId != "" {
 			opts := azcore.ClientOptions{
 				Cloud:     azureCloud(args.CloudName, args.CloudEndpoint, args.CloudIdentityEndpoint),
@@ -173,21 +186,12 @@ func (c environProviderCredentials) FinalizeCredential(
 				return nil, errors.Trace(err)
 			}
 			return c.deviceCodeCredential(ctx, args, azureauth.ServicePrincipalParams{
+				CloudName:      azCloudName,
 				SubscriptionId: subscriptionId,
 				TenantId:       tenantID,
 			})
 		}
-		var azCloudName string
-		switch args.CloudName {
-		case "azure":
-			azCloudName = "AzureCloud"
-		case "azure-china":
-			azCloudName = "AzureChinaCloud"
-		case "azure-gov":
-			azCloudName = "AzureUSGovernment"
-		default:
-			return nil, errors.Errorf("unknown Azure cloud name %q", args.CloudName)
-		}
+
 		params, err := c.getServicePrincipalParams(azCloudName)
 		if err != nil {
 			return nil, errors.Trace(err)
@@ -295,6 +299,7 @@ func (c environProviderCredentials) getServicePrincipalParams(cloudName string)
 		}
 	}
 	return azureauth.ServicePrincipalParams{
+		CloudName:      cloudName,
 		SubscriptionId: acc.ID,
 		TenantId:       acc.AuthTenantId(),
 	}, nil

diff --git a/provider/azure/credentials_test.go b/provider/azure/credentials_test.go
@@ -256,6 +256,7 @@ func (s *credentialsSuite) TestFinalizeCredentialInteractive(c *gc.C) {
 	in := cloud.NewCredential("interactive", map[string]string{"subscription-id": fakeSubscriptionId})
 	ctx := cmdtesting.Context(c)
 	out, err := s.provider.FinalizeCredential(ctx, environs.FinalizeCredentialParams{
+		CloudName:             "azure",
 		Credential:            in,
 		CloudEndpoint:         "https://arm.invalid",
 		CloudStorageEndpoint:  "https://core.invalid",
@@ -274,6 +275,7 @@ func (s *credentialsSuite) TestFinalizeCredentialInteractive(c *gc.C) {
 	s.servicePrincipalCreator.CheckCallNames(c, "InteractiveCreate")
 	args := s.servicePrincipalCreator.Calls()[0].Args
 	c.Assert(args[2], jc.DeepEquals, azureauth.ServicePrincipalParams{
+		CloudName:      "AzureCloud",
 		SubscriptionId: fakeSubscriptionId,
 		TenantId:       fakeTenantId,
 	})
@@ -285,6 +287,7 @@ func (s *credentialsSuite) TestFinalizeCredentialInteractiveError(c *gc.C) {
 	s.servicePrincipalCreator.SetErrors(errors.New("blargh"))
 	ctx := cmdtesting.Context(c)
 	_, err := s.provider.FinalizeCredential(ctx, environs.FinalizeCredentialParams{
+		CloudName:             "azure",
 		Credential:            in,
 		CloudEndpoint:         "https://arm.invalid",
 		CloudIdentityEndpoint: "https://graph.invalid",

diff --git a/provider/azure/internal/azureauth/serviceprincipal.go b/provider/azure/internal/azureauth/serviceprincipal.go
@@ -35,14 +35,33 @@ const (
 	// the application.
 	jujuApplicationId = "60a04dc9-1857-425f-8076-5ba81ca53d66"
 
-	// JujuApplicationObjectId is the ObjectId of the Azure application.
+	// jujuApplicationObjectId is the ObjectId of the Azure application.
 	jujuApplicationObjectId = "8b744cea-179d-4a73-9dff-20d52126030a"
 
+	// defaultAzureKeyVaultApplicationId is the default Azure Key Vault
+	// applicationID if not specified for a specific cloud type.
+	defaultAzureKeyVaultApplicationId = "cfa8b339-82a2-471a-a3c9-0fc0be7a4093"
+
 	// passwordExpiryDuration is how long the application password we
 	// set will remain valid.
 	passwordExpiryDuration = 365 * 24 * time.Hour
 )
 
+const (
+	// These consts represent the Azure cloud types.
+
+	AzureCloud        = "AzureCloud"
+	AzureChinaCloud   = "AzureChinaCloud"
+	AzureUSGovernment = "AzureUSGovernment"
+)
+
+// cloudVaultApps holds the IDs of the Azure Key Vault Application
+// for each cloud type.
+var cloudVaultApps = map[string]string{
+	AzureCloud:        defaultAzureKeyVaultApplicationId,
+	AzureUSGovernment: "7e7c393b-45d0-48b1-a35e-2905ddf8183c",
+}
+
 // MaybeJujuApplicationObjectID returns the Juju Application Object ID
 // if the passed in application ID is the Juju Enterprise App.
 func MaybeJujuApplicationObjectID(appID string) (string, error) {
@@ -54,6 +73,7 @@ func MaybeJujuApplicationObjectID(appID string) (string, error) {
 
 // ServicePrincipalParams are used when creating Juju service principal.
 type ServicePrincipalParams struct {
+	CloudName string
 	// Credential is the authorization needed to contact the
 	// Azure graph API.
 	Credential azcore.TokenCredential
@@ -119,7 +139,18 @@ func (c *ServicePrincipalCreator) Create(sdkCtx context.Context, params ServiceP
 	if err != nil {
 		return "", "", "", errors.Trace(err)
 	}
-	servicePrincipalObjectId, password, err := c.createOrUpdateServicePrincipal(sdkCtx, client)
+
+	// The user account must have a service principal for the Azure Key Vault application.
+	azureKeyVaultApplicationId, ok := cloudVaultApps[params.CloudName]
+	if !ok {
+		azureKeyVaultApplicationId = defaultAzureKeyVaultApplicationId
+	}
+	_, err = c.createOrUpdateServicePrincipal(sdkCtx, client, azureKeyVaultApplicationId, "Azure Key Vault application")
+	if err != nil {
+		return "", "", "", errors.Trace(err)
+	}
+
+	servicePrincipalObjectId, password, err := c.createOrUpdateJujuServicePrincipal(sdkCtx, client)
 	if err != nil {
 		return "", "", "", errors.Trace(err)
 	}
@@ -129,7 +160,7 @@ func (c *ServicePrincipalCreator) Create(sdkCtx context.Context, params ServiceP
 	return jujuApplicationId, servicePrincipalObjectId, password, nil
 }
 
-func (c *ServicePrincipalCreator) createOrUpdateServicePrincipal(sdkCtx context.Context, client *msgraphsdkgo.GraphServiceClient) (servicePrincipalObjectId, password string, _ error) {
+func (c *ServicePrincipalCreator) createOrUpdateJujuServicePrincipal(sdkCtx context.Context, client *msgraphsdkgo.GraphServiceClient) (servicePrincipalObjectId, password string, _ error) {
 	passwordCredential, err := c.preparePasswordCredential()
 	if err != nil {
 		return "", "", errors.Annotate(err, "preparing password credential")
@@ -155,24 +186,30 @@ func (c *ServicePrincipalCreator) createOrUpdateServicePrincipal(sdkCtx context.
 		return toValue(servicePrincipal.GetId()), toValue(addPassword.GetSecretText()), nil
 	}
 
-	// The service principal might already exist, so we need to query
-	// its object ID, and fetch the existing password credentials
-	// to update.
-	servicePrincipal, err := client.ServicePrincipalsWithAppId(to.Ptr(jujuApplicationId)).Get(sdkCtx, nil)
+	servicePrincipal, err := c.createOrUpdateServicePrincipal(sdkCtx, client, jujuApplicationId, "Juju Application")
+	if err != nil {
+		return "", "", errors.Trace(err)
+	}
+	id, password, err := addPassword(servicePrincipal)
+	if err != nil {
+		return "", "", errors.Annotate(err, "creating service principal password")
+	}
+	return id, password, nil
+}
+
+func (c *ServicePrincipalCreator) createOrUpdateServicePrincipal(sdkCtx context.Context, client *msgraphsdkgo.GraphServiceClient, appId, label string) (models.ServicePrincipalable, error) {
+	// The service principal might already exist, so we need to query its application ID.
+	servicePrincipal, err := client.ServicePrincipalsWithAppId(to.Ptr(appId)).Get(sdkCtx, nil)
 	if err == nil {
-		id, password, err := addPassword(servicePrincipal)
-		if err != nil {
-			return "", "", errors.Annotate(err, "creating service principal password")
-		}
-		return id, password, nil
+		return servicePrincipal, nil
 	}
 	if !isNotFound(err) {
-		return "", "", errors.Annotate(ReportableError(err), "looking for existing service principal")
+		return nil, errors.Annotatef(ReportableError(err), "looking for existing service principal for %s", label)
 	}
 
 	createServicePrincipal := func() error {
 		requestBody := models.NewServicePrincipal()
-		requestBody.SetAppId(to.Ptr(jujuApplicationId))
+		requestBody.SetAppId(to.Ptr(appId))
 		requestBody.SetAccountEnabled(to.Ptr(true))
 		servicePrincipal, err = client.ServicePrincipals().Post(sdkCtx, requestBody, nil)
 		return errors.Annotate(ReportableError(err), "creating service principal")
@@ -192,21 +229,14 @@ func (c *ServicePrincipalCreator) createOrUpdateServicePrincipal(sdkCtx context.
 	}
 	if err := retry.Call(retryArgs); err != nil {
 		if !isAlreadyExists(err) {
-			return "", "", errors.Trace(err)
+			return nil, errors.Trace(err)
 		}
-		// The service principal already exists, so we'll fall out
-		// and update the service principal's password credentials.
-		servicePrincipal, err = client.ServicePrincipalsWithAppId(to.Ptr(jujuApplicationId)).Get(sdkCtx, nil)
+		servicePrincipal, err = client.ServicePrincipalsWithAppId(to.Ptr(appId)).Get(sdkCtx, nil)
 		if err != nil {
-			return "", "", errors.Annotate(ReportableError(err), "looking for service principal")
+			return nil, errors.Annotatef(ReportableError(err), "looking for service principal for %s", label)
 		}
 	}
-
-	id, password, err := addPassword(servicePrincipal)
-	if err != nil {
-		return "", "", errors.Annotate(err, "creating service principal password")
-	}
-	return id, password, nil
+	return servicePrincipal, nil
 }
 
 func (c *ServicePrincipalCreator) preparePasswordCredential() (*models.PasswordCredential, error) {

diff --git a/provider/azure/internal/azureauth/serviceprincipal_test.go b/provider/azure/internal/azureauth/serviceprincipal_test.go
@@ -142,6 +142,10 @@ func (s *InteractiveSuite) TestInteractive(c *gc.C) {
 
 	mockAdaptor := &MockRequestAdaptor{NetHttpRequestAdapter: ra}
 	mockAdaptor.results = []requestResult{{
+		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
+		Params:      map[string]string{"appId": "cfa8b339-82a2-471a-a3c9-0fc0be7a4093"},
+		Result:      models.NewServicePrincipal(),
+	}, {
 		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
 		Params:      map[string]string{"appId": "60a04dc9-1857-425f-8076-5ba81ca53d66"},
 		Result:      sp,
@@ -167,6 +171,7 @@ func (s *InteractiveSuite) TestInteractive(c *gc.C) {
 	sdkCtx := context.Background()
 
 	appId, spObjectId, password, err := spc.InteractiveCreate(sdkCtx, &stderr, azureauth.ServicePrincipalParams{
+		CloudName:      "AzureCloud",
 		SubscriptionId: subscriptionId,
 		TenantId:       fakeTenantId,
 		Credential:     &azuretesting.FakeCredential{},
@@ -193,6 +198,10 @@ func (s *InteractiveSuite) TestInteractiveRoleAssignmentAlreadyExists(c *gc.C) {
 
 	mockAdaptor := &MockRequestAdaptor{NetHttpRequestAdapter: ra}
 	mockAdaptor.results = []requestResult{{
+		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
+		Params:      map[string]string{"appId": "cfa8b339-82a2-471a-a3c9-0fc0be7a4093"},
+		Result:      models.NewServicePrincipal(),
+	}, {
 		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
 		Params:      map[string]string{"appId": "60a04dc9-1857-425f-8076-5ba81ca53d66"},
 		Result:      sp,
@@ -218,6 +227,7 @@ func (s *InteractiveSuite) TestInteractiveRoleAssignmentAlreadyExists(c *gc.C) {
 	sdkCtx := context.Background()
 
 	appId, spObjectId, password, err := spc.InteractiveCreate(sdkCtx, &stderr, azureauth.ServicePrincipalParams{
+		CloudName:      "AzureCloud",
 		SubscriptionId: subscriptionId,
 		TenantId:       fakeTenantId,
 		Credential:     &azuretesting.FakeCredential{},
@@ -254,6 +264,10 @@ func (s *InteractiveSuite) TestInteractiveServicePrincipalNotFound(c *gc.C) {
 
 	mockAdaptor := &MockRequestAdaptor{NetHttpRequestAdapter: ra}
 	mockAdaptor.results = []requestResult{{
+		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
+		Params:      map[string]string{"appId": "cfa8b339-82a2-471a-a3c9-0fc0be7a4093"},
+		Result:      models.NewServicePrincipal(),
+	}, {
 		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
 		Params:      map[string]string{"appId": "60a04dc9-1857-425f-8076-5ba81ca53d66"},
 		Err:         dataError("Request_ResourceNotFound"),
@@ -282,6 +296,7 @@ func (s *InteractiveSuite) TestInteractiveServicePrincipalNotFound(c *gc.C) {
 	sdkCtx := context.Background()
 
 	appId, spObjectId, password, err := spc.InteractiveCreate(sdkCtx, &stderr, azureauth.ServicePrincipalParams{
+		CloudName:      "AzureCloud",
 		SubscriptionId: subscriptionId,
 		TenantId:       fakeTenantId,
 		Credential:     &azuretesting.FakeCredential{},
@@ -304,6 +319,10 @@ func (s *InteractiveSuite) TestInteractiveServicePrincipalNotFoundRace(c *gc.C)
 
 	mockAdaptor := &MockRequestAdaptor{NetHttpRequestAdapter: ra}
 	mockAdaptor.results = []requestResult{{
+		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
+		Params:      map[string]string{"appId": "cfa8b339-82a2-471a-a3c9-0fc0be7a4093"},
+		Result:      models.NewServicePrincipal(),
+	}, {
 		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
 		Params:      map[string]string{"appId": "60a04dc9-1857-425f-8076-5ba81ca53d66"},
 		Err:         dataError("Request_ResourceNotFound"),
@@ -335,6 +354,7 @@ func (s *InteractiveSuite) TestInteractiveServicePrincipalNotFoundRace(c *gc.C)
 	sdkCtx := context.Background()
 
 	appId, spObjectId, password, err := spc.InteractiveCreate(sdkCtx, &stderr, azureauth.ServicePrincipalParams{
+		CloudName:      "AzureCloud",
 		SubscriptionId: subscriptionId,
 		TenantId:       fakeTenantId, Credential: &azuretesting.FakeCredential{},
 	})
@@ -356,6 +376,10 @@ func (s *InteractiveSuite) TestInteractiveRetriesRoleAssignment(c *gc.C) {
 
 	mockAdaptor := &MockRequestAdaptor{NetHttpRequestAdapter: ra}
 	mockAdaptor.results = []requestResult{{
+		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
+		Params:      map[string]string{"appId": "cfa8b339-82a2-471a-a3c9-0fc0be7a4093"},
+		Result:      models.NewServicePrincipal(),
+	}, {
 		PathPattern: regexp.QuoteMeta("{+baseurl}/servicePrincipals(appId='{appId}')") + ".*",
 		Params:      map[string]string{"appId": "60a04dc9-1857-425f-8076-5ba81ca53d66"},
 		Result:      sp,
@@ -381,6 +405,7 @@ func (s *InteractiveSuite) TestInteractiveRetriesRoleAssignment(c *gc.C) {
 	subscriptionId := "22222222-2222-2222-2222-222222222222"
 	sdkCtx := context.Background()
 	appId, spObjectId, password, err := spc.InteractiveCreate(sdkCtx, &stderr, azureauth.ServicePrincipalParams{
+		CloudName:      "AzureCloud",
 		SubscriptionId: subscriptionId,
 		TenantId:       fakeTenantId, Credential: &azuretesting.FakeCredential{},
 	})