Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow authentication via managed service identity #639

Merged
merged 12 commits into from
Mar 1, 2018

Conversation

hbuckle
Copy link
Contributor

@hbuckle hbuckle commented Dec 18, 2017

Adds a provider option to allow authentication using managed service identity
e.g.

provider "azurerm" {
  subscription_id = "xxx"
  tenant_id       = "xxx"
  use_msi         = true
}

@tombuildsstuff tombuildsstuff added this to the 1.0.2 milestone Jan 10, 2018
@tombuildsstuff tombuildsstuff modified the milestones: 1.0.2, 1.0.3 Jan 24, 2018
paultyng
paultyng previously approved these changes Jan 26, 2018
@paultyng
Copy link
Contributor

paultyng commented Jan 26, 2018

@hbuckle Code wise this looks good to me. I have no experience with MSI though. We have some additional authentication changes coming targeted at 1.0.3 so grouping this up with those (see the other issues under the authentication label).

@paultyng
Copy link
Contributor

It looks like this has a minor merge conflict, could maybe use a rebase.

@VaijanathB
Copy link
Contributor

@hbuckle ,

Am I understanding correctly that this only supports MSI with ServicePrincipal and does not do it with azure cli ? Also I pulled your branch and tried to verify using the construct specified

provider "azurerm" {
subscription_id = "xxx"
tenant_id = "xxx"
use_msi = true
}
and was not able to authenticate. Can you please elaborate how to do it ?

Thanks,
VJ

@hbuckle
Copy link
Contributor Author

hbuckle commented Jan 27, 2018

@VaijanathB MSI works by creating a service principal to act on behalf of an Azure resource (such as a VM). For it to work you need to be running Terraform from a VM in Azure that has MSI configured - see authenticating_via_msi.html.markdown for details

SkipCredentialsValidation: d.Get("skip_credentials_validation").(bool),
SkipProviderRegistration: d.Get("skip_provider_registration").(bool),
}

if config.ClientSecret != "" {
if config.UseMsi {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we use the same if-else logic as in the azurerm/config.go ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean from getAuthorizationToken ? I think the logic is slightly different in each

Copy link
Contributor

@metacpp metacpp Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I mean why we don't check if ClientSecret exists firstly.

Managed service identity can also be configured using Terraform. The following template shows how. Note that for a Linux VM you must use the ManagedIdentityExtensionForLinux.

```hcl
resource "azurerm_virtual_machine" "virtual_machine" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put a Linux example ? This example creates WindowsServer and the documentation above says about Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted, for Linux it would be exactly the same except you use ManagedIdentityExtensionForLinux instead of ManagedIdentityExtensionForWindows extension

@tombuildsstuff tombuildsstuff modified the milestones: 1.1.1, 1.1.2 Feb 5, 2018
"msi_endpoint": {
Type: schema.TypeString,
Optional: true,
DefaultFunc: schema.EnvDefaultFunc("ARM_MSI_ENDPOINT", ""),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the default environment variable to "AZURE_MSI_ENDPOINT", coz this endpoint is not just for getting tokens for ARM, but also for graph/datalake/keyvault.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same here) - given the prefix for the other Environment Variables is ARM - we'd be best to leave this as ARM_MSI_ENDPOINT rather than making this AZURE_MSI_ENDPOINT - in future we can migrate across to use AZURE as a prefix if needed)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be defaulted to http://localhost:50342/oauth2/token?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tombuildsstuff by leaving it blank then the endpoint should be auto-discovered using GetMSIVMEndpoint(). I only put the option there in case the endpoint can't be discovered (such as when running in a container)

"use_msi": {
Type: schema.TypeBool,
Optional: true,
DefaultFunc: schema.EnvDefaultFunc("ARM_USE_MSI", false),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to "USE_MSI".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given all the other Environment Variables are prefixed with ARM - we should leave this as ARM_USE_MSI for consistency (we may change this for AZURE_USE_MSI in future, but it should be consistent for now)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

@tombuildsstuff tombuildsstuff self-assigned this Feb 6, 2018
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @hbuckle

Thanks for this PR - apologies for the delayed review here!

I've taken a look through and left some comments in-line but this mostly looks good to me - if we can fix those minor issues this should be good to merge, as such I'll go ahead and test this PR in the meantime :)

Thanks!

"msi_endpoint": {
Type: schema.TypeString,
Optional: true,
DefaultFunc: schema.EnvDefaultFunc("ARM_MSI_ENDPOINT", ""),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be defaulted to http://localhost:50342/oauth2/token?


Terraform supports authenticating to Azure through Managed Service Identity, Service Principal or the Azure CLI.

We recommend using Managed Service Identity when running in a Shared Environment (such as within a CI server/automation) that you do not wish to configure credentials for - and [authenticating via the Azure CLI](authenticating_via_azure_cli.html) when you're running Terraform locally. Note that managed service identity is only available for virtual machines within Azure.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest we change this to:

Managed Service Identity can be used to access other Azure Services from within a Virtual Machine in Azure instead of specifying a Service Principal or Azure CLI credentials.


# Grant the VM identity contributor rights to the current subscription
resource "azurerm_role_assignment" "role_assignment" {
name = "${uuid()}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this field is optional - so we should be able to not specify it and it'll be populated automatically?


### Configuring Managed Service Identity using Terraform

Managed service identity can also be configured using Terraform. The following template shows how. Note that for a Linux VM you must use the ManagedIdentityExtensionForLinux.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this "ManagedIdentityExtensionForLinux extension"


Managed Service Identity allows an Azure virtual machine to retrieve a token to access the Azure API without needing to pass in credentials. This works by creating a service principal in Azure Active Directory that is associated to a virtual machine. This service principal can then be granted permissions to Azure resources.
There are various ways to configure managed service identity - see the [Microsoft documentation](https://docs.microsoft.com/en-us/azure/active-directory/msi-overview) for details.
You can then run Terraform from the MSI enabled virtual machine by setting the use_msi provider option to true.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we quote use_msi and true here?

@metacpp
Copy link
Contributor

metacpp commented Feb 9, 2018

@hbuckle any update ?

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Feb 14, 2018

Results of testing this so far:

  • ❌ issues in CloudShell
  • ✅ MSI on a Windows VM
  • ✅ MSI on a Linux VM

Once I've confirmed this works on Linux
I'm looking into why this fails in CloudShell (since MSI is now available there too)

@tombuildsstuff
Copy link
Contributor

hey @hbuckle

Just to give an update here - I've been working through testing this, and this looks good on the VM's (both Linux and Windows), however there's an issue when running in CloudShell (where MSI's recently been enabled). In CloudShell the API doesn't return the same schema as the API available when running in a VM (an integer value is returned, rather than a string) for the expiresOn field, which means deserialization fails.

Since MSI is enabled by default in CloudShell - I've reached out to Microsoft to see how if they can patch the API to return the correct data type - if that's not going to be possible for a while the other option would be to error when running in CloudShell with MSI enabled, until we've confirmed the API's been patched (which would allow us to ship this for use in VM's).

Apologies for the delay, but bear with us here - I'm hoping we'll have an answer about this later this week :)

Thanks!

@tombuildsstuff tombuildsstuff added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Feb 14, 2018
@tombuildsstuff tombuildsstuff modified the milestones: 1.1.2, 1.1.3 Feb 15, 2018
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @hbuckle

Apologies for the delay getting this merged - we've been working with Microsoft to fix the broken MSI API. That's now been deployed - and I can confirm this looks good in our testing.

Thanks for pushing those updates to this PR - this now LGTM 👍

Thanks!

@tombuildsstuff tombuildsstuff removed the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Mar 1, 2018
@tombuildsstuff tombuildsstuff merged commit d7e6931 into hashicorp:master Mar 1, 2018
tombuildsstuff added a commit that referenced this pull request Mar 1, 2018
also restructuring the changelog to match the releases better
@tombuildsstuff
Copy link
Contributor

👋 hey @hbuckle

Just to let you know that support for MSI has just been released in v1.2.0 of the AzureRM Provider - full details of what's included are available here: https://github.com/terraform-providers/terraform-provider-azurerm/blob/v1.2.0/CHANGELOG.md#120-march-02-2018

Thanks!

@ghost
Copy link

ghost commented Mar 31, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants