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

Service Principal Authentication Support #745

Closed
davidcorrigan714 opened this issue Apr 3, 2023 · 32 comments · Fixed by #747
Closed

Service Principal Authentication Support #745

davidcorrigan714 opened this issue Apr 3, 2023 · 32 comments · Fixed by #747

Comments

@davidcorrigan714
Copy link
Contributor

davidcorrigan714 commented Apr 3, 2023

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

The provider should support authenticating as a Service Principal to Azure DevOps.

Potential Terraform Configuration

provider "azuredevops" {
  org_service_url = "https://dev.azure.com/my-org"
  sp_client_id    = "00000000-0000-0000-0000-000000000001"
  sp_tenant_id    = "00000000-0000-0000-0000-000000000001"

  sp_client_secret = "a client secret from AzureAD"

  # sp_client_secret_path = "C:\\client_secret.txt"

  # sp_oidc_token = "my long oidc token"
  # sp_oidc_token_path = "C:\\oidc_token.txt"
  # sp_oidc_github_actions = true
  # sp_oidc_github_actions_audience = "my-aud"
  # sp_oidc_hcp = true
  # sp_client_certificate_path = "C:\\cert_tes.pfx"
  # sp_client_certificate          = "base64 encoded cert"
  # sp_client_certificate_password = "cert password"
}

References

Use Azure AD service principals & managed identities

@davidcorrigan714
Copy link
Contributor Author

Working on getting this working, figured I'd start an issue on it though for visibility.

@davidcorrigan714
Copy link
Contributor Author

Well it all works well enough. Gotta figure out testing and write some docs. So far it works with GitHub OIDC tokens, Client Secrets, Client Certs, and HCP Cloud tokens.

@krottiers
Copy link

Looking forward to this integration as it will get rid of quite some issues regarding PAT token rotation.

@tmeckel
Copy link
Contributor

tmeckel commented Apr 5, 2023

@davidcorrigan714 the Go SDK for Azure DevOps only supports REST API <= 6.0 and thus there's no support for the required REST API for Service Principal Entitlements https://learn.microsoft.com/en-us/rest/api/azure/devops/memberentitlementmanagement/service-principal-entitlements?view=azure-devops-rest-7.1 and the support for authenticating via Service Principal Credentials, which is of course a part of the GO SDK for Azure DevOps

https://github.com/microsoft/azure-devops-go-api

image

Microsoft is neglecting the Azure DevOps GO SDK ever since and blocks the implementation of features for the Terraform provider. If you go through the issue list here you'll find a couple of requested features which are marked as blocked because of this.

@davidcorrigan714
Copy link
Contributor Author

davidcorrigan714 commented Apr 5, 2023

@tmeckel This won't require any of those API endpoints. It just uses the Azure Go SDK to authenticate and get an access token. Though we will need data sources for the service principals to assign them into the system. One of the identity APISs does seem to return them but should probably wait for those new service-principal specific ones to do it properly. For now you can just find the descriptor and put it in manually. I'll make an issue for the data source and tag that thread. Sounds like we may actually get an updated Go SDK soon which is good news.

@davidcorrigan714
Copy link
Contributor Author

Filed #746

@tmeckel
Copy link
Contributor

tmeckel commented Apr 5, 2023

@davidcorrigan714 Nice to hear that there will probably an updated version of the GO SDK, but I've to admit that because of the behavior of Microsoft around the Azure Dev GO SDK I lost my motivation to further participate in the development of the Terraform provider.

@davidcorrigan714
Copy link
Contributor Author

Yeah, really wish it was in better shape and more feature complete. We're really leaning into Terraform so I've had some run time to contribute back to it but my time for that is running out soon. Got one or two service connections to do still, and then update them again once OIDC support is officially out. I really want want access control lists too instead of one-off permissions, not too hard to implement but a good chunk of code none the less.

@cveld
Copy link

cveld commented Apr 26, 2023

Side question - Is it actually required that providers are built with go? Maybe it is better to eventually switch to .NET? Or Python? Not sure if there is any reusability with az cli devops extension code.

@cveld
Copy link

cveld commented Apr 26, 2023

Design question - will it be possible to use the same authentication mechanism that the azurerm provider uses? I.e. az cli for user accounts and environment variables for service principals? Not sure actually why the azurerm provider is not supporting az cli for service principals.

@davidcorrigan714
Copy link
Contributor Author

Technically no. They run in a separate process and communicate over gRPC iirc. Practically Hashicorp put a lot of effort into the Go SDK and the ROI of duplicating that for another language is pretty darn low unless adopted by a wide range of plugins. I'm not a Go person myself and only know it to write plugins for Terraform and Vault, but really the code isn't particularly complicated language wise. There wouldn't be any reusability from other stuff other than the API for Azure DevOps itself BUT everyone should be using OpenAPI and their generator of choice anyways for whatever languages need to be supported which is more or less what Microsoft built out for Azure DevOps.

@davidcorrigan714
Copy link
Contributor Author

For fun someone tried to do one in Rust: https://tevps.net/blog/2021/11/7/poc-terraform-provider-rust/

@cveld
Copy link

cveld commented Apr 26, 2023

Bold question, what would be the ETA on the service principal support?

@davidcorrigan714
Copy link
Contributor Author

davidcorrigan714 commented Apr 27, 2023

Whenever Microsoft gets around to reviewing and merging the PR. Seems like they try to release the provider quarterly-ish. We've been using this authentication method on our fork for a bit now and it's doing its thing fine. Actually just got asked by a team that wants to use it a bunch more but we're hitting the challenges and limits mentioned here and here where there's effectively a 10 workspace limit per service principal. We're looking to setup a bunch of environments for a project, each with a terraform workspace that just needs to put some details into service connections. Really hope Microsoft adds some pattern matching support to federated authentication soon so it'll scale a bit better.

@cveld
Copy link

cveld commented Apr 28, 2023

@davidcorrigan714 do you have guidance how I would set up your fork? somehow I need to get the forked executable into the providers workspace folder. Is it possible to publish this forked executable somewhere and get it pulled by terraform init with some reference?

@davidcorrigan714
Copy link
Contributor Author

@cveld They're all here. Our fork is public primarily to upstream work here, but we do run those binaries and they are available on the terraform registry. We will break things between releases as we fix or re-align fields as reviews here are addressed and re-merged back to our fork.

@nwmcsween
Copy link

Currently waiting on this to automate devops instead of the messy keyvault PAT pattern.

@cveld
Copy link

cveld commented Jul 10, 2023

@davidcorrigan714 release 0.6 just got published but unfortunately without your nice work #747 😢

@nwmcsween
Copy link

I haven't looked at the code but I'm assuming it's going to be a bit of work to redo this on the new rest API support

@davidcorrigan714
Copy link
Contributor Author

@cveld Yeah, been a busy few weeks and haven't had time to circle back and get this, or my other PRs, updated.

@nwmcsween This one shouldn't be too bad since it really doesn't do much with the REST API, just obtains a token from a different source. Though with the new API some more SP functions should be available so we can get the data sources working with them too.

@cveld
Copy link

cveld commented Sep 5, 2023

0.9 got out and still no service principal auth 😢@davidcorrigan714 how can I help? Can we set up a block of some hours to work together on the PR? #747

@tmeckel
Copy link
Contributor

tmeckel commented Sep 13, 2023

@cveld @davidcorrigan714 I don't like to disappoint other people, but from my point of view the support for a service principal authentication has bigger problems than a few lines of code in the provider.

#747 (comment)

These are the minimum changes to the Azure DevOps GO SDK to support short living OAuth access tokens:

diff --git a/azuredevops/v7/client.go b/azuredevops/v7/client.go
index e0d0d4c..7f0b463 100644
--- a/azuredevops/v7/client.go
+++ b/azuredevops/v7/client.go
@@ -84,7 +84,7 @@ func NewClientWithOptions(connection *Connection, baseUrl string, options ...Cli
 type Client struct {
        baseUrl                 string
        client                  *http.Client
-       authorization           string
+       authorization           func() (string, error)
        suppressFedAuthRedirect bool
        forceMsaPassThrough     bool
        userAgent               string
@@ -169,8 +169,12 @@ func (client *Client) CreateRequestMessage(ctx context.Context,
                req = req.WithContext(ctx)
        }

-       if client.authorization != "" {
-               req.Header.Add(headerKeyAuthorization, client.authorization)
+       if client.authorization != nil {
+               ah, err := client.authorization()
+               if err != nil {
+                       return nil, err
+               }
+               req.Header.Add(headerKeyAuthorization, ah)
        }
        accept := acceptMediaType
        if apiVersion != "" {
diff --git a/azuredevops/v7/connection.go b/azuredevops/v7/connection.go
index eb76f53..9bb3fc5 100644
--- a/azuredevops/v7/connection.go
+++ b/azuredevops/v7/connection.go
@@ -19,7 +19,9 @@ func NewPatConnection(organizationUrl string, personalAccessToken string) *Conne
        authorizationString := CreateBasicAuthHeaderValue("", personalAccessToken)
        organizationUrl = normalizeUrl(organizationUrl)
        return &Connection{
-               AuthorizationString:     authorizationString,
+               AuthorizationString: func() (string, error) {
+                       return authorizationString, nil
+               },
                BaseUrl:                 organizationUrl,
                SuppressFedAuthRedirect: true,
        }
@@ -34,7 +36,7 @@ func NewAnonymousConnection(organizationUrl string) *Connection {
 }

 type Connection struct {
-       AuthorizationString     string
+       AuthorizationString     func() (string, error)
        BaseUrl                 string
        UserAgent               string
        SuppressFedAuthRedirect bool

Far from ideal but it will work, because CreateRequestMessage is called by every endpoint implementation in the SDK (via a call to cliend.Send) and by always calling the authorization function the implementer of this authorization function can return a new OAuth Bearer header if an access token has expired in the meanwhile since the last call.

The current implementation only allows setting the OAuth Bearer header once. But the runtime of the provider instance might exceed the lifetime of the (static) access token.

@davidcorrigan714
Copy link
Contributor Author

@cveld I'm sad too, both life and work have been crazy for me to find time to get this over the finish line. Making it a priority this week to try to get this in. tmeckel makes a good point on the token lifetime though, and I should've known better myself to handle that token expiration somehow. Maybe I was thinking they were valid for a week, but that wouldn't have been reliable anyways.

@cveld
Copy link

cveld commented Sep 13, 2023

Just to note that I just figured out that I don't need service principal auth for most of my use cases at all as I can just pass the pipeline access token as the "PAT" into the provider 😅

Secondly what is very cool that I got access to the private preview of workload identity: now I can connect credential free with Azure from Azure Pipelines like GitHub supports sinds November 2021 😍

@davidcorrigan714
Copy link
Contributor Author

@cveld Nice. I've played with it a bit. There's still some things lacking with WIF, more claims like GH has would be nice. Azure Service Principals really need better support for more flexible claim parsing too. GH desperately needs OIDC authentication into their platform. One of my upcoming projects is to use it to authenticate to Artifactory, using their Vault plugin to handle the OIDC authentication.

@LaurentLesle
Copy link

Do we have an implementation already to invite an Azure Devops User into a devops organization and also assign project's permissions? Looks like Service Principal, User managed Identity and System Identity would be the other way the provider could also login to Azure Devops

https://learn.microsoft.com/en-us/azure/devops/integrate/get-started/authentication/service-principal-managed-identity?view=azure-devops

@cveld
Copy link

cveld commented Sep 26, 2023 via email

@cveld
Copy link

cveld commented Oct 23, 2023

An access token is valid for 1 hour right? That should be sufficient for most use cases. I would ship this as a rough first version already.

@michelefa1988
Copy link

With the introduction of workload identity federations, we are now able to eliminiate a good bunch of tokens. When will this be released please as it is a must have in todays world. Having to manually manage this specific token is super frustrating + introduces many security flaws.

@xuzhang3 xuzhang3 linked a pull request Dec 13, 2023 that will close this issue
11 tasks
@michelefa1988
Copy link

@xuzhang3 is Azure workload identity supported?

@xuzhang3
Copy link
Collaborator

@michelefa1988 managed identity/ SPN authorization are supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants