review feedback

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, fmt.Errorf("could not determined the ObjectID for the user, which is required for Windows builds")
-		return nil, err
 	}
 
 	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
 }

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.
 //

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))
-
+		return spt, nil
-		// 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
-		}
 	}
 
 	// 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.