From e6368b9246a69e07a57c33cd7411c86759df8b7b Mon Sep 17 00:00:00 2001 From: Sylvia Moss Date: Tue, 24 Mar 2020 14:43:24 +0100 Subject: [PATCH] Fix azure winrm_password attribution and allow to set winrm_username (#8928) --- builder/azure/arm/builder.go | 2 +- builder/azure/arm/config.go | 58 ++++++++++++++++--- builder/azure/arm/config_test.go | 99 ++++++++++++++++++++++++++++---- common/random/string.go | 7 ++- 4 files changed, 143 insertions(+), 23 deletions(-) diff --git a/builder/azure/arm/builder.go b/builder/azure/arm/builder.go index 54cb8eee2..7d1c1abfe 100644 --- a/builder/azure/arm/builder.go +++ b/builder/azure/arm/builder.go @@ -254,7 +254,7 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack WinRMConfig: func(multistep.StateBag) (*communicator.WinRMConfig, error) { return &communicator.WinRMConfig{ Username: b.config.UserName, - Password: b.config.Comm.WinRMPassword, + Password: b.config.Password, }, nil }, }, diff --git a/builder/azure/arm/config.go b/builder/azure/arm/config.go index d87ff3426..103ebb337 100644 --- a/builder/azure/arm/config.go +++ b/builder/azure/arm/config.go @@ -18,6 +18,8 @@ import ( "strings" "time" + "github.com/hashicorp/packer/common/random" + "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-04-01/compute" "github.com/Azure/go-autorest/autorest/to" "github.com/masterzen/winrm" @@ -528,7 +530,10 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) { provideDefaultValues(c) setRuntimeValues(c) - setUserNamePassword(c) + err = setUserNamePassword(c) + if err != nil { + return nil, err + } // copy singular blocks for _, kv := range c.AzureTag { @@ -651,23 +656,34 @@ func setRuntimeValues(c *Config) { c.tmpKeyVaultName = tempName.KeyVaultName } -func setUserNamePassword(c *Config) { +func setUserNamePassword(c *Config) error { + // SSH comm if c.Comm.SSHUsername == "" { c.Comm.SSHUsername = DefaultUserName } - c.UserName = c.Comm.SSHUsername if c.Comm.SSHPassword != "" { c.Password = c.Comm.SSHPassword - return + return nil + } + + // WinRM comm + if c.Comm.WinRMUser == "" { + c.Comm.WinRMUser = DefaultUserName } + c.UserName = c.Comm.WinRMUser - // Configure password settings using Azure generated credentials - c.Password = c.tmpAdminPassword if c.Comm.WinRMPassword == "" { - c.Comm.WinRMPassword = c.Password + // Configure password settings using Azure generated credentials + c.Comm.WinRMPassword = c.tmpAdminPassword } + if !isValidPassword(c.Comm.WinRMPassword) { + return fmt.Errorf("The supplied \"winrm_password\" must be between 8-123 characters long and must satisfy at least 3 from the following: \n1) Contains an uppercase character \n2) Contains a lowercase character\n3) Contains a numeric digit\n4) Contains a special character\n5) Control characters are not allowed") + } + c.Password = c.Comm.WinRMPassword + + return nil } func setCustomData(c *Config) error { @@ -1037,6 +1053,34 @@ func isValidAzureName(re *regexp.Regexp, rgn string) bool { !strings.HasSuffix(rgn, "-") } +// The supplied password must be between 8-123 characters long and must satisfy at least 3 of password complexity requirements from the following: +// 1) Contains an uppercase character +// 2) Contains a lowercase character +// 3) Contains a numeric digit +// 4) Contains a special character +// 5) Control characters are not allowed (a very specific case - not included in this validation) +func isValidPassword(password string) bool { + if !(len(password) >= 8 && len(password) <= 123) { + return false + } + + requirements := 0 + if strings.ContainsAny(password, random.PossibleNumbers) { + requirements++ + } + if strings.ContainsAny(password, random.PossibleLowerCase) { + requirements++ + } + if strings.ContainsAny(password, random.PossibleUpperCase) { + requirements++ + } + if strings.ContainsAny(password, random.PossibleSpecialCharacter) { + requirements++ + } + + return requirements >= 3 +} + func (c *Config) validateLocationZoneResiliency(say func(s string)) { // Docs on regions that support Availibility Zones: // https://docs.microsoft.com/en-us/azure/availability-zones/az-overview#regions-that-support-availability-zones diff --git a/builder/azure/arm/config_test.go b/builder/azure/arm/config_test.go index 128df2be3..afeae9f3e 100644 --- a/builder/azure/arm/config_test.go +++ b/builder/azure/arm/config_test.go @@ -71,32 +71,53 @@ func TestConfigShouldBeAbleToOverrideDefaultedValues(t *testing.T) { t.Fatalf("newConfig failed: %s", err) } + if c.VMSize != "override_vm_size" { + t.Errorf("Expected 'vm_size' to be set to 'override_vm_size', but found %q!", c.VMSize) + } + + if c.managedImageStorageAccountType != compute.StorageAccountTypesPremiumLRS { + t.Errorf("Expected 'managed_image_storage_account_type' to be set to 'Premium_LRS', but found %q!", c.managedImageStorageAccountType) + } + + if c.diskCachingType != compute.CachingTypesNone { + t.Errorf("Expected 'disk_caching_type' to be set to 'None', but found %q!", c.diskCachingType) + } + + // SSH comm if c.Password != "override_password" { t.Errorf("Expected 'Password' to be set to 'override_password', but found %q!", c.Password) } - if c.Comm.SSHPassword != "override_password" { t.Errorf("Expected 'c.Comm.SSHPassword' to be set to 'override_password', but found %q!", c.Comm.SSHPassword) } - if c.UserName != "override_username" { t.Errorf("Expected 'UserName' to be set to 'override_username', but found %q!", c.UserName) } - if c.Comm.SSHUsername != "override_username" { t.Errorf("Expected 'c.Comm.SSHUsername' to be set to 'override_username', but found %q!", c.Comm.SSHUsername) } - if c.VMSize != "override_vm_size" { - t.Errorf("Expected 'vm_size' to be set to 'override_vm_size', but found %q!", c.VMSize) + // Winrm comm + c = Config{} + builderValues = getArmBuilderConfiguration() + builderValues["communicator"] = "winrm" + builderValues["winrm_password"] = "Override_winrm_password1" + builderValues["winrm_username"] = "override_winrm_username" + _, err = c.Prepare(builderValues, getPackerConfiguration()) + if err != nil { + t.Fatalf("newConfig failed: %s", err) } - - if c.managedImageStorageAccountType != compute.StorageAccountTypesPremiumLRS { - t.Errorf("Expected 'managed_image_storage_account_type' to be set to 'Premium_LRS', but found %q!", c.managedImageStorageAccountType) + if c.Password != "Override_winrm_password1" { + t.Errorf("Expected 'Password' to be set to 'Override_winrm_password1', but found %q!", c.Password) } - - if c.diskCachingType != compute.CachingTypesNone { - t.Errorf("Expected 'disk_caching_type' to be set to 'None', but found %q!", c.diskCachingType) + if c.Comm.WinRMPassword != "Override_winrm_password1" { + t.Errorf("Expected 'c.Comm.WinRMPassword' to be set to 'Override_winrm_password1', but found %q!", c.Comm.SSHPassword) + } + if c.UserName != "override_winrm_username" { + t.Errorf("Expected 'UserName' to be set to 'override_winrm_username', but found %q!", c.UserName) + } + if c.Comm.WinRMUser != "override_winrm_username" { + t.Errorf("Expected 'c.Comm.WinRMUser' to be set to 'override_winrm_username', but found %q!", c.Comm.SSHUsername) } } @@ -574,7 +595,7 @@ func TestWinRMConfigShouldSetRoundTripDecorator(t *testing.T) { config := getArmBuilderConfiguration() config["communicator"] = "winrm" config["winrm_username"] = "username" - config["winrm_password"] = "password" + config["winrm_password"] = "Password123" var c Config _, err := c.Prepare(config, getPackerConfiguration()) @@ -1920,6 +1941,60 @@ func Test_GivenZoneSupportingResiliency_ConfigValidate_ShouldNotWarn(t *testing. } } +func TestConfig_PrepareProvidedWinRMPassword(t *testing.T) { + config := getArmBuilderConfiguration() + config["communicator"] = "winrm" + + var c Config + tc := []struct { + name string + password string + shouldFail bool + }{ + { + name: "password should be longer than 8 characters", + password: "packer", + shouldFail: true, + }, + { + name: "password should be shorter than 123 characters", + password: "1Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + shouldFail: true, + }, + { + name: "password should have valid size but only lower and upper case letters", + password: "AAAbbbCCC", + shouldFail: true, + }, + { + name: "password should have valid size but only digits and upper case letters", + password: "AAA12345", + shouldFail: true, + }, + { + name: "password should have valid size, digits, upper and lower case letters", + password: "AAA12345bbb", + shouldFail: false, + }, + { + name: "password should have valid size, digits, special characters and lower case letters", + password: "//12345bbb", + shouldFail: false, + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + config["winrm_password"] = tt.password + _, err := c.Prepare(config) + fail := err != nil + if tt.shouldFail != fail { + t.Fatalf("bad: %s. Expected fail is: %t but it was %t", tt.name, tt.shouldFail, fail) + } + }) + } +} + func getArmBuilderConfiguration() map[string]string { m := make(map[string]string) for _, v := range requiredConfigValues { diff --git a/common/random/string.go b/common/random/string.go index 74c79bc90..17926c401 100644 --- a/common/random/string.go +++ b/common/random/string.go @@ -7,9 +7,10 @@ import ( ) var ( - PossibleNumbers = "0123456789" - PossibleLowerCase = "abcdefghijklmnopqrstuvwxyz" - PossibleUpperCase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + PossibleNumbers = "0123456789" + PossibleLowerCase = "abcdefghijklmnopqrstuvwxyz" + PossibleUpperCase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + PossibleSpecialCharacter = " !\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" PossibleAlphaNum = PossibleNumbers + PossibleLowerCase + PossibleUpperCase PossibleAlphaNumLower = PossibleNumbers + PossibleLowerCase