refactor: feature-toggled authentication methods #2199

tombuildsstuff · 2018-10-31T23:38:56Z

This PR refactors the authentication package such that it's capable of supporting multiple authentication methods which may be feature toggled on or off. This is the second step in refactoring the authentication package out into it's own library such that it can be consumed in Terraform Core(for the AzureRM Backend), the Azure Stack Provider and this Provider.

Due to the nature of the fact this is authentication related; this may require some additional unit testing and some manual UX testing to ensure we're supporting all the known scenarios:

Azure CLI
- Parsing the ClientID, Subscription ID and Tenant ID when nothings specified
- Parsing the Tenant ID when the Subscription ID is specified
- Parsing the Client ID when the Subscription ID & Tenant ID are specified

CloudShell
- Parsing the ClientID, Subscription ID and Tenant ID when nothings specified
- Parsing the Tenant ID when the Subscription Id is specified
- Parsing the Client ID when the Subscription ID & Tenant ID are specified

Managed Service Identity
Service Principal Client Certificate (this is intentionally feature-toggled off)
Service Principal Client Secret

tombuildsstuff · 2018-11-01T01:08:44Z

Acceptance Tests still pass - so that works too 🙌

 $ acctests azurerm TestAccAzureRMResourceGroup_withTags
=== RUN   TestAccAzureRMResourceGroup_withTags
--- PASS: TestAccAzureRMResourceGroup_withTags (88.29s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	90.068s

tombuildsstuff

some comments on the first skim-thru

azurerm/helpers/authentication/auth_method_msi_test.go

azurerm/helpers/authentication/builder.go

azurerm/helpers/authentication/config.go

azurerm/helpers/authentication/builder.go

azurerm/helpers/authentication/config.go

azurerm/helpers/authentication/auth_method.go

paultyng · 2018-11-02T20:38:53Z

azurerm/helpers/authentication/auth_method_client_secret.go

+	if a.tenantId == "" {
+		err = multierror.Append(err, fmt.Errorf(fmtErrorMessage, "Tenant ID"))
+	}
+	if a.environment == "" {


seems like some of these fields aren't used, do they have to be required?

katbyte

Aside from a couple comments LGTM 👍

azurerm/helpers/authentication/auth_method.go

azurerm/helpers/authentication/auth_method_client_cert.go

azurerm/helpers/authentication/auth_method_client_cert_test.go

…ovider

feature toggling support for service principal client secret

making a bunch of the methods private tests pass: ``` $ go test -v ./azurerm/helpers/authentication/ === RUN TestAzureCLIParsingAuth_validate --- PASS: TestAzureCLIParsingAuth_validate (0.00s) === RUN TestServicePrincipalClientSecretAuth_builder --- PASS: TestServicePrincipalClientSecretAuth_builder (0.00s) === RUN TestServicePrincipalClientSecretAuth_validate --- PASS: TestServicePrincipalClientSecretAuth_validate (0.00s) === RUN TestManagedServiceIdentity_builder 2018/10/31 23:24:40 [DEBUG] Using MSI endpoint "https://hello-world" --- PASS: TestManagedServiceIdentity_builder (0.00s) === RUN TestManagedServiceIdentity_validate --- PASS: TestManagedServiceIdentity_validate (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_InvalidDate --- PASS: TestAzureFindValidAccessTokenForTenant_InvalidDate (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_Expired 2018/10/31 23:24:40 [DEBUG] Token "7cabcf30-8dca-43f9-91e6-fd56dfb8632f" has expired --- PASS: TestAzureFindValidAccessTokenForTenant_Expired (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ExpiringIn --- PASS: TestAzureFindValidAccessTokenForTenant_ExpiringIn (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_InvalidManagementDomain 2018/10/31 23:24:40 [DEBUG] Resource "https://portal.azure.com/" isn't a management domain --- PASS: TestAzureFindValidAccessTokenForTenant_InvalidManagementDomain (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_DifferentTenant 2018/10/31 23:24:40 [DEBUG] Resource "https://management.core.windows.net/" isn't for the correct Tenant --- PASS: TestAzureFindValidAccessTokenForTenant_DifferentTenant (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ValidFromCloudShell --- PASS: TestAzureFindValidAccessTokenForTenant_ValidFromCloudShell (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ValidFromAzureCLI --- PASS: TestAzureFindValidAccessTokenForTenant_ValidFromAzureCLI (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_NoTokens --- PASS: TestAzureFindValidAccessTokenForTenant_NoTokens (0.00s) === RUN TestAzureCliProfile_populateSubscriptionIdMissing --- PASS: TestAzureCliProfile_populateSubscriptionIdMissing (0.00s) === RUN TestAzureCliProfile_populateSubscriptionIdNoDefault --- PASS: TestAzureCliProfile_populateSubscriptionIdNoDefault (0.00s) === RUN TestAzureCliProfile_populateSubscriptionIdValid --- PASS: TestAzureCliProfile_populateSubscriptionIdValid (0.00s) === RUN TestAzureCliProfile_populateTenantIdEmpty --- PASS: TestAzureCliProfile_populateTenantIdEmpty (0.00s) === RUN TestAzureCliProfile_populateTenantIdMissingSubscription --- PASS: TestAzureCliProfile_populateTenantIdMissingSubscription (0.00s) === RUN TestAzureCliProfile_populateTenantIdValid --- PASS: TestAzureCliProfile_populateTenantIdValid (0.00s) === RUN TestAzureCLIProfileFindDefaultSubscription --- PASS: TestAzureCLIProfileFindDefaultSubscription (0.00s) === RUN TestAzureCLIProfileFindSubscription --- PASS: TestAzureCLIProfileFindSubscription (0.00s) === RUN TestAzureEnvironmentNames --- PASS: TestAzureEnvironmentNames (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/authentication 0.791s ```

… package

NOTE: this is /intentionally/ feature-toggled off for now

Tests pass: ``` $ go test -v ./azurerm/helpers/authentication/ === RUN TestAzureCLIParsingAuth_isApplicable --- PASS: TestAzureCLIParsingAuth_isApplicable (0.00s) === RUN TestAzureCLIParsingAuth_populateConfig --- PASS: TestAzureCLIParsingAuth_populateConfig (0.00s) === RUN TestAzureCLIParsingAuth_validate --- PASS: TestAzureCLIParsingAuth_validate (0.00s) === RUN TestServicePrincipalClientCertAuth_builder --- PASS: TestServicePrincipalClientCertAuth_builder (0.00s) === RUN TestServicePrincipalClientCertAuth_isApplicable --- PASS: TestServicePrincipalClientCertAuth_isApplicable (0.00s) === RUN TestServicePrincipalClientCertAuth_populateConfig --- PASS: TestServicePrincipalClientCertAuth_populateConfig (0.00s) === RUN TestServicePrincipalClientCertAuth_validate --- PASS: TestServicePrincipalClientCertAuth_validate (0.00s) === RUN TestServicePrincipalClientSecretAuth_builder --- PASS: TestServicePrincipalClientSecretAuth_builder (0.00s) === RUN TestServicePrincipalClientSecretAuth_isApplicable --- PASS: TestServicePrincipalClientSecretAuth_isApplicable (0.00s) === RUN TestServicePrincipalClientSecretAuth_populateConfig --- PASS: TestServicePrincipalClientSecretAuth_populateConfig (0.00s) === RUN TestServicePrincipalClientSecretAuth_validate --- PASS: TestServicePrincipalClientSecretAuth_validate (0.00s) === RUN TestManagedServiceIdentity_builder 2018/11/06 14:18:37 [DEBUG] Using MSI endpoint "https://hello-world" --- PASS: TestManagedServiceIdentity_builder (0.00s) === RUN TestManagedServiceIdentity_isApplicable --- PASS: TestManagedServiceIdentity_isApplicable (0.00s) === RUN TestManagedServiceIdentity_populateConfig --- PASS: TestManagedServiceIdentity_populateConfig (0.00s) === RUN TestManagedServiceIdentity_validate --- PASS: TestManagedServiceIdentity_validate (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_InvalidDate --- PASS: TestAzureFindValidAccessTokenForTenant_InvalidDate (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_Expired 2018/11/06 14:18:37 [DEBUG] Token "7cabcf30-8dca-43f9-91e6-fd56dfb8632f" has expired --- PASS: TestAzureFindValidAccessTokenForTenant_Expired (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ExpiringIn --- PASS: TestAzureFindValidAccessTokenForTenant_ExpiringIn (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_InvalidManagementDomain 2018/11/06 14:18:37 [DEBUG] Resource "https://portal.azure.com/" isn't a management domain --- PASS: TestAzureFindValidAccessTokenForTenant_InvalidManagementDomain (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_DifferentTenant 2018/11/06 14:18:37 [DEBUG] Resource "https://management.core.windows.net/" isn't for the correct Tenant --- PASS: TestAzureFindValidAccessTokenForTenant_DifferentTenant (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ValidFromCloudShell --- PASS: TestAzureFindValidAccessTokenForTenant_ValidFromCloudShell (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_ValidFromAzureCLI --- PASS: TestAzureFindValidAccessTokenForTenant_ValidFromAzureCLI (0.00s) === RUN TestAzureFindValidAccessTokenForTenant_NoTokens --- PASS: TestAzureFindValidAccessTokenForTenant_NoTokens (0.00s) === RUN TestAzureCliProfile_populateSubscriptionIdMissing --- PASS: TestAzureCliProfile_populateSubscriptionIdMissing (0.00s) === RUN TestAzureCliProfile_populateSubscriptionIdNoDefault --- PASS: TestAzureCliProfile_populateSubscriptionIdNoDefault (0.00s) === RUN TestAzureCliProfile_populateSubscriptionIdValid --- PASS: TestAzureCliProfile_populateSubscriptionIdValid (0.00s) === RUN TestAzureCliProfile_populateTenantIdEmpty --- PASS: TestAzureCliProfile_populateTenantIdEmpty (0.00s) === RUN TestAzureCliProfile_populateTenantIdMissingSubscription --- PASS: TestAzureCliProfile_populateTenantIdMissingSubscription (0.00s) === RUN TestAzureCliProfile_populateTenantIdValid --- PASS: TestAzureCliProfile_populateTenantIdValid (0.00s) === RUN TestAzureCLIProfileFindDefaultSubscription --- PASS: TestAzureCLIProfileFindDefaultSubscription (0.00s) === RUN TestAzureCLIProfileFindSubscription --- PASS: TestAzureCLIProfileFindSubscription (0.00s) === RUN TestAzureEnvironmentNames --- PASS: TestAzureEnvironmentNames (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/authentication 0.851s ```

ghost · 2019-03-06T13:57:34Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

tombuildsstuff added enhancement authentication labels Oct 31, 2018

tombuildsstuff added this to the 1.19.0 milestone Oct 31, 2018

ghost added the size/XXL label Oct 31, 2018

tombuildsstuff mentioned this pull request Oct 31, 2018

Authenticating as a Service Principal using a Certificate #1573

Closed

tombuildsstuff requested review from bflad and katbyte November 1, 2018 01:07

tombuildsstuff commented Nov 1, 2018

View reviewed changes

tombuildsstuff force-pushed the refactor/auth branch 2 times, most recently from 5fc8593 to 925061f Compare November 2, 2018 19:23

paultyng reviewed Nov 2, 2018

View reviewed changes

azurerm/helpers/authentication/auth_method.go Show resolved Hide resolved

paultyng reviewed Nov 2, 2018

View reviewed changes

tombuildsstuff mentioned this pull request Nov 3, 2018

Linting: include sub packages #2219

Merged

katbyte approved these changes Nov 3, 2018

View reviewed changes

azurerm/helpers/authentication/auth_method.go Show resolved Hide resolved

azurerm/helpers/authentication/auth_method_client_cert.go Show resolved Hide resolved

azurerm/helpers/authentication/auth_method_client_cert_test.go Show resolved Hide resolved

tombuildsstuff added 15 commits November 6, 2018 12:55

refactor: moving the unused SkipCredentialsValidation field to the Pr…

b28fdc3

…ovider

refactor: adding a builder to parse out the config

ca623b6

feature toggling support for service principal client secret

refactor: feature-toggling support for msi

9537e8d

refactoring: feature-toggling azure cli parsing/cloudshell auth

36520aa

Removing the unused validate method

1d8ea24

Feature-Toggling on Azure CLI Parsing auth

6ad36ad

defining an interface for authentication methods

e45d2a9

New Authentication Method: Service Principal Client Secret

db9f9cd

removing the unused config from the GetAuthToken method

7b6e4d3

refactor: removing the unused 'accessToken' field

743cecc

refactor: removing SkipProviderRegistration from the authentication…

afafe5e

… package

Re-enabling Client Secret auth for the acceptance tests

0cc33d4

New authentication method: Service Principal Client Certificate

e12e5db

NOTE: this is /intentionally/ feature-toggled off for now

vendoring golang.org/x/crypto/pkcs12

64f9ce0

Code review fixes

60f30af

tombuildsstuff force-pushed the refactor/auth branch from 925061f to 60f30af Compare November 6, 2018 12:56

tombuildsstuff added 2 commits November 6, 2018 13:11

refactoring: making the method signatures more consistent

98e620a

tombuildsstuff merged commit d7974cf into master Nov 6, 2018

tombuildsstuff deleted the refactor/auth branch November 6, 2018 16:12

tombuildsstuff added a commit that referenced this pull request Nov 6, 2018

Updating to include #2199

eb47600

tombuildsstuff mentioned this pull request Nov 21, 2018

Update vendor of Azure/azure-sdk-for-go to v21.1.0 hashicorp/terraform#18968

Closed

ghost locked and limited conversation to collaborators Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: feature-toggled authentication methods #2199

refactor: feature-toggled authentication methods #2199

tombuildsstuff commented Oct 31, 2018 •

edited

Loading

tombuildsstuff commented Nov 1, 2018

tombuildsstuff left a comment

paultyng Nov 2, 2018

katbyte left a comment

ghost commented Mar 6, 2019

refactor: feature-toggled authentication methods #2199

refactor: feature-toggled authentication methods #2199

Conversation

tombuildsstuff commented Oct 31, 2018 • edited Loading

tombuildsstuff commented Nov 1, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

paultyng Nov 2, 2018

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Mar 6, 2019

tombuildsstuff commented Oct 31, 2018 •

edited

Loading