From ea9b2a8b5fdef856da8fefa4d2a591ba9928ed09 Mon Sep 17 00:00:00 2001 From: Hariharan Jayaraman Date: Fri, 18 May 2018 12:09:42 -0700 Subject: [PATCH] review feedback --- builder/azure/arm/builder.go | 26 +++++++++----- builder/azure/arm/builder_acc_test.go | 2 +- builder/azure/common/devicelogin.go | 50 +++------------------------ 3 files changed, 23 insertions(+), 55 deletions(-) diff --git a/builder/azure/arm/builder.go b/builder/azure/arm/builder.go index 1c7badb8d..7eb9e47f3 100644 --- a/builder/azure/arm/builder.go +++ b/builder/azure/arm/builder.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - packerAzureCommon "github.com/hashicorp/packer/builder/azure/common" "log" "os" "runtime" @@ -15,6 +14,7 @@ import ( "github.com/Azure/azure-sdk-for-go/storage" "github.com/Azure/go-autorest/autorest/adal" "github.com/dgrijalva/jwt-go" + packerAzureCommon "github.com/hashicorp/packer/builder/azure/common" "github.com/hashicorp/packer/builder/azure/common/constants" "github.com/hashicorp/packer/builder/azure/common/lin" packerCommon "github.com/hashicorp/packer/common" @@ -53,9 +53,6 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, error) { func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packer.Artifact, error) { - claims := jwt.MapClaims{} - var p jwt.Parser - ui.Say("Running builder ...") ctx, cancel := context.WithCancel(context.Background()) @@ -95,6 +92,9 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe return nil, err } + claims := jwt.MapClaims{} + var p jwt.Parser + _, _, err = p.ParseUnverified(spnCloud.OAuthToken(), claims) if err != nil { @@ -103,8 +103,7 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe b.config.ObjectID = claims["oid"].(string) if b.config.ObjectID == "" && b.config.OSType != constants.Target_Linux { - ui.Error("\n Got empty Object ID in the OAuth token , we need this for Key vault Access, bailing") - return nil, err + return nil, fmt.Errorf("could not determined the ObjectID for the user, which is required for Windows builds") } if b.config.isManagedImage() { @@ -403,17 +402,26 @@ func (b *Builder) getServicePrincipalTokens(say func(string)) (*adal.ServicePrin if err != nil { return nil, nil, err } - servicePrincipalToken.EnsureFresh() servicePrincipalTokenVault, err = auth.getServicePrincipalTokenWithResource( strings.TrimRight(b.config.cloudEnvironment.KeyVaultEndpoint, "/")) - if err != nil { return nil, nil, err } - servicePrincipalTokenVault.EnsureFresh() } + err = servicePrincipalToken.EnsureFresh() + + if err != nil { + return nil, nil, err + } + + err = servicePrincipalTokenVault.EnsureFresh() + + if err != nil { + return nil, nil, err + } + return servicePrincipalToken, servicePrincipalTokenVault, nil } diff --git a/builder/azure/arm/builder_acc_test.go b/builder/azure/arm/builder_acc_test.go index 056ee6e0b..3b03025d4 100644 --- a/builder/azure/arm/builder_acc_test.go +++ b/builder/azure/arm/builder_acc_test.go @@ -10,7 +10,7 @@ package arm // * ARM_STORAGE_ACCOUNT // // The subscription in question should have a resource group -// called "packer-acceptance-test" in "West US" region. The +// called "packer-acceptance-test" in "South Central US" region. The // storage account refered to in the above variable should // be inside this resource group and in "South Central US" as well. // diff --git a/builder/azure/common/devicelogin.go b/builder/azure/common/devicelogin.go index ad4a2ce36..8d053f802 100644 --- a/builder/azure/common/devicelogin.go +++ b/builder/azure/common/devicelogin.go @@ -41,10 +41,9 @@ var ( // Authenticate fetches a token from the local file cache or initiates a consent // flow and waits for token to be obtained. -func Authenticate(env azure.Environment, tenantID string, say func(string), apiScope string) (*adal.ServicePrincipalToken, error) { +func Authenticate(env azure.Environment, tenantID string, say func(string), scope string) (*adal.ServicePrincipalToken, error) { clientID, ok := clientIDs[env.Name] var resourceid string - var endpoint string if !ok { return nil, fmt.Errorf("packer-azure application not set up for Azure environment %q", env.Name) @@ -57,14 +56,11 @@ func Authenticate(env azure.Environment, tenantID string, say func(string), apiS // for AzurePublicCloud (https://management.core.windows.net/), this old // Service Management scope covers both ASM and ARM. - //apiScope := env.ServiceManagementEndpoint - if strings.Contains(apiScope, "vault") { + if strings.Contains(scope, "vault") { resourceid = "vault" - endpoint = env.KeyVaultEndpoint } else { resourceid = "mgmt" - endpoint = env.ResourceManagerEndpoint } tokenPath := tokenCachePath(tenantID + resourceid) @@ -75,41 +71,18 @@ func Authenticate(env azure.Environment, tenantID string, say func(string), apiS } // Lookup the token cache file for an existing token. - spt, err := tokenFromFile(say, *oauthCfg, tokenPath, clientID, apiScope, saveTokenCallback) + spt, err := tokenFromFile(say, *oauthCfg, tokenPath, clientID, scope, saveTokenCallback) if err != nil { return nil, err } if spt != nil { say(fmt.Sprintf("Auth token found in file: %s", tokenPath)) - - // NOTE(ahmetalpbalkan): The token file we found may contain an - // expired access_token. In that case, the first call to Azure SDK will - // attempt to refresh the token using refresh_token, which might have - // expired[1], in that case we will get an error and we shall remove the - // token file and initiate token flow again so that the user would not - // need removing the token cache file manually. - // - // [1]: expiration date of refresh_token is not returned in AAD /token - // response, we just know it is 14 days. Therefore user’s token - // will go stale every 14 days and we will delete the token file, - // re-initiate the device flow. - say("Validating the token.") - if err = validateToken(endpoint, spt); err != nil { - say(fmt.Sprintf("Error: %v", err)) - say("Stored Azure credentials expired. Please reauthenticate.") - say(fmt.Sprintf("Deleting %s", tokenPath)) - if err := os.RemoveAll(tokenPath); err != nil { - return nil, fmt.Errorf("Error deleting stale token file: %v", err) - } - } else { - say("Token works.") - return spt, nil - } + return spt, nil } // Start an OAuth 2.0 device flow say(fmt.Sprintf("Initiating device flow: %s", tokenPath)) - spt, err = tokenFromDeviceFlow(say, *oauthCfg, clientID, apiScope) + spt, err = tokenFromDeviceFlow(say, *oauthCfg, clientID, scope) if err != nil { return nil, err } @@ -195,19 +168,6 @@ func mkTokenCallback(path string) adal.TokenRefreshCallback { } } -// validateToken makes a call to Azure SDK with given token, essentially making -// sure if the access_token valid, if not it uses SDK’s functionality to -// automatically refresh the token using refresh_token (which might have -// expired). This check is essentially to make sure refresh_token is good. -func validateToken(env string, token *adal.ServicePrincipalToken) error { - err := token.EnsureFresh() - - if err != nil { - return fmt.Errorf("%s token validity check failed: %v", env, err) - } - return nil -} - // FindTenantID figures out the AAD tenant ID of the subscription by making an // unauthenticated request to the Get Subscription Details endpoint and parses // the value from WWW-Authenticate header.