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.

Sign up for GitHub

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

Jump to bottom

[identity] Support Federated Identity for Service connections - new credential #29392

Merged

KarishmaGhiya merged 37 commits into Azure:main from KarishmaGhiya:service-connection-credential

Apr 26, 2024

Member

KarishmaGhiya commented Apr 23, 2024 •

edited

Loading

Packages impacted by this PR

Issues associated with this PR

Fixes #27093
Gist

Describe the problem that is addressed by this PR

A new Credential is created to support the WI federation for service connections in Azure Devops.
For complete details on the design please review Gist
Alternate designs that we had discussed and rejected due to complexity and bad UX- [Identity] Callback design for service connection support for Workload Identity Federation #29349 and [Identity] Support Workload Identity for service connections #28628
We are not supporting this in DAC at the moment because of introduction of complicated design issues in DAC. Also we don't see this niche scenario that should be added to DAC.
We can always make additional changes in the future to DAC with minimal design changes once Devops service provides better environment variable support for this scenario. But if we introduce the support now, we'll have to expose the parameter through DAC Options bag and it can be disruptive to remove in the future.

Work TBD:

Update TS guide
Update error messages
Test it out for all scenarios

Checklists

Added impacted package name to the issue description
Added a changelog (if necessary)

KarishmaGhiya requested review from bterlson, xirzec, jeremymeng, mpodwysocki, maorleger, schaabs and a team as code owners

April 23, 2024 00:35

KarishmaGhiya requested review from pvaneck, billwert, ahsonkhan and xiangyan99

April 23, 2024 00:35

github-actions bot added the Azure.Identity label

KarishmaGhiya changed the title ~~Service connection credential~~ [identity] Service connection credential

This was referenced Apr 23, 2024

[Identity] Callback design for service connection support for Workload Identity Federation #29349

Closed

[Identity] Support Workload Identity for service connections #28628

Closed

KarishmaGhiya changed the title ~~[identity] Service connection credential~~ [identity] Support Federated Identity for Service connections

KarishmaGhiya changed the title ~~[identity] Support Federated Identity for Service connections~~ [identity] Support Federated Identity for Service connections - new credential

Collaborator

azure-sdk commented Apr 23, 2024

API change check

APIView has identified API level changes in this PR and created following API reviews.

@azure/identity

Member Author

KarishmaGhiya commented Apr 23, 2024

// should we test for a GUID?

maorleger requested changes

View reviewed changes

Member

maorleger left a comment

Thanks for getting this started! There are a few items that should be addressed before merging but it's coming together well - let me know how I can help

sdk/identity/identity/review/identity.api.md Outdated Show resolved Hide resolved

sdk/identity/identity/review/identity.api.md

                   tenantId?: string;
               }
+              // @public (undocumented)
+              export class AzurePipelinesServiceConnectionCredential implements TokenCredential {

Member

maorleger Apr 23, 2024

I do think that AzurePipelinesCredential with a constructor that takes a required serviceConnectionId should be in consideration.

I know we've gone back and forth on this design and will support this current design, but I think

const myServiceConnectionId = "12345"
const credential = new AzurePipelinesCredential(clientId, tenantId, myServiceConnectionId)

is clear and offers a path to extending this via constructor overloads. Then, everything under the umbrella of "use this credential in Azure DevOps" can be bucketed and discovered under one name...

Member

maorleger Apr 24, 2024 •

edited

Loading

To clarify, I am proposing renaming this credential to AzurePipelineCredential -otherwise the constructor LGTM

Member Author

KarishmaGhiya Apr 24, 2024

plural :D

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts Outdated Show resolved Hide resolved

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts Outdated

+                private serviceConnectionId: string | undefined;
+                /**
+                 * AzurePipelinesServiceConnectionCredential supports Microsoft Entra Workload ID on Kubernetes.

Member

maorleger Apr 23, 2024

I think this comment is incorrect right? Probably copy-paste?

What do we want to document here?

I think we probably want to:

Link to the entra docs
Provide a little more context on what this credential is used for

Try copilot to generate some docs😄

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts Outdated Show resolved Hide resolved

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts Outdated Show resolved Hide resolved

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts Outdated Show resolved Hide resolved

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts Outdated Show resolved Hide resolved

sdk/identity/identity/src/util/logging.ts Show resolved Hide resolved

sdk/identity/identity/test/public/node/azurePipelinesServiceConnection.spec.ts Outdated Show resolved Hide resolved

scottaddie reviewed

View reviewed changes

Member

scottaddie left a comment

Reminder to add the new credential type to https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/README.md#credential-classes

maorleger reviewed

View reviewed changes

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts Outdated Show resolved Hide resolved

maorleger reviewed

View reviewed changes

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts

+                  ) {
+                    return;
+                  }
+                  const missingEnvVars = [];

Member

maorleger Apr 25, 2024

nit (feel free to ignore): a loop would be easier to follow and modify in the future

Pseudocode:

const requiredEnvVars = ["SYSTEM_TEAMFOUNDATIONCOLLECTIONURI", "SYSTEM_TEAMPROJECTID" ....etc]
const missingEnvVars = requiredEnvVars.filter(e => !process.env[e])
if (missingEnvVars.length > 0) { etc

scottaddie reviewed

View reviewed changes

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts Outdated Show resolved Hide resolved

maorleger approved these changes

View reviewed changes

Member

maorleger left a comment

Just a few more comments that have not been addressed but I don't want to block us getting a nightly build out. Thank you for being patient with me!

sdk/identity/identity/review/identity.api.md Outdated Show resolved Hide resolved

sdk/identity/identity/src/credentials/azurePipelinesServiceConnectionCredential.ts

+                  serviceConnectionId: string,
+                  options?: AzurePipelinesServiceConnectionCredentialOptions
+                ) {
+                  checkTenantId(logger, tenantId);

Member

maorleger Apr 25, 2024

Not blocking this PR if you prefer to make an issue and get a dev-build going

sdk/identity/identity/test/public/node/azurePipelinesServiceConnection.spec.ts

+                  if (!isLiveMode()) {
+                    this.skip();
+                  }
+                  // this serviceConnection corresponds to the Azure SDK Test Resources - LiveTestSecrets service

Member

maorleger Apr 25, 2024

👍

KarishmaGhiya force-pushed the service-connection-credential branch from a8aefa0 to 00e9aec Compare

April 25, 2024 22:43

KarishmaGhiya added 4 commits

April 25, 2024 16:56


          update the api view for service connections WI

8413a7a


          service connection for WI

ed09ef6


          update the WIC


          undo changes to DAC

b947d1d

KarishmaGhiya and others added 18 commits

April 25, 2024 16:57


          update code

987b0c6


          using correct field name

03747da


          add credential

c909ac4


          update pr

da72605


          update api

8f80a2f


          format

7b7a949


          fix lint

f5e5bee


          update credential

320b1ee


          update the design and api view

74fa416


          update docs

42742e4


          fix linting

f91d627


          update service connection

f66b82f


          test cleanup

4a700e9


          Update sdk/identity/identity/src/credentials/azurePipelinesServiceCon…

606d70e

…nectionCredential.ts

Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>


          Update sdk/identity/identity/src/credentials/azurePipelinesServiceCon…

…nectionCredential.ts

Co-authored-by: Maor Leger <maorleger@users.noreply.github.com>


          add verification

9b39405


          update code

45da28e


          update api view

9b42986

KarishmaGhiya force-pushed the service-connection-credential branch from 3ea025e to 9b42986 Compare

April 25, 2024 23:57


          update docs, samples, readme and lint

e084c7b

Member Author

KarishmaGhiya commented Apr 26, 2024 •

edited

Loading

Reminder to add the new credential type to https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/README.md#credential-classes

done, don't have a learn.microsoft url for this since credential is not published so didn't add that

KarishmaGhiya enabled auto-merge (squash)

April 26, 2024 00:24


          update pnpm lock

1ca059d

KarishmaGhiya requested review from ckairen and deyaaeldeen as code owners

April 26, 2024 00:45


          formatting

07d908e

KarishmaGhiya merged commit 63dce5e into Azure:main

24 checks passed

christothes reviewed

View reviewed changes

sdk/identity/identity/samples/AzureIdentityExamples.md

+                const clientId = "<YOUR_CLIENT_ID>"
+                const tenantId = "<YOUR_TENANT_ID>"
+                const serviceConnectionId = "<YOUR_SERVICE_CONNECTION_ID>"
+                const credential = new AzurePipelinesServiceConnection(tenantId, clientId, serviceConnectionId);

Member

christothes Apr 26, 2024

Is this intended to be AzurePipelinesServiceConnectionCredential?

jhwj9617 commented Jun 3, 2024

Does this/will this work with DefaultAzureCredential?

jhwj9617 mentioned this pull request

Azure CLI task fails with AADSTS700024 after 60 minutes Azure/azure-cli#28708

Open

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

Reviewers

christothes christothes left review comments

scottaddie scottaddie left review comments

maorleger maorleger approved these changes

bterlson Awaiting requested review from bterlson bterlson is a code owner

xirzec Awaiting requested review from xirzec

jeremymeng Awaiting requested review from jeremymeng jeremymeng is a code owner

mpodwysocki Awaiting requested review from mpodwysocki

schaabs Awaiting requested review from schaabs schaabs is a code owner

pvaneck Awaiting requested review from pvaneck pvaneck is a code owner automatically assigned from Azure/azure-sdk-write-identity

billwert Awaiting requested review from billwert billwert is a code owner automatically assigned from Azure/azure-sdk-write-identity

ahsonkhan Awaiting requested review from ahsonkhan ahsonkhan is a code owner automatically assigned from Azure/azure-sdk-write-identity

xiangyan99 Awaiting requested review from xiangyan99 xiangyan99 is a code owner automatically assigned from Azure/azure-sdk-write-identity

ckairen Awaiting requested review from ckairen ckairen is a code owner

deyaaeldeen Awaiting requested review from deyaaeldeen

Labels