From feeae1514ee1488f45e60f8724a99446acb063dc Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Fri, 31 May 2019 22:37:43 +0000 Subject: [PATCH 1/3] Refactor client config --- builder/azure/arm/builder.go | 35 ++-- builder/azure/arm/config.go | 14 +- builder/azure/arm/config_retriever.go | 75 --------- builder/azure/arm/config_retriever_test.go | 62 -------- builder/azure/arm/config_test.go | 16 +- builder/azure/arm/template_factory.go | 6 +- builder/azure/arm/template_factory_test.go | 8 +- .../client/config.go} | 138 ++++++++++++---- .../azure/common/client/config_retriever.go | 37 +++++ .../common/client/config_retriever_test.go | 56 +++++++ .../client/config_test.go} | 149 +++++++++--------- .../client/tokenprovider.go} | 2 +- .../client/tokenprovider_cert.go} | 4 +- .../client/tokenprovider_devicewflow.go} | 2 +- .../client/tokenprovider_jwt.go} | 2 +- .../client/tokenprovider_msi.go} | 2 +- .../client/tokenprovider_secret.go} | 2 +- .../client/tokenprovider_secret_test.go} | 2 +- .../azure/{arm => common}/template_funcs.go | 5 +- .../{arm => common}/template_funcs_test.go | 2 +- .../source/docs/builders/azure.html.md.erb | 4 +- .../arm/_ClientConfig-not-required.html.md | 20 --- .../builder/azure/arm/_ClientConfig.html.md | 2 - .../client/_Config-not-required.html.md | 25 +++ .../azure/common/client/_Config.html.md | 9 ++ 25 files changed, 357 insertions(+), 322 deletions(-) delete mode 100644 builder/azure/arm/config_retriever.go delete mode 100644 builder/azure/arm/config_retriever_test.go rename builder/azure/{arm/clientconfig.go => common/client/config.go} (59%) create mode 100644 builder/azure/common/client/config_retriever.go create mode 100644 builder/azure/common/client/config_retriever_test.go rename builder/azure/{arm/clientconfig_test.go => common/client/config_test.go} (78%) rename builder/azure/{arm/authenticate.go => common/client/tokenprovider.go} (94%) rename builder/azure/{arm/authenticate_cert.go => common/client/tokenprovider_cert.go} (98%) rename builder/azure/{arm/authenticate_devicewflow.go => common/client/tokenprovider_devicewflow.go} (98%) rename builder/azure/{arm/authenticate_jwt.go => common/client/tokenprovider_jwt.go} (98%) rename builder/azure/{arm/authenticate_msi.go => common/client/tokenprovider_msi.go} (97%) rename builder/azure/{arm/authenticate_secret.go => common/client/tokenprovider_secret.go} (98%) rename builder/azure/{arm/authenticate_secret_test.go => common/client/tokenprovider_secret_test.go} (98%) rename builder/azure/{arm => common}/template_funcs.go (92%) rename builder/azure/{arm => common}/template_funcs_test.go (98%) delete mode 100644 website/source/partials/builder/azure/arm/_ClientConfig-not-required.html.md delete mode 100644 website/source/partials/builder/azure/arm/_ClientConfig.html.md create mode 100644 website/source/partials/builder/azure/common/client/_Config-not-required.html.md create mode 100644 website/source/partials/builder/azure/common/client/_Config.html.md diff --git a/builder/azure/arm/builder.go b/builder/azure/arm/builder.go index 2a0c37d39..6fbb611d1 100644 --- a/builder/azure/arm/builder.go +++ b/builder/azure/arm/builder.go @@ -57,13 +57,10 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack ctx, cancel := context.WithCancel(ctx) defer cancel() - // User's intent to use MSI is indicated with empty subscription id, tenant, client id, client cert, client secret and jwt. - // FillParameters function will set subscription and tenant id here. Therefore getServicePrincipalTokens won't select right auth type. - // If we run this after getServicePrincipalTokens call then getServicePrincipalTokens won't have tenant id. - if !b.config.useMSI() { - if err := newConfigRetriever().FillParameters(b.config); err != nil { - return nil, err - } + // FillParameters function will set the authType from supplied parameters set the subscription and tenant id. + err := b.config.ClientConfig.FillParameters() + if err != nil { + return nil, err } log.Print(":: Configuration") @@ -77,19 +74,12 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack return nil, err } - // We need subscription id and tenant id for arm operations. Users hasn't specified one so we try to detect them here. - if b.config.useMSI() { - if err := newConfigRetriever().FillParameters(b.config); err != nil { - return nil, err - } - } - ui.Message("Creating Azure Resource Manager (ARM) client ...") azureClient, err := NewAzureClient( - b.config.SubscriptionID, + b.config.ClientConfig.SubscriptionID, b.config.ResourceGroupName, b.config.StorageAccount, - b.config.cloudEnvironment, + b.config.ClientConfig.CloudEnvironment, b.config.SharedGalleryTimeout, spnCloud, spnKeyVault) @@ -102,13 +92,13 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack if err := resolver.Resolve(b.config); err != nil { return nil, err } - if b.config.ObjectID == "" { - b.config.ObjectID = getObjectIdFromToken(ui, spnCloud) + if b.config.ClientConfig.ObjectID == "" { + b.config.ClientConfig.ObjectID = getObjectIdFromToken(ui, spnCloud) } else { ui.Message("You have provided Object_ID which is no longer needed, azure packer builder determines this dynamically from the authentication token") } - if b.config.ObjectID == "" && b.config.OSType != constants.Target_Linux { + if b.config.ClientConfig.ObjectID == "" && b.config.OSType != constants.Target_Linux { return nil, fmt.Errorf("could not determine the ObjectID for the user, which is required for Windows builds") } @@ -302,7 +292,8 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack } if b.config.isManagedImage() { - managedImageID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/images/%s", b.config.SubscriptionID, b.config.ManagedImageResourceGroupName, b.config.ManagedImageName) + managedImageID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/images/%s", + b.config.ClientConfig.SubscriptionID, b.config.ManagedImageResourceGroupName, b.config.ManagedImageName) if b.config.SharedGalleryDestination.SigDestinationGalleryName != "" { return NewManagedImageArtifactWithSIGAsDestination(b.config.OSType, b.config.ManagedImageResourceGroupName, b.config.ManagedImageName, b.config.manageImageLocation, managedImageID, b.config.ManagedImageOSDiskSnapshotName, b.config.ManagedImageDataDiskSnapshotPrefix, b.stateBag.Get(constants.ArmManagedImageSharedGalleryId).(string)) } @@ -405,7 +396,7 @@ func (b *Builder) configureStateBag(stateBag multistep.StateBag) { stateBag.Put(constants.ArmManagedImageSharedGalleryName, b.config.SharedGalleryDestination.SigDestinationGalleryName) stateBag.Put(constants.ArmManagedImageSharedGalleryImageName, b.config.SharedGalleryDestination.SigDestinationImageName) stateBag.Put(constants.ArmManagedImageSharedGalleryImageVersion, b.config.SharedGalleryDestination.SigDestinationImageVersion) - stateBag.Put(constants.ArmManagedImageSubscription, b.config.SubscriptionID) + stateBag.Put(constants.ArmManagedImageSubscription, b.config.ClientConfig.SubscriptionID) } } @@ -424,7 +415,7 @@ func (b *Builder) setImageParameters(stateBag multistep.StateBag) { } func (b *Builder) getServicePrincipalTokens(say func(string)) (*adal.ServicePrincipalToken, *adal.ServicePrincipalToken, error) { - return b.config.ClientConfig.getServicePrincipalTokens(say) + return b.config.ClientConfig.GetServicePrincipalTokens(say) } func getObjectIdFromToken(ui packer.Ui, token *adal.ServicePrincipalToken) string { diff --git a/builder/azure/arm/config.go b/builder/azure/arm/config.go index 4d8cef9a2..7243bd2a8 100644 --- a/builder/azure/arm/config.go +++ b/builder/azure/arm/config.go @@ -20,6 +20,8 @@ import ( "github.com/Azure/go-autorest/autorest/to" "github.com/masterzen/winrm" + azcommon "github.com/hashicorp/packer/builder/azure/common" + "github.com/hashicorp/packer/builder/azure/common/client" "github.com/hashicorp/packer/builder/azure/common/constants" "github.com/hashicorp/packer/builder/azure/pkcs12" "github.com/hashicorp/packer/common" @@ -92,7 +94,7 @@ type Config struct { common.PackerConfig `mapstructure:",squash"` // Authentication via OAUTH - ClientConfig `mapstructure:",squash"` + ClientConfig client.Config `mapstructure:",squash"` // Capture CaptureNamePrefix string `mapstructure:"capture_name_prefix"` @@ -385,7 +387,7 @@ func (c *Config) toVMID() string { } else { resourceGroupName = c.BuildResourceGroupName } - return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/virtualMachines/%s", c.SubscriptionID, resourceGroupName, c.tmpComputeName) + return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/virtualMachines/%s", c.ClientConfig.SubscriptionID, resourceGroupName, c.tmpComputeName) } func (c *Config) isManagedImage() bool { @@ -477,7 +479,7 @@ func (c *Config) createCertificate() (string, error) { func newConfig(raws ...interface{}) (*Config, []string, error) { var c Config - c.ctx.Funcs = TemplateFuncs + c.ctx.Funcs = azcommon.TemplateFuncs err := config.Decode(&c, &config.DecodeOpts{ Interpolate: true, InterpolateContext: &c.ctx, @@ -490,7 +492,7 @@ func newConfig(raws ...interface{}) (*Config, []string, error) { provideDefaultValues(&c) setRuntimeValues(&c) setUserNamePassword(&c) - err = c.ClientConfig.setCloudEnvironment() + err = c.ClientConfig.SetDefaultValues() if err != nil { return nil, nil, err } @@ -650,7 +652,7 @@ func provideDefaultValues(c *Config) { c.ImageVersion = DefaultImageVersion } - c.provideDefaultValues() + c.ClientConfig.SetDefaultValues() } func assertTagProperties(c *Config, errs *packer.MultiError) { @@ -669,7 +671,7 @@ func assertTagProperties(c *Config, errs *packer.MultiError) { } func assertRequiredParametersSet(c *Config, errs *packer.MultiError) { - c.ClientConfig.assertRequiredParametersSet(errs) + c.ClientConfig.Validate(errs) ///////////////////////////////////////////// // Capture diff --git a/builder/azure/arm/config_retriever.go b/builder/azure/arm/config_retriever.go deleted file mode 100644 index 4760041d8..000000000 --- a/builder/azure/arm/config_retriever.go +++ /dev/null @@ -1,75 +0,0 @@ -package arm - -// Method to resolve information about the user so that a client can be -// constructed to communicated with Azure. -// -// The following data are resolved. -// -// 1. TenantID - -import ( - "encoding/json" - "io/ioutil" - "net/http" - - "github.com/Azure/go-autorest/autorest/azure" - "github.com/hashicorp/packer/builder/azure/common" -) - -type configRetriever struct { - // test seams - findTenantID func(azure.Environment, string) (string, error) -} - -func newConfigRetriever() configRetriever { - return configRetriever{ - common.FindTenantID, - } -} - -func (cr configRetriever) FillParameters(c *Config) error { - if c.SubscriptionID == "" { - subscriptionID, err := cr.getSubscriptionFromIMDS() - if err != nil { - return err - } - c.SubscriptionID = subscriptionID - } - - if c.TenantID == "" { - tenantID, err := cr.findTenantID(*c.cloudEnvironment, c.SubscriptionID) - if err != nil { - return err - } - c.TenantID = tenantID - } - - return nil -} - -func (cr configRetriever) getSubscriptionFromIMDS() (string, error) { - client := &http.Client{} - - req, _ := http.NewRequest("GET", "http://169.254.169.254/metadata/instance/compute", nil) - req.Header.Add("Metadata", "True") - - q := req.URL.Query() - q.Add("format", "json") - q.Add("api-version", "2017-08-01") - - req.URL.RawQuery = q.Encode() - resp, err := client.Do(req) - if err != nil { - return "", err - } - - defer resp.Body.Close() - resp_body, _ := ioutil.ReadAll(resp.Body) - result := map[string]string{} - err = json.Unmarshal(resp_body, &result) - if err != nil { - return "", err - } - - return result["subscriptionId"], nil -} diff --git a/builder/azure/arm/config_retriever_test.go b/builder/azure/arm/config_retriever_test.go deleted file mode 100644 index 2fd84845f..000000000 --- a/builder/azure/arm/config_retriever_test.go +++ /dev/null @@ -1,62 +0,0 @@ -package arm - -import ( - "errors" - "testing" - - "github.com/Azure/go-autorest/autorest/azure" -) - -func TestConfigRetrieverFillsTenantIDWhenEmpty(t *testing.T) { - c, _, _ := newConfig(getArmBuilderConfiguration(), getPackerConfiguration()) - if expected := ""; c.TenantID != expected { - t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) - } - - sut := newTestConfigRetriever() - retrievedTid := "my-tenant-id" - sut.findTenantID = func(azure.Environment, string) (string, error) { return retrievedTid, nil } - if err := sut.FillParameters(c); err != nil { - t.Errorf("Unexpected error when calling sut.FillParameters: %v", err) - } - - if expected := retrievedTid; c.TenantID != expected { - t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) - } -} - -func TestConfigRetrieverLeavesTenantIDWhenNotEmpty(t *testing.T) { - c, _, _ := newConfig(getArmBuilderConfiguration(), getPackerConfiguration()) - userSpecifiedTid := "not-empty" - c.TenantID = userSpecifiedTid - - sut := newTestConfigRetriever() - sut.findTenantID = nil // assert that this not even called - if err := sut.FillParameters(c); err != nil { - t.Errorf("Unexpected error when calling sut.FillParameters: %v", err) - } - - if expected := userSpecifiedTid; c.TenantID != expected { - t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) - } -} - -func TestConfigRetrieverReturnsErrorWhenTenantIDEmptyAndRetrievalFails(t *testing.T) { - c, _, _ := newConfig(getArmBuilderConfiguration(), getPackerConfiguration()) - if expected := ""; c.TenantID != expected { - t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) - } - - sut := newTestConfigRetriever() - errorString := "sorry, I failed" - sut.findTenantID = func(azure.Environment, string) (string, error) { return "", errors.New(errorString) } - if err := sut.FillParameters(c); err != nil && err.Error() != errorString { - t.Errorf("Unexpected error when calling sut.FillParameters: %v", err) - } -} - -func newTestConfigRetriever() configRetriever { - return configRetriever{ - findTenantID: func(azure.Environment, string) (string, error) { return "findTenantID is mocked", nil }, - } -} diff --git a/builder/azure/arm/config_test.go b/builder/azure/arm/config_test.go index ef5c9fb93..9ed25fffa 100644 --- a/builder/azure/arm/config_test.go +++ b/builder/azure/arm/config_test.go @@ -41,8 +41,8 @@ func TestConfigShouldProvideReasonableDefaultValues(t *testing.T) { t.Error("Expected 'VMSize' to be populated, but it was empty!") } - if c.ObjectID != "" { - t.Errorf("Expected 'ObjectID' to be nil, but it was '%s'!", c.ObjectID) + if c.ClientConfig.ObjectID != "" { + t.Errorf("Expected 'ObjectID' to be nil, but it was '%s'!", c.ClientConfig.ObjectID) } if c.managedImageStorageAccountType == "" { @@ -273,12 +273,12 @@ func TestConfigVirtualNetworkSubnetNameMustBeSetWithVirtualNetworkName(t *testin func TestConfigShouldDefaultToPublicCloud(t *testing.T) { c, _, _ := newConfig(getArmBuilderConfiguration(), getPackerConfiguration()) - if c.CloudEnvironmentName != "Public" { - t.Errorf("Expected 'CloudEnvironmentName' to default to 'Public', but got '%s'.", c.CloudEnvironmentName) + if c.ClientConfig.CloudEnvironmentName != "Public" { + t.Errorf("Expected 'CloudEnvironmentName' to default to 'Public', but got '%s'.", c.ClientConfig.CloudEnvironmentName) } - if c.cloudEnvironment == nil || c.cloudEnvironment.Name != "AzurePublicCloud" { - t.Errorf("Expected 'cloudEnvironment' to be set to 'AzurePublicCloud', but got '%s'.", c.cloudEnvironment) + if c.ClientConfig.CloudEnvironment == nil || c.ClientConfig.CloudEnvironment.Name != "AzurePublicCloud" { + t.Errorf("Expected 'cloudEnvironment' to be set to 'AzurePublicCloud', but got '%s'.", c.ClientConfig.CloudEnvironment) } } @@ -327,8 +327,8 @@ func TestConfigInstantiatesCorrectAzureEnvironment(t *testing.T) { t.Fatal(err) } - if c.cloudEnvironment == nil || c.cloudEnvironment.Name != x.environmentName { - t.Errorf("Expected 'cloudEnvironment' to be set to '%s', but got '%s'.", x.environmentName, c.cloudEnvironment) + if c.ClientConfig.CloudEnvironment == nil || c.ClientConfig.CloudEnvironment.Name != x.environmentName { + t.Errorf("Expected 'cloudEnvironment' to be set to '%s', but got '%s'.", x.environmentName, c.ClientConfig.CloudEnvironment) } } } diff --git a/builder/azure/arm/template_factory.go b/builder/azure/arm/template_factory.go index 6a2bf3d2a..0401f9f9b 100644 --- a/builder/azure/arm/template_factory.go +++ b/builder/azure/arm/template_factory.go @@ -18,8 +18,8 @@ func GetKeyVaultDeployment(config *Config) (*resources.Deployment, error) { params := &template.TemplateParameters{ KeyVaultName: &template.TemplateParameter{Value: config.tmpKeyVaultName}, KeyVaultSecretValue: &template.TemplateParameter{Value: config.winrmCertificate}, - ObjectId: &template.TemplateParameter{Value: config.ObjectID}, - TenantId: &template.TemplateParameter{Value: config.TenantID}, + ObjectId: &template.TemplateParameter{Value: config.ClientConfig.ObjectID}, + TenantId: &template.TemplateParameter{Value: config.ClientConfig.TenantID}, } builder, _ := template.NewTemplateBuilder(template.KeyVault) @@ -64,7 +64,7 @@ func GetVirtualMachineDeployment(config *Config) (*resources.Deployment, error) builder.SetManagedDiskUrl(config.customManagedImageID, config.managedImageStorageAccountType, config.diskCachingType) } else if config.ManagedImageName != "" && config.ImagePublisher != "" { imageID := fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Compute/locations/%s/publishers/%s/ArtifactTypes/vmimage/offers/%s/skus/%s/versions/%s", - config.SubscriptionID, + config.ClientConfig.SubscriptionID, config.Location, config.ImagePublisher, config.ImageOffer, diff --git a/builder/azure/arm/template_factory_test.go b/builder/azure/arm/template_factory_test.go index af003f0c6..382a0b490 100644 --- a/builder/azure/arm/template_factory_test.go +++ b/builder/azure/arm/template_factory_test.go @@ -488,11 +488,11 @@ func TestKeyVaultDeployment02(t *testing.T) { t.Fatal(err) } - if params.ObjectId.Value != c.ObjectID { - t.Errorf("Expected template parameter 'ObjectId' to be %s, but got %s.", params.ObjectId.Value, c.ObjectID) + if params.ObjectId.Value != c.ClientConfig.ObjectID { + t.Errorf("Expected template parameter 'ObjectId' to be %s, but got %s.", params.ObjectId.Value, c.ClientConfig.ObjectID) } - if params.TenantId.Value != c.TenantID { - t.Errorf("Expected template parameter 'TenantId' to be %s, but got %s.", params.TenantId.Value, c.TenantID) + if params.TenantId.Value != c.ClientConfig.TenantID { + t.Errorf("Expected template parameter 'TenantId' to be %s, but got %s.", params.TenantId.Value, c.ClientConfig.TenantID) } if params.KeyVaultName.Value != c.tmpKeyVaultName { t.Errorf("Expected template parameter 'KeyVaultName' to be %s, but got %s.", params.KeyVaultName.Value, c.tmpKeyVaultName) diff --git a/builder/azure/arm/clientconfig.go b/builder/azure/common/client/config.go similarity index 59% rename from builder/azure/arm/clientconfig.go rename to builder/azure/common/client/config.go index f3a561d73..5cdb0e8c0 100644 --- a/builder/azure/arm/clientconfig.go +++ b/builder/azure/common/client/config.go @@ -1,9 +1,10 @@ //go:generate struct-markdown -package arm +package client import ( "fmt" + "github.com/hashicorp/packer/builder/azure/common" "os" "strings" "time" @@ -14,41 +15,65 @@ import ( "github.com/hashicorp/packer/packer" ) -// ClientConfig allows for various ways to authenticate Azure clients -type ClientConfig struct { +// Config allows for various ways to authenticate Azure clients. +// When `client_id` and `subscription_id` are specified, Packer will use the +// specified Azure Active Directoty (AAD) Service Principal (SP). +// If only `subscription_id` is specified, Packer will try to interactively +// log on the current user (tokens will be cached). +// If none of these options are specified, Packer will attempt to use the +// Managed Identity and subscription of the VM that Packer is running on. +// This will only work if Packer is running on an Azure VM. +type Config struct { // One of Public, China, Germany, or // USGovernment. Defaults to Public. Long forms such as // USGovernmentCloud and AzureUSGovernmentCloud are also supported. CloudEnvironmentName string `mapstructure:"cloud_environment_name" required:"false"` - cloudEnvironment *azure.Environment + CloudEnvironment *azure.Environment // Authentication fields - // Client ID + // The application ID of the AAD Service Principal. + // Requires either `client_secret`, `client_cert_path` or `client_jwt` to be set as well. ClientID string `mapstructure:"client_id"` - // Client secret/password + // A password/secret registered for the AAD SP. ClientSecret string `mapstructure:"client_secret"` - // Certificate path for client auth + // The path to a certificate that will be used to authenticate as the specified AAD SP. ClientCertPath string `mapstructure:"client_cert_path"` - // JWT bearer token for client auth (RFC 7523, Sec. 2.2) + // A JWT bearer token for client auth (RFC 7523, Sec. 2.2) that will be used + // to authenticate the AAD SP. Provides more control over token the expiration + // when using certificate authentication than when using `client_cert_path`. ClientJWT string `mapstructure:"client_jwt"` - ObjectID string `mapstructure:"object_id"` - // The account identifier with which your client_id and - // subscription_id are associated. If not specified, tenant_id will be - // looked up using subscription_id. - TenantID string `mapstructure:"tenant_id" required:"false"` + // The object ID for the AAD SP. Optional, will be derived from the oAuth token if left empty. + ObjectID string `mapstructure:"object_id"` + + // The Active Directory tenant identifier with which your `client_id` and + // `subscription_id` are associated. If not specified, `tenant_id` will be + // looked up using `subscription_id`. + TenantID string `mapstructure:"tenant_id" required:"false"` + // The subscription to use. SubscriptionID string `mapstructure:"subscription_id"` + + authType string } +const ( + authTypeDeviceLogin = "DeviceLogin" + authTypeMSI = "ManagedIdentity" + authTypeClientSecret = "ClientSecret" + authTypeClientCert = "ClientCertificate" + authTypeClientBearerJWT = "ClientBearerJWT" +) + const DefaultCloudEnvironmentName = "Public" -func (c *ClientConfig) provideDefaultValues() { +func (c *Config) SetDefaultValues() error { if c.CloudEnvironmentName == "" { c.CloudEnvironmentName = DefaultCloudEnvironmentName } + return c.setCloudEnvironment() } -func (c *ClientConfig) setCloudEnvironment() error { +func (c *Config) setCloudEnvironment() error { lookup := map[string]string{ "CHINA": "AzureChinaCloud", "CHINACLOUD": "AzureChinaCloud", @@ -78,11 +103,11 @@ func (c *ClientConfig) setCloudEnvironment() error { } env, err := azure.EnvironmentFromName(envName) - c.cloudEnvironment = &env + c.CloudEnvironment = &env return err } -func (c ClientConfig) assertRequiredParametersSet(errs *packer.MultiError) { +func (c Config) Validate(errs *packer.MultiError) { ///////////////////////////////////////////// // Authentication via OAUTH @@ -95,7 +120,7 @@ func (c ClientConfig) assertRequiredParametersSet(errs *packer.MultiError) { // readable by the ObjectID of the App. There may be another way to handle // this case, but I am not currently aware of it - send feedback. - if c.useMSI() { + if c.UseMSI() { return } @@ -156,7 +181,7 @@ func (c ClientConfig) assertRequiredParametersSet(errs *packer.MultiError) { " - subscription_id, client_id and client_jwt.")) } -func (c ClientConfig) useDeviceLogin() bool { +func (c Config) useDeviceLogin() bool { return c.SubscriptionID != "" && c.ClientID == "" && c.ClientSecret == "" && @@ -164,7 +189,7 @@ func (c ClientConfig) useDeviceLogin() bool { c.ClientCertPath == "" } -func (c ClientConfig) useMSI() bool { +func (c Config) UseMSI() bool { return c.SubscriptionID == "" && c.ClientID == "" && c.ClientSecret == "" && @@ -173,7 +198,7 @@ func (c ClientConfig) useMSI() bool { c.TenantID == "" } -func (c ClientConfig) getServicePrincipalTokens( +func (c Config) GetServicePrincipalTokens( say func(string)) ( servicePrincipalToken *adal.ServicePrincipalToken, servicePrincipalTokenVault *adal.ServicePrincipalToken, @@ -182,25 +207,27 @@ func (c ClientConfig) getServicePrincipalTokens( tenantID := c.TenantID var auth oAuthTokenProvider - - if c.useDeviceLogin() { + switch c.authType { + case authTypeDeviceLogin: say("Getting tokens using device flow") - auth = NewDeviceFlowOAuthTokenProvider(*c.cloudEnvironment, say, tenantID) - } else if c.useMSI() { + auth = NewDeviceFlowOAuthTokenProvider(*c.CloudEnvironment, say, tenantID) + case authTypeMSI: say("Getting tokens using Managed Identity for Azure") - auth = NewMSIOAuthTokenProvider(*c.cloudEnvironment) - } else if c.ClientSecret != "" { + auth = NewMSIOAuthTokenProvider(*c.CloudEnvironment) + case authTypeClientSecret: say("Getting tokens using client secret") - auth = NewSecretOAuthTokenProvider(*c.cloudEnvironment, c.ClientID, c.ClientSecret, tenantID) - } else if c.ClientCertPath != "" { + auth = NewSecretOAuthTokenProvider(*c.CloudEnvironment, c.ClientID, c.ClientSecret, tenantID) + case authTypeClientCert: say("Getting tokens using client certificate") - auth, err = NewCertOAuthTokenProvider(*c.cloudEnvironment, c.ClientID, c.ClientCertPath, tenantID) + auth, err = NewCertOAuthTokenProvider(*c.CloudEnvironment, c.ClientID, c.ClientCertPath, tenantID) if err != nil { return nil, nil, err } - } else { + case authTypeClientBearerJWT: say("Getting tokens using client bearer JWT") - auth = NewJWTOAuthTokenProvider(*c.cloudEnvironment, c.ClientID, c.ClientJWT, tenantID) + auth = NewJWTOAuthTokenProvider(*c.CloudEnvironment, c.ClientID, c.ClientJWT, tenantID) + default: + panic("authType not set, call FillParameters, or set explicitly") } servicePrincipalToken, err = auth.getServicePrincipalToken() @@ -214,7 +241,7 @@ func (c ClientConfig) getServicePrincipalTokens( } servicePrincipalTokenVault, err = auth.getServicePrincipalTokenWithResource( - strings.TrimRight(c.cloudEnvironment.KeyVaultEndpoint, "/")) + strings.TrimRight(c.CloudEnvironment.KeyVaultEndpoint, "/")) if err != nil { return nil, nil, err } @@ -226,3 +253,48 @@ func (c ClientConfig) getServicePrincipalTokens( return servicePrincipalToken, servicePrincipalTokenVault, nil } + +func (c *Config) FillParameters() error { + if c.authType == "" { + if c.useDeviceLogin() { + c.authType = authTypeDeviceLogin + } else if c.UseMSI() { + c.authType = authTypeMSI + } else if c.ClientSecret != "" { + c.authType = authTypeClientSecret + } else if c.ClientCertPath != "" { + c.authType = authTypeClientCert + } else { + c.authType = authTypeClientBearerJWT + } + } + + if c.authType == authTypeMSI && c.SubscriptionID == "" { + + subscriptionID, err := getSubscriptionFromIMDS() + if err != nil { + return fmt.Errorf("error fetching subscriptionID from VM metadata service for Managed Identity authentication: %v", err) + } + c.SubscriptionID = subscriptionID + } + + if c.TenantID == "" { + tenantID, err := common.FindTenantID(*c.CloudEnvironment, c.SubscriptionID) + if err != nil { + return err + } + c.TenantID = tenantID + } + + if c.CloudEnvironment == nil { + err := c.setCloudEnvironment() + if err != nil { + return err + } + } + + return nil +} + +// allow override for unit tests +var findTenantID = common.FindTenantID diff --git a/builder/azure/common/client/config_retriever.go b/builder/azure/common/client/config_retriever.go new file mode 100644 index 000000000..b4b25e5fb --- /dev/null +++ b/builder/azure/common/client/config_retriever.go @@ -0,0 +1,37 @@ +package client + +import ( + "encoding/json" + "io/ioutil" + "net/http" +) + +// allow override for unit tests +var getSubscriptionFromIMDS = _getSubscriptionFromIMDS + +func _getSubscriptionFromIMDS() (string, error) { + client := &http.Client{} + + req, _ := http.NewRequest("GET", "http://169.254.169.254/metadata/instance/compute", nil) + req.Header.Add("Metadata", "True") + + q := req.URL.Query() + q.Add("format", "json") + q.Add("api-version", "2017-08-01") + + req.URL.RawQuery = q.Encode() + resp, err := client.Do(req) + if err != nil { + return "", err + } + + defer resp.Body.Close() + resp_body, _ := ioutil.ReadAll(resp.Body) + result := map[string]string{} + err = json.Unmarshal(resp_body, &result) + if err != nil { + return "", err + } + + return result["subscriptionId"], nil +} diff --git a/builder/azure/common/client/config_retriever_test.go b/builder/azure/common/client/config_retriever_test.go new file mode 100644 index 000000000..6c707e95b --- /dev/null +++ b/builder/azure/common/client/config_retriever_test.go @@ -0,0 +1,56 @@ +package client + +import ( + "errors" + "testing" + + "github.com/Azure/go-autorest/autorest/azure" +) + +func TestConfigRetrieverFillsTenantIDWhenEmpty(t *testing.T) { + c := Config{CloudEnvironmentName: "AzurePublicCloud"} + if expected := ""; c.TenantID != expected { + t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) + } + + retrievedTid := "my-tenant-id" + findTenantID = func(azure.Environment, string) (string, error) { return retrievedTid, nil } + getSubscriptionFromIMDS = func() (string, error) { return "unittest", nil } + if err := c.FillParameters(); err != nil { + t.Errorf("Unexpected error when calling c.FillParameters: %v", err) + } + + if expected := retrievedTid; c.TenantID != expected { + t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) + } +} + +func TestConfigRetrieverLeavesTenantIDWhenNotEmpty(t *testing.T) { + c := Config{CloudEnvironmentName: "AzurePublicCloud"} + userSpecifiedTid := "not-empty" + c.TenantID = userSpecifiedTid + + findTenantID = nil // assert that this not even called + getSubscriptionFromIMDS = func() (string, error) { return "unittest", nil } + if err := c.FillParameters(); err != nil { + t.Errorf("Unexpected error when calling c.FillParameters: %v", err) + } + + if expected := userSpecifiedTid; c.TenantID != expected { + t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) + } +} + +func TestConfigRetrieverReturnsErrorWhenTenantIDEmptyAndRetrievalFails(t *testing.T) { + c := Config{CloudEnvironmentName: "AzurePublicCloud"} + if expected := ""; c.TenantID != expected { + t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) + } + + errorString := "sorry, I failed" + findTenantID = func(azure.Environment, string) (string, error) { return "", errors.New(errorString) } + getSubscriptionFromIMDS = func() (string, error) { return "unittest", nil } + if err := c.FillParameters(); err != nil && err.Error() != errorString { + t.Errorf("Unexpected error when calling c.FillParameters: %v", err) + } +} diff --git a/builder/azure/arm/clientconfig_test.go b/builder/azure/common/client/config_test.go similarity index 78% rename from builder/azure/arm/clientconfig_test.go rename to builder/azure/common/client/config_test.go index 3ae8af6f0..3e76b4d2b 100644 --- a/builder/azure/arm/clientconfig_test.go +++ b/builder/azure/common/client/config_test.go @@ -1,4 +1,4 @@ -package arm +package client import ( crand "crypto/rand" @@ -21,52 +21,52 @@ func Test_ClientConfig_RequiredParametersSet(t *testing.T) { tests := []struct { name string - config ClientConfig + config Config wantErr bool }{ { name: "no client_id, client_secret or subscription_id should enable MSI auth", - config: ClientConfig{}, + config: Config{}, wantErr: false, }, { name: "subscription_id is set will trigger device flow", - config: ClientConfig{ + config: Config{ SubscriptionID: "error", }, wantErr: false, }, { name: "client_id without client_secret, client_cert_path or client_jwt should error", - config: ClientConfig{ + config: Config{ ClientID: "error", }, wantErr: true, }, { name: "client_secret without client_id should error", - config: ClientConfig{ + config: Config{ ClientSecret: "error", }, wantErr: true, }, { name: "client_cert_path without client_id should error", - config: ClientConfig{ + config: Config{ ClientCertPath: "/dev/null", }, wantErr: true, }, { name: "client_jwt without client_id should error", - config: ClientConfig{ + config: Config{ ClientJWT: "error", }, wantErr: true, }, { name: "missing subscription_id when using secret", - config: ClientConfig{ + config: Config{ ClientID: "ok", ClientSecret: "ok", }, @@ -74,7 +74,7 @@ func Test_ClientConfig_RequiredParametersSet(t *testing.T) { }, { name: "missing subscription_id when using certificate", - config: ClientConfig{ + config: Config{ ClientID: "ok", ClientCertPath: "ok", }, @@ -82,7 +82,7 @@ func Test_ClientConfig_RequiredParametersSet(t *testing.T) { }, { name: "missing subscription_id when using JWT", - config: ClientConfig{ + config: Config{ ClientID: "ok", ClientJWT: "ok", }, @@ -90,7 +90,7 @@ func Test_ClientConfig_RequiredParametersSet(t *testing.T) { }, { name: "too many client_* values", - config: ClientConfig{ + config: Config{ SubscriptionID: "ok", ClientID: "ok", ClientSecret: "ok", @@ -100,7 +100,7 @@ func Test_ClientConfig_RequiredParametersSet(t *testing.T) { }, { name: "too many client_* values (2)", - config: ClientConfig{ + config: Config{ SubscriptionID: "ok", ClientID: "ok", ClientSecret: "ok", @@ -110,7 +110,7 @@ func Test_ClientConfig_RequiredParametersSet(t *testing.T) { }, { name: "tenant_id alone should fail", - config: ClientConfig{ + config: Config{ TenantID: "ok", }, wantErr: true, @@ -120,7 +120,7 @@ func Test_ClientConfig_RequiredParametersSet(t *testing.T) { t.Run(tt.name, func(t *testing.T) { errs := &packer.MultiError{} - tt.config.assertRequiredParametersSet(errs) + tt.config.Validate(errs) if (len(errs.Errors) != 0) != tt.wantErr { t.Errorf("newConfig() error = %v, wantErr %v", errs, tt.wantErr) return @@ -131,13 +131,13 @@ func Test_ClientConfig_RequiredParametersSet(t *testing.T) { func Test_ClientConfig_DeviceLogin(t *testing.T) { getEnvOrSkip(t, "AZURE_DEVICE_LOGIN") - cfg := ClientConfig{ + cfg := Config{ SubscriptionID: getEnvOrSkip(t, "AZURE_SUBSCRIPTION"), - cloudEnvironment: getCloud(), + CloudEnvironment: getCloud(), } assertValid(t, cfg) - spt, sptkv, err := cfg.getServicePrincipalTokens( + spt, sptkv, err := cfg.GetServicePrincipalTokens( func(s string) { fmt.Printf("SAY: %s\n", s) }) if err != nil { t.Fatalf("Expected nil err, but got: %v", err) @@ -159,16 +159,16 @@ func Test_ClientConfig_DeviceLogin(t *testing.T) { } func Test_ClientConfig_ClientPassword(t *testing.T) { - cfg := ClientConfig{ + cfg := Config{ SubscriptionID: getEnvOrSkip(t, "AZURE_SUBSCRIPTION"), ClientID: getEnvOrSkip(t, "AZURE_CLIENTID"), ClientSecret: getEnvOrSkip(t, "AZURE_CLIENTSECRET"), TenantID: getEnvOrSkip(t, "AZURE_TENANTID"), - cloudEnvironment: getCloud(), + CloudEnvironment: getCloud(), } assertValid(t, cfg) - spt, sptkv, err := cfg.getServicePrincipalTokens(func(s string) { fmt.Printf("SAY: %s\n", s) }) + spt, sptkv, err := cfg.GetServicePrincipalTokens(func(s string) { fmt.Printf("SAY: %s\n", s) }) if err != nil { t.Fatalf("Expected nil err, but got: %v", err) } @@ -189,16 +189,16 @@ func Test_ClientConfig_ClientPassword(t *testing.T) { } func Test_ClientConfig_ClientCert(t *testing.T) { - cfg := ClientConfig{ + cfg := Config{ SubscriptionID: getEnvOrSkip(t, "AZURE_SUBSCRIPTION"), ClientID: getEnvOrSkip(t, "AZURE_CLIENTID"), ClientCertPath: getEnvOrSkip(t, "AZURE_CLIENTCERT"), TenantID: getEnvOrSkip(t, "AZURE_TENANTID"), - cloudEnvironment: getCloud(), + CloudEnvironment: getCloud(), } assertValid(t, cfg) - spt, sptkv, err := cfg.getServicePrincipalTokens(func(s string) { fmt.Printf("SAY: %s\n", s) }) + spt, sptkv, err := cfg.GetServicePrincipalTokens(func(s string) { fmt.Printf("SAY: %s\n", s) }) if err != nil { t.Fatalf("Expected nil err, but got: %v", err) } @@ -219,16 +219,16 @@ func Test_ClientConfig_ClientCert(t *testing.T) { } func Test_ClientConfig_ClientJWT(t *testing.T) { - cfg := ClientConfig{ + cfg := Config{ SubscriptionID: getEnvOrSkip(t, "AZURE_SUBSCRIPTION"), ClientID: getEnvOrSkip(t, "AZURE_CLIENTID"), ClientJWT: getEnvOrSkip(t, "AZURE_CLIENTJWT"), TenantID: getEnvOrSkip(t, "AZURE_TENANTID"), - cloudEnvironment: getCloud(), + CloudEnvironment: getCloud(), } assertValid(t, cfg) - spt, sptkv, err := cfg.getServicePrincipalTokens(func(s string) { fmt.Printf("SAY: %s\n", s) }) + spt, sptkv, err := cfg.GetServicePrincipalTokens(func(s string) { fmt.Printf("SAY: %s\n", s) }) if err != nil { t.Fatalf("Expected nil err, but got: %v", err) } @@ -270,106 +270,109 @@ func getCloud() *azure.Environment { func Test_ClientConfig_CanUseDeviceCode(t *testing.T) { // TenantID is optional, but Builder will look up tenant ID before requesting t.Run("without TenantID", func(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" + cfg := Config{ + SubscriptionID: "12345", + } assertValid(t, cfg) }) t.Run("with TenantID", func(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" - cfg.TenantID = "12345" + cfg := Config{ + SubscriptionID: "12345", + TenantID: "12345", + } assertValid(t, cfg) }) } -func assertValid(t *testing.T, cfg ClientConfig) { +func assertValid(t *testing.T, cfg Config) { errs := &packer.MultiError{} - cfg.assertRequiredParametersSet(errs) + cfg.Validate(errs) if len(errs.Errors) != 0 { t.Fatal("Expected errs to be empty: ", errs) } } -func assertInvalid(t *testing.T, cfg ClientConfig) { +func assertInvalid(t *testing.T, cfg Config) { errs := &packer.MultiError{} - cfg.assertRequiredParametersSet(errs) + cfg.Validate(errs) if len(errs.Errors) == 0 { t.Fatal("Expected errs to be non-empty") } } func Test_ClientConfig_CanUseClientSecret(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" - cfg.ClientID = "12345" - cfg.ClientSecret = "12345" + cfg := Config{ + SubscriptionID: "12345", + ClientID: "12345", + ClientSecret: "12345", + } assertValid(t, cfg) } func Test_ClientConfig_CanUseClientSecretWithTenantID(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" - cfg.ClientID = "12345" - cfg.ClientSecret = "12345" - cfg.TenantID = "12345" + cfg := Config{ + SubscriptionID: "12345", + ClientID: "12345", + ClientSecret: "12345", + TenantID: "12345", + } assertValid(t, cfg) } func Test_ClientConfig_CanUseClientJWT(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" - cfg.ClientID = "12345" - cfg.ClientJWT = getJWT(10*time.Minute, true) + cfg := Config{ + SubscriptionID: "12345", + ClientID: "12345", + ClientJWT: getJWT(10*time.Minute, true), + } assertValid(t, cfg) } func Test_ClientConfig_CanUseClientJWTWithTenantID(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" - cfg.ClientID = "12345" - cfg.ClientJWT = getJWT(10*time.Minute, true) - cfg.TenantID = "12345" + cfg := Config{ + SubscriptionID: "12345", + ClientID: "12345", + ClientJWT: getJWT(10*time.Minute, true), + TenantID: "12345", + } assertValid(t, cfg) } func Test_ClientConfig_CannotUseBothClientJWTAndSecret(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" - cfg.ClientID = "12345" - cfg.ClientSecret = "12345" - cfg.ClientJWT = getJWT(10*time.Minute, true) + cfg := Config{ + SubscriptionID: "12345", + ClientID: "12345", + ClientSecret: "12345", + ClientJWT: getJWT(10*time.Minute, true), + } assertInvalid(t, cfg) } func Test_ClientConfig_ClientJWTShouldBeValidForAtLeast5Minutes(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" - cfg.ClientID = "12345" - cfg.ClientJWT = getJWT(time.Minute, true) + cfg := Config{ + SubscriptionID: "12345", + ClientID: "12345", + ClientJWT: getJWT(time.Minute, true), + } assertInvalid(t, cfg) } func Test_ClientConfig_ClientJWTShouldHaveThumbprint(t *testing.T) { - cfg := emptyClientConfig() - cfg.SubscriptionID = "12345" - cfg.ClientID = "12345" - cfg.ClientJWT = getJWT(10*time.Minute, false) + cfg := Config{ + SubscriptionID: "12345", + ClientID: "12345", + ClientJWT: getJWT(10*time.Minute, false), + } assertInvalid(t, cfg) } -func emptyClientConfig() ClientConfig { - cfg := ClientConfig{} - _ = cfg.setCloudEnvironment() - return cfg -} - func Test_getJWT(t *testing.T) { if getJWT(time.Minute, true) == "" { t.Fatalf("getJWT is broken") diff --git a/builder/azure/arm/authenticate.go b/builder/azure/common/client/tokenprovider.go similarity index 94% rename from builder/azure/arm/authenticate.go rename to builder/azure/common/client/tokenprovider.go index 8204660de..86cd80a9c 100644 --- a/builder/azure/arm/authenticate.go +++ b/builder/azure/common/client/tokenprovider.go @@ -1,4 +1,4 @@ -package arm +package client import ( "github.com/Azure/go-autorest/autorest/adal" diff --git a/builder/azure/arm/authenticate_cert.go b/builder/azure/common/client/tokenprovider_cert.go similarity index 98% rename from builder/azure/arm/authenticate_cert.go rename to builder/azure/common/client/tokenprovider_cert.go index 0c1fb6e49..38947827c 100644 --- a/builder/azure/arm/authenticate_cert.go +++ b/builder/azure/common/client/tokenprovider_cert.go @@ -1,4 +1,4 @@ -package arm +package client import ( "crypto/ecdsa" @@ -14,7 +14,7 @@ import ( "time" "github.com/Azure/go-autorest/autorest/azure" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" ) func NewCertOAuthTokenProvider(env azure.Environment, clientID, clientCertPath, tenantID string) (oAuthTokenProvider, error) { diff --git a/builder/azure/arm/authenticate_devicewflow.go b/builder/azure/common/client/tokenprovider_devicewflow.go similarity index 98% rename from builder/azure/arm/authenticate_devicewflow.go rename to builder/azure/common/client/tokenprovider_devicewflow.go index b859a3066..3c2b9fd17 100644 --- a/builder/azure/arm/authenticate_devicewflow.go +++ b/builder/azure/common/client/tokenprovider_devicewflow.go @@ -1,4 +1,4 @@ -package arm +package client import ( "fmt" diff --git a/builder/azure/arm/authenticate_jwt.go b/builder/azure/common/client/tokenprovider_jwt.go similarity index 98% rename from builder/azure/arm/authenticate_jwt.go rename to builder/azure/common/client/tokenprovider_jwt.go index d4bb41748..905d1f22f 100644 --- a/builder/azure/arm/authenticate_jwt.go +++ b/builder/azure/common/client/tokenprovider_jwt.go @@ -1,4 +1,4 @@ -package arm +package client import ( "net/url" diff --git a/builder/azure/arm/authenticate_msi.go b/builder/azure/common/client/tokenprovider_msi.go similarity index 97% rename from builder/azure/arm/authenticate_msi.go rename to builder/azure/common/client/tokenprovider_msi.go index b9f551eeb..8c97a821d 100644 --- a/builder/azure/arm/authenticate_msi.go +++ b/builder/azure/common/client/tokenprovider_msi.go @@ -1,4 +1,4 @@ -package arm +package client import ( "github.com/Azure/go-autorest/autorest/adal" diff --git a/builder/azure/arm/authenticate_secret.go b/builder/azure/common/client/tokenprovider_secret.go similarity index 98% rename from builder/azure/arm/authenticate_secret.go rename to builder/azure/common/client/tokenprovider_secret.go index 6a434591a..7834742bb 100644 --- a/builder/azure/arm/authenticate_secret.go +++ b/builder/azure/common/client/tokenprovider_secret.go @@ -1,4 +1,4 @@ -package arm +package client import ( "github.com/Azure/go-autorest/autorest/adal" diff --git a/builder/azure/arm/authenticate_secret_test.go b/builder/azure/common/client/tokenprovider_secret_test.go similarity index 98% rename from builder/azure/arm/authenticate_secret_test.go rename to builder/azure/common/client/tokenprovider_secret_test.go index ce8868ff5..9ca685e47 100644 --- a/builder/azure/arm/authenticate_secret_test.go +++ b/builder/azure/common/client/tokenprovider_secret_test.go @@ -1,4 +1,4 @@ -package arm +package client import ( "testing" diff --git a/builder/azure/arm/template_funcs.go b/builder/azure/common/template_funcs.go similarity index 92% rename from builder/azure/arm/template_funcs.go rename to builder/azure/common/template_funcs.go index 1aa9546ab..1a399b53f 100644 --- a/builder/azure/arm/template_funcs.go +++ b/builder/azure/common/template_funcs.go @@ -1,4 +1,4 @@ -package arm +package common import ( "bytes" @@ -23,9 +23,6 @@ func isValidByteValue(b byte) bool { // Clean up image name by replacing invalid characters with "-" // Names are not allowed to end in '.', '-', or '_' and are trimmed. func templateCleanImageName(s string) string { - if ok, _ := assertManagedImageName(s, ""); ok { - return s - } b := []byte(s) newb := make([]byte, len(b)) for i := range newb { diff --git a/builder/azure/arm/template_funcs_test.go b/builder/azure/common/template_funcs_test.go similarity index 98% rename from builder/azure/arm/template_funcs_test.go rename to builder/azure/common/template_funcs_test.go index 68acefba8..1bdcf303f 100644 --- a/builder/azure/arm/template_funcs_test.go +++ b/builder/azure/common/template_funcs_test.go @@ -1,4 +1,4 @@ -package arm +package common import "testing" diff --git a/website/source/docs/builders/azure.html.md.erb b/website/source/docs/builders/azure.html.md.erb index 39168f003..b09ff58fd 100644 --- a/website/source/docs/builders/azure.html.md.erb +++ b/website/source/docs/builders/azure.html.md.erb @@ -157,10 +157,12 @@ To use an existing resource group you **must** provide: Providing `temp_resource_group_name` or `location` in combination with `build_resource_group_name` is not allowed. +<%= partial "partials/builder/azure/common/client/_Config" %> + ### Optional: <%= partial "partials/builder/azure/arm/Config-not-required" %> -<%= partial "partials/builder/azure/arm/ClientConfig-not-required" %> +<%= partial "partials/builder/azure/common/client/_Config-not-required" %> ## Basic Example diff --git a/website/source/partials/builder/azure/arm/_ClientConfig-not-required.html.md b/website/source/partials/builder/azure/arm/_ClientConfig-not-required.html.md deleted file mode 100644 index f07c96675..000000000 --- a/website/source/partials/builder/azure/arm/_ClientConfig-not-required.html.md +++ /dev/null @@ -1,20 +0,0 @@ - - -- `cloud_environment_name` (string) - One of Public, China, Germany, or - USGovernment. Defaults to Public. Long forms such as - USGovernmentCloud and AzureUSGovernmentCloud are also supported. - -- `client_id` (string) - Client ID - -- `client_secret` (string) - Client secret/password - -- `client_cert_path` (string) - Certificate path for client auth - -- `client_jwt` (string) - JWT bearer token for client auth (RFC 7523, Sec. 2.2) - -- `object_id` (string) - Object ID -- `tenant_id` (string) - The account identifier with which your client_id and - subscription_id are associated. If not specified, tenant_id will be - looked up using subscription_id. - -- `subscription_id` (string) - Subscription ID \ No newline at end of file diff --git a/website/source/partials/builder/azure/arm/_ClientConfig.html.md b/website/source/partials/builder/azure/arm/_ClientConfig.html.md deleted file mode 100644 index 89ed1f25d..000000000 --- a/website/source/partials/builder/azure/arm/_ClientConfig.html.md +++ /dev/null @@ -1,2 +0,0 @@ - -ClientConfig allows for various ways to authenticate Azure clients diff --git a/website/source/partials/builder/azure/common/client/_Config-not-required.html.md b/website/source/partials/builder/azure/common/client/_Config-not-required.html.md new file mode 100644 index 000000000..1ba957dd5 --- /dev/null +++ b/website/source/partials/builder/azure/common/client/_Config-not-required.html.md @@ -0,0 +1,25 @@ + + +- `cloud_environment_name` (string) - One of Public, China, Germany, or + USGovernment. Defaults to Public. Long forms such as + USGovernmentCloud and AzureUSGovernmentCloud are also supported. + +- `client_id` (string) - The application ID of the AAD Service Principal. + Requires either `client_secret`, `client_cert_path` or `client_jwt` to be set as well. + +- `client_secret` (string) - A password/secret registered for the AAD SP. + +- `client_cert_path` (string) - The path to a certificate that will be used to authenticate as the specified AAD SP. + +- `client_jwt` (string) - A JWT bearer token for client auth (RFC 7523, Sec. 2.2) that will be used + to authenticate the AAD SP. Provides more control over token the expiration + when using certificate authentication than when using `client_cert_path`. + +- `object_id` (string) - The object ID for the AAD SP. Optional, will be derived from the oAuth token if left empty. + +- `tenant_id` (string) - The Active Directory tenant identifier with which your `client_id` and + `subscription_id` are associated. If not specified, `tenant_id` will be + looked up using `subscription_id`. + +- `subscription_id` (string) - The subscription to use. + \ No newline at end of file diff --git a/website/source/partials/builder/azure/common/client/_Config.html.md b/website/source/partials/builder/azure/common/client/_Config.html.md new file mode 100644 index 000000000..086f9ceb1 --- /dev/null +++ b/website/source/partials/builder/azure/common/client/_Config.html.md @@ -0,0 +1,9 @@ + +Config allows for various ways to authenticate Azure clients. +When `client_id` and `subscription_id` are specified, Packer will use the +specified Azure Active Directoty (AAD) Service Principal (SP). +If only `subscription_id` is specified, Packer will try to interactively +log on the current user (tokens will be cached). +If none of these options are specified, Packer will attempt to use the +Managed Identity and subscription of the VM that Packer is running on. +This will only work if Packer is running on an Azure VM. From 094a8840d840c66452300909d9e6fee77bec4d6f Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Wed, 25 Sep 2019 17:16:17 +0000 Subject: [PATCH 2/3] Improve comments for FillParameters --- builder/azure/arm/builder.go | 2 +- builder/azure/common/client/config.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/builder/azure/arm/builder.go b/builder/azure/arm/builder.go index 6fbb611d1..27bc32a4a 100644 --- a/builder/azure/arm/builder.go +++ b/builder/azure/arm/builder.go @@ -57,7 +57,7 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack ctx, cancel := context.WithCancel(ctx) defer cancel() - // FillParameters function will set the authType from supplied parameters set the subscription and tenant id. + // FillParameters function captures authType and sets defaults. err := b.config.ClientConfig.FillParameters() if err != nil { return nil, err diff --git a/builder/azure/common/client/config.go b/builder/azure/common/client/config.go index 5cdb0e8c0..96ca8c651 100644 --- a/builder/azure/common/client/config.go +++ b/builder/azure/common/client/config.go @@ -254,6 +254,8 @@ func (c Config) GetServicePrincipalTokens( return servicePrincipalToken, servicePrincipalTokenVault, nil } +// FillParameters capture the user intent from the supplied parameter set in authType, retrieves the TenantID and CloudEnvironment if not specified. +// The SubscriptionID is also retrieved in case MSI auth is requested. func (c *Config) FillParameters() error { if c.authType == "" { if c.useDeviceLogin() { From c72a612b44203c39dee4ed7fbf1082f289a803af Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Wed, 25 Sep 2019 17:30:58 +0000 Subject: [PATCH 3/3] Re-added tests for FillParamters --- builder/azure/common/client/config.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/builder/azure/common/client/config.go b/builder/azure/common/client/config.go index 96ca8c651..9a4a736e8 100644 --- a/builder/azure/common/client/config.go +++ b/builder/azure/common/client/config.go @@ -280,14 +280,6 @@ func (c *Config) FillParameters() error { c.SubscriptionID = subscriptionID } - if c.TenantID == "" { - tenantID, err := common.FindTenantID(*c.CloudEnvironment, c.SubscriptionID) - if err != nil { - return err - } - c.TenantID = tenantID - } - if c.CloudEnvironment == nil { err := c.setCloudEnvironment() if err != nil { @@ -295,6 +287,14 @@ func (c *Config) FillParameters() error { } } + if c.TenantID == "" { + tenantID, err := findTenantID(*c.CloudEnvironment, c.SubscriptionID) + if err != nil { + return err + } + c.TenantID = tenantID + } + return nil }