Service Principal Authentication #747

davidcorrigan714 · 2023-04-06T02:58:52Z

All Submissions:

Have you added an explanation of what your changes do and why you'd like us to include them?
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
My code follows the code style of this project.
I ran lint checks locally prior to submission.
Have you checked to ensure there aren't other open PRs for the same update/change?

Allows the terraform provider to authenticate to azure devops through a service principal using a variety of authentication methods: client secret, client certificate, GitHub Actions OIDC tokens, Terraform Cloud OIDC tokens, & any other OIDC token provided directly as an attribute

Issue Number: #745

Does this introduce a change to `go.mod`, `go.sum` or `vendor/`?

Yes
No

Adds the Azure Go SDK & some crypto functions.

Does this introduce a breaking change?

Yes
No

Adding the JFrog V2 service connections. I just copy+pasted the existing Artifactory resource, tests & docs then adjusted accordingly. They all use the same parameters. One unit test needed fixing, the argocd connection seemed to be based on these so I adopted the same fix that was in that resource. Unit & acceptance tests for the service connections have passed. Docs have been updated. The formatting scripts in the repo want to reformat all sorts of stuff. I'll leave that to Microsoft's discretion when I upstream this.

This provider stores secret values in the state file by hashing them with bcrypt. bcrypt only uses the first 72 characters for the hash which results in issues with Artifactory JWTs because the first 72 characters of those tokens contain data about the algorithms and subjects which don't change when they're regenerated. The recommendation from OWASP is to use Argon2 as the current standard for password hashing. I've adopted that for our fork of the provider in this PR. I have no inclination of what Microsoft might ultimately do to resolve the issue upstream but worst case our secrets will get re-pushed to Azure DevOps when upstream resolves this issue and we either merge it in or use the upstream provider again.

…uredevops into ni-main

* Created service end point for NuGet * Create documentation * Minor fixes * Modelled along the lines of jfrog_artifactory_v2 * Fix doc as per JFrog V2 * Added the third variant * Fix implementation * Fix nugetkey * Cleanup * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Update azuredevops/internal/service/serviceendpoint/resource_serviceendpoint_nuget.go Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com> * Put fixes after addressing review comments --------- Co-authored-by: David Corrigan <47994662+davidcorrigan714@users.noreply.github.com>

Minor data handling fixes, affects importing and some picky unit tests.

* Service Principal Support

* remove hashing shenanigans

Handle padded & unpadded base64 from Terraform tokens

website/docs/index.html.markdown

xuzhang3 · 2023-05-30T09:30:38Z

@davidcorrigan714 can you help rebase to the latest code, we upgrade the SDK from v6 to v7,.

jaredfholgate · 2023-07-24T15:28:26Z

azuredevops/provider.go

@@ -141,6 +167,134 @@ func Provider() *schema.Provider {
 				Description: "The personal access token which should be used.",
 				Sensitive:   true,
 			},
+			"sp_client_id": {


@davidcorrigan714 Apologies if this is already documented somewhere, but I'm wondering why the choice was made to use different env vars to the azurerm, azuread and azapi providers? If it worked the same as those providers by default, it might reduce the setup complexity for some users. E.g. ARM_USE_OIDC, etc.

I can go double check this, the main concern is ensuring that they can be configured independently when needed and support multiple instances to different accounts. Not sure the Azure one suffered that bug but the AWS implementation had issues with that, I should find that bug and double check what they did there. The environment variable names for them do seem to provider specific though, like this isn't Azure Resource manager so ARM_.... doesn't really make sense to me

@davidcorrigan714 @jaredfholgate @xuzhang3 whatever happened to not using ARM_* vars? This is now breaking pipelines that have (a subset) of those vars set for the other azure* providers, but have no relevance for azuredevops.

I understand the — let's make this easy for people so it "just works" — sentiment, but in practice this will "just break" things for people in an unsuspecting way if they (or their pipeline) use a combination of the azure{rm,ad,devops} providers. Which will not be an empty groups me thinks. $0.02 given.

Thinking through on this, how about when this (for some people unintended) OIDC authentication is attempted and it fails, the provider shows a hint in how this new behavior can be disabled. Perhaps with ~~the code snippet I give in the comment below~~, or a more elegant way like for instance ignore_arm_env_vars = true. #OpenForExtensionClosedForModification

Here's a workaround in case someone else is stumbling onto this:

provider "azuredevops" { // … other settings // prevent enabling OIDC when `ARM_*` vars are detected in CI use_oidc = false // not used, but these need to be set to valid GUID if `use_oidc` is set client_id = "deadbeef-dead-d34d-dead-deadbeefdead" tenant_id = "deadbeef-dead-d34d-dead-deadbeefdead" }

Edit: turns out this does not work 😢 after all:

│ Error: Invalid combination of arguments │ │ with provider["registry.terraform.io/microsoft/azuredevops"], │ on _providers.tf line 21, in provider "azuredevops": │ 21: provider "azuredevops" { │ │ "client_secret_path": only one of │ `client_certificate,client_certificate_path,client_secret,client_secret_path,personal_access_token,use_msi,use_oidc` │ can be specified, but `personal_access_token,use_oidc` were specified.

Pinning to version = "~> 0.11" until this is resolved or until we have time to refactor our authentication configuration.

tmeckel · 2023-09-13T09:18:39Z

azuredevops/provider.go

+			return nil, diag.FromErr(err)
+		}
+
+		client, err := client.GetAzdoClient(token, d.Get("org_service_url").(string), terraformVersion)


The call to GetAzdoClient will call NewPatConnection, what in turn will create a Basic Auth HTTP header using an OAuth token, what eventually will fail the authentication, because OAuth tokens must be passed as a Bearer Auth Header.

An even bigger issue is here that the Azure DevOps GO API uses a struct instead of an interface to pass a Connection around to the various REST API endpoint implementations and the Connection struct only supports a static auth header value. This is sufficient for a long living PAT but far from ideal for short living OAuth tokens. As long as this issue isn't fixed in the GO API is don't see a reliable way to support Service Principal Authentication for Azure DevOps.

@xuzhang3 any infos who's actually in charge for the GO API? Or is it abandoned again?

#745 (comment)

That's a good point, I thought the OAuth tokens were valid long enough that it wouldn't practically be a concern. Looking at your other comment on the other thread I can test that and put it in a PR up to the Go SDK if someone is available to review it.

@davidcorrigan714 The issue with the Azure DevOps GO SDK ist, that it's generated using a framework not created manually. So that's why most PRs got rejected there in past, because issues have to be fixed in the framework (which is not available here on GitHub) and not in the generated code in the repo you see. Of course, the fact that the AzDO team maintains the GO SDK so carelessly makes things even worse. This is one of the main reasons why I personally stopped contributing to the project and in addition to me I think Microsoft focuses on GitHub and slowly let AzDO die. That AzDO has much more stability issues compared to GitHub is a another good indicator that the manpower behind AzDO is very limited.

Oh I'm well aware of the code-gen happening in all the SDKs. Pretty sure they completely lost or broke the Python SDK generator tooling a few years back, or lost the expertise that was running it. connection.go just doesn't show any history of ever being a generated file like the rest of the SDK is.

Don't want to derail this into GH vs AzDO too much. The reality is they don't have feature parity on the roadmap for the two platforms in certain areas and the rate they've been improving since trying to pitch GH as an AzDO alternative still puts it at least 5+ years out as a viable alternative for most of our use cases. We also do some "interesting" things with Pipelines that GH may never support in Actions.

Any update? I would say ship a first version without refresh token support 😊

The Azure DevOps team doesn't really have go developers, Azure DevOps is built in .NET, PowerShell and a bit of NodeJS and some web front-end stuff. The fact they even have a Go SDK amazes me, the team generates the Go and the Node SDK from the .NET API metadata as far as I know.

I don't think they maintain these libraries "carelessly", it's just that the person who wrote it, probably isn't available at the moment.

It's one of the big pains of building SDKs at all, you tend to need to build them in a plethora of languages, Python, .NET, JS, Go, Ruby etc, but the team that builds the SDK doesn't normally work with any of thee languages other than .NET. I see the same issue with many other frameworks and SDKs, the best SDK is written in for the language of the tool itself.

The team maintaining TF az* providers is a central team.
While you wait, consider this workaround that uses the external datasource to acquire a token with the Azure CLI:

data external azdo_token { program = [ "az", "account", "get-access-token", "--resource", "499b84ac-1321-427f-aa17-267ca6975798", # Azure DevOps "--query","{accessToken:accessToken}", "-o","json" ] } provider azuredevops { org_service_url = local.azdo_organization_url personal_access_token = data.external.azdo_token.result.accessToken }

@jessehouwing yeah the choice of Go for Terraform always makes me a bit sad, it's the only reason I've picked up Go and I'm certainly not amazing at it though the Terraform stuff usually has plenty of examples to draw from.

Catching up on this thread again now and seeing where things are at with the code.

xuzhang3 · 2023-11-17T08:13:16Z

@davidcorrigan714 can you help resolve the conflict?

XtratusCloud · 2023-12-11T08:05:43Z

I am very interested in the final implementation of this functionality, as it would enable the use of this functionality for authentication through Managed Identities, on the road to password-less processes.
Please, if it would be possible to complete this pull request to include this functionality in the next version?
It would be of great benefit.
Thanks,

davidcorrigan714 · 2023-12-14T03:32:25Z

Got most of the feedback incorporated with refreshing the access token as needed and aligning with the other Azure terraform provider variables where possible. Going to do a bit of manual testing tomorrow for the various cloud identity scenarios - GH, Managed Identities & HCP Workload Identities. The changes to the Azure DevOps Go API are currently hacked into it in the vendor folder so I need to do a PR there before this goes in.

davidcorrigan714 · 2023-12-14T03:35:03Z

Maybe @xuzhang3 can provide some insights on what's failing in the automated checks. Wouldn't surprise me if I missed a format or package checks but can't see the logs ☹

xuzhang3 · 2023-12-14T06:27:56Z

Maybe @xuzhang3 can provide some insights on what's failing in the automated checks. Wouldn't surprise me if I missed a format or package checks but can't see the logs ☹

@davidcorrigan714 The pipe fails due to the line break in the vendor files. I usually add the git config git config --global core.autocrlf true to my local env to ensure that different end-of-file lines will be managed automatically by git. Since the vendor files has been add pushed to this branch you need git config --global core.autocrlf false and push the files again

valentin-fischer · 2024-01-11T09:16:53Z

Can anyone check and fix the build so we can get this merged? Thanks!

troyel · 2024-01-25T19:26:05Z

@davidcorrigan714 Looks to me like you have a lot of great work in here! Do you think you have any time to take this through the home stretch? I think this would be very useful for a lot of people and orgs! 🙌

xuzhang3 · 2024-02-06T05:40:32Z

website/docs/guides/authenticating_managed_identity.html.md

+```hcl
+terraform {
+  required_providers {
+    azapi = {


should be azuredevops not azapi

xuzhang3 · 2024-02-06T05:40:49Z

website/docs/guides/authenticating_managed_identity.html.md

+```hcl
+terraform {
+  required_providers {
+    azapi = {


should be azuredevops not azapi

xuzhang3 · 2024-02-06T05:50:02Z

website/docs/guides/authenticating_service_principal_using_an_oidc_token.html.md

+
+  client_id = "00000000-0000-0000-0000-000000000001"
+  tenant_id = "00000000-0000-0000-0000-000000000001"
+  oidc_hcp  = true


oidc_hcp is not supported

xuzhang3 · 2024-02-06T05:50:09Z

website/docs/guides/authenticating_service_principal_using_an_oidc_token.html.md

+  client_id_apply = "00000000-0000-0000-0000-000000000001"
+  tenant_id_plan  = "00000000-0000-0000-0000-000000000001"
+  tenant_id_apply = "00000000-0000-0000-0000-000000000001"
+  oidc_hcp  = true


oidc_hcp is not supported

xuzhang3 · 2024-02-06T05:57:55Z

website/docs/index.html.markdown

+
+The OIDC service principal authentication methods allow for secure passwordless authentication from [Terraform Cloud](https://developer.hashicorp.com/terraform/cloud-docs/workspaces/dynamic-provider-credentials) & [GitHub Actions](https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect).
+
+* [Authenticating to a Service Principal with a Terraform Cloud Workload Identity Token](guides/authenticating_service_principal_using_hcp_token.html)


Documention authenticating_service_principal_using_hcp_token.html does not exist，should be the manage identity document

xuzhang3 · 2024-02-06T06:02:43Z

website/docs/index.html.markdown

+- `oidc_github_actions_audience` - Custom audience for the GitHub Actions OIDC token.
+It can also be sourced from the `ARM_OIDC_GITHUB_ACTIONS_AUDIENCE` environment variable.
+
+- `oidc_hcp` - Boolean, set to true to use the Terraform Cloud OIDC workload identity token to authenticate to a service principal.


oidc_hcp does not exist

xuzhang3 · 2024-02-06T06:12:37Z

@davidcorrigan714 if you do not have time to update this PR, can we create another PR based on your changes?

xuzhang3 · 2024-02-19T08:37:07Z

@davidcorrigan714 close this PR via #970

davidcorrigan714 · 2024-03-08T22:04:07Z

@xuzhang3 appreciate you all finishing this up and getting it in. Life has been too darn busy and still is.

davidcorrigan714 and others added 13 commits January 17, 2023 16:12

Add Workflow for publishing releases

1cc80b5

add library & variable group permissions (#7)

48f8085

Merge remote-tracking branch 'origin/main' into ni-main

9381614

revert

7ca8339

Merge remote-tracking branch 'origin/main' into ni-main

001454b

Merge branch 'ni-main' of https://github.com/ni/terraform-provider-az…

7ba6afa

…uredevops into ni-main

remove build comments/commands(?) (#9)

cf54a73

Make username visible for NuGet (#10)

3a39eba

Minor data handling fixes, affects importing and some picky unit tests.

Merge branch 'microsoft:main' into ni-main

64bd5e2

Service Principal Auth Support

f45caa9

davidcorrigan714 force-pushed the users/dcorriga/upstream-sp-auth branch from 3ceefae to f45caa9 Compare April 6, 2023 03:49

davidcorrigan714 marked this pull request as ready for review April 6, 2023 03:51

davidcorrigan714 and others added 6 commits April 6, 2023 08:46

Add Service Principal Authentication methods (#12)

132ac70

* Service Principal Support

Remove service connections hashing & secret data handling cleanup (#11)

cbc4fc0

* remove hashing shenanigans

Update provider.go

3591ba4

Merge pull request #14 from ni/users/dcorriga/fix-padding

ec30e49

Handle padded & unpadded base64 from Terraform tokens

Fix token base64 padding

72b6e71

fix protection status (#16)

4460c7b

xuzhang3 added the new-feature label Apr 27, 2023

cveld reviewed May 3, 2023

View reviewed changes

website/docs/index.html.markdown Outdated Show resolved Hide resolved

typo

00cd0f2

xuzhang3 mentioned this pull request Jun 6, 2023

Enabling authentication with service principal #784

Open

cveld mentioned this pull request Jul 10, 2023

Service Principal Authentication Support #745

Closed

davidcorrigan714 added 3 commits July 20, 2023 22:02

merge

d44424d

cleanup WIP

c78566a

More cleanup WIP

927440e

jaredfholgate reviewed Jul 24, 2023

View reviewed changes

tmeckel reviewed Sep 13, 2023

View reviewed changes

xuzhang3 mentioned this pull request Sep 20, 2023

Allow authentication in same manner as azurerm via azure cli login #419

Open

jackmtpt mentioned this pull request Oct 16, 2023

Support for federated authentication instead of PAT? #903

Closed

xuzhang3 added the enhancement label Nov 17, 2023

davidcorrigan714 added 5 commits December 8, 2023 15:29

align env naming

035b7a5

initial refactor for refreshing access tokens

2732f11

merge with main

e4d375c

Doc fixes, OIDC WIP

02136e3

oidc cleanup, msi

09a4564

MSI & docs improvements

9755f19

xuzhang3 linked an issue Dec 13, 2023 that may be closed by this pull request

Service Principal Authentication Support #745

Closed

fixup tests

601a429

xuzhang3 added the documentation label Feb 6, 2024

xuzhang3 requested changes Feb 6, 2024

View reviewed changes

xuzhang3 mentioned this pull request Feb 19, 2024

Service Principal Authentication #970

Merged

11 tasks

xuzhang3 merged commit 601a429 into microsoft:main Feb 19, 2024
1 of 3 checks passed

This was referenced Mar 7, 2024

Use PAT Lifecyle Management APIs to generate dynamic/ephemeral PAT token for provider #943

Open

Sourcing from AZDO_PERSONAL_ACCESS_TOKEN environment does not work anymore for microsoft/azuredevops v1.0.0 #990

Closed

algompluecker mentioned this pull request Mar 11, 2024

The Azure DevOps Provider should be able to authenticate with a managed identity. pulumi/pulumi-azuredevops#238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service Principal Authentication #747

Service Principal Authentication #747

davidcorrigan714 commented Apr 6, 2023

xuzhang3 commented May 30, 2023

jaredfholgate Jul 24, 2023

davidcorrigan714 Sep 13, 2023

jubr Mar 7, 2024 •

edited

jubr Mar 7, 2024 •

edited

tmeckel Sep 13, 2023 •

edited

davidcorrigan714 Sep 13, 2023

tmeckel Sep 13, 2023 •

edited

davidcorrigan714 Sep 13, 2023

cveld Dec 6, 2023

jessehouwing Dec 7, 2023

geekzter Dec 8, 2023

davidcorrigan714 Dec 8, 2023

xuzhang3 commented Nov 17, 2023

XtratusCloud commented Dec 11, 2023 •

edited

davidcorrigan714 commented Dec 14, 2023

davidcorrigan714 commented Dec 14, 2023

xuzhang3 commented Dec 14, 2023

valentin-fischer commented Jan 11, 2024

troyel commented Jan 25, 2024

xuzhang3 Feb 6, 2024

xuzhang3 Feb 6, 2024

xuzhang3 Feb 6, 2024

xuzhang3 Feb 6, 2024

xuzhang3 Feb 6, 2024

xuzhang3 Feb 6, 2024

xuzhang3 commented Feb 6, 2024

xuzhang3 commented Feb 19, 2024

davidcorrigan714 commented Mar 8, 2024


		The OIDC service principal authentication methods allow for secure passwordless authentication from [Terraform Cloud](https://developer.hashicorp.com/terraform/cloud-docs/workspaces/dynamic-provider-credentials) & [GitHub Actions](https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect).

		* [Authenticating to a Service Principal with a Terraform Cloud Workload Identity Token](guides/authenticating_service_principal_using_hcp_token.html)

Service Principal Authentication #747

Service Principal Authentication #747

Conversation

davidcorrigan714 commented Apr 6, 2023

All Submissions:

Does this introduce a change to go.mod, go.sum or vendor/?

Does this introduce a breaking change?

xuzhang3 commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubr Mar 7, 2024 • edited

Choose a reason for hiding this comment

jubr Mar 7, 2024 • edited

Choose a reason for hiding this comment

tmeckel Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeckel Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhang3 commented Nov 17, 2023

XtratusCloud commented Dec 11, 2023 • edited

davidcorrigan714 commented Dec 14, 2023

davidcorrigan714 commented Dec 14, 2023

xuzhang3 commented Dec 14, 2023

valentin-fischer commented Jan 11, 2024

troyel commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhang3 commented Feb 6, 2024

xuzhang3 commented Feb 19, 2024

davidcorrigan714 commented Mar 8, 2024

Does this introduce a change to `go.mod`, `go.sum` or `vendor/`?

jubr Mar 7, 2024 •

edited

jubr Mar 7, 2024 •

edited

tmeckel Sep 13, 2023 •

edited

tmeckel Sep 13, 2023 •

edited

XtratusCloud commented Dec 11, 2023 •

edited