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 Workload Identity for service connections #28628

Closed

KarishmaGhiya wants to merge 18 commits into Azure:main from KarishmaGhiya:testing-something

Member

KarishmaGhiya commented Feb 21, 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

WorkloadIdentityCredential is extended to support the WI federation for servie connections in Azure Devops.
For complete details on the design please review Gist
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, maorleger, witemple-msft, schaabs and a team as code owners

February 21, 2024 21:19

KarishmaGhiya requested review from pvaneck, billwert, chlowell, mpodwysocki and xiangyan99

February 21, 2024 21:19

KarishmaGhiya marked this pull request as draft

February 21, 2024 21:19

github-actions bot added the Azure.Identity label

Collaborator

azure-sdk commented Feb 21, 2024

API change check

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

@azure/identity

KarishmaGhiya added 2 commits

April 10, 2024 17:17


          update the api view for service connections WI

67e9bcd


          service connection for WI

KarishmaGhiya force-pushed the testing-something branch from ae08909 to 2111129 Compare

April 11, 2024 00:17

KarishmaGhiya added 3 commits

April 12, 2024 15:35


          update the WIC

4b22738


          undo changes to DAC

9cb216c


          update the api view

5f70c00

KarishmaGhiya changed the title ~~[Identity] update the api view for service connections Workload Identity~~ [Identity] Support Workload Identity for service connections

KarishmaGhiya mentioned this pull request

Add Support for Workload Identity Federation for Azure Service Connections #27093

Closed

KarishmaGhiya marked this pull request as ready for review

April 12, 2024 22:59


          update api view

1ac46e0

maorleger reviewed

View reviewed changes

Member

maorleger left a comment

Have a few questions / comments / nits - coming together though!

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

                   tenantId?: string;
                   tokenFilePath?: string;
               }
+              // @public
+              export type WorkloadIdentityCredentialOptions = WorkloadIdentityCredentialKubernetesOptions | WorkloadIdentityCredentialServiceConnectionOptions;

Member

maorleger Apr 15, 2024

What is the difference between WorkloadIdentityCredentialKubernetesOptions and WorkloadIdentityCredentialServiceConnectionOptions ? They look identical

Member Author

KarishmaGhiya Apr 15, 2024

One has federatedTokenFilePath required and the other has serviceConnectionId required

Member

maorleger Apr 16, 2024 •

edited

Loading

Oh I see! I missed the never on each side.

This makes more sense now, but I wonder if undefined is a better type than never here - because the union of <type> and never is just <type> this ends up being undefined anyway.

What I mean is, try this in a typescript playground:

type Foo = string | never

If you hover over Foo in the editor you'll notice that the type is just string - TypeScript is smart enough to know that never can not be in the set of values here. When a user tries to set both, the error message will say:

Type 'string' is not assignable to type 'undefined'.ts(2322)

Which can be confusing.

So what I am suggesting is to change never to explicit undefined as a starting point. It'll make for less confusing error messages. Does that make sense? Happy to discuss more or brainstorm an API shape that provides better devex!

Member Author

KarishmaGhiya Apr 17, 2024

Tagging @bterlson to discuss which type should we use here for forbidden fields.

Member

maorleger Apr 17, 2024

Thanks Karishma - happy to learn one way or the other 👍

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

                   if (tenantId) {
                     checkTenantId(logger, tenantId);
                   }
+                  if (this.federatedTokenFilePath && this.serviceConnectionId) {
+                    const errorMessage = `${credentialName}: ambiguous. serviceConnectionId and federatedTokenFilePath cannot be provided at the same time. These are used for supporting WorkloadIdentity for different environments.

Member

maorleger Apr 15, 2024

I think federatedTokenFilePath is what we name the variable internally but the param is called tokenFilePath - if that's the case we should change the error message to specify the correct param name (otherwise it may be confusing to the user).

Also, is it an error to have AZURE_FEDERATED_TOKEN_FILE env var set and pass in serviceConnectionId via the params? Just curious

Member Author

KarishmaGhiya Apr 15, 2024

Yes that is going to be an error scenario.

Member

maorleger Apr 16, 2024

Makes sense, thanks!

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

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

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

+                    },
+                  };
+                  const response = await fetch(oidcRequestUrl, requestOptions);

Member

maorleger Apr 15, 2024

We should, ideally, use the core-rest-pipeline primitives unless there's a reason to use fetch directly

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

+                    return true;
+                  }
+                  if (!process.env.SYSTEM_TEAMFOUNDATIONCOLLECTIONURI) {
+                    this.systemError = "SYSTEM_TEAMFOUNDATIONCOLLECTIONURI,";

Member

maorleger Apr 15, 2024

I see that we're building this systemError and then setting it as an instance variable - where do we actually use it / throw it?

I also wonder if it would be cleaner to collect missing env var names in an array and then creating the error if the array is not empty (pseudocode below)

const missingEnvVars = []
if (!process.env.SYSTEM_TEAMFOUNDATIONCOLLECTIONURI) missingEnvVars.push("SYSTEM_TEAMFOUNDATIONCOLLECTIONURI")
...
...
if (missingEnvVars.length > 0) this.systemError = "Missing system variable(s) - ${missingEnvVars.join(', ')}"

Last question here is: is this a developer error or an SDK error for these to be missing? They're set by Azure DevOps right?

Member Author

KarishmaGhiya Apr 15, 2024

They are set by Azure Devops. So definitely not an error by developer. but by Azure Devops

Member Author

KarishmaGhiya Apr 16, 2024

Yes, that's a better strategy using the array

Member Author

KarishmaGhiya Apr 16, 2024

done

scottaddie reviewed

View reviewed changes

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

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

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

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

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

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

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

KarishmaGhiya added 2 commits

April 15, 2024 17:51


          incorporate feedback

55cc8d5


          incorporate docs feedback

43baa31

Member

maorleger commented Apr 16, 2024

Apologies for the PR noise 😄 but the new code should have test coverage - could you add tests?

KarishmaGhiya added 10 commits

April 16, 2024 15:45


          update WI error messages

0de15a9


          update the test

99b1b8b


          wi dont skip

ed5e9fe


          update tests and code

dea306d


          test specific test

31a56eb


          use core-rest

f44bd2a


          add system access token in pipeline

aed3150


          update the Json response fields

3e50f6a


          update code

ebbbeda


          using correct field name

76c97eb

maorleger approved these changes

View reviewed changes

Member

maorleger left a comment

From a JS perspective this looks much better, thank you for indulging me! I'd like someone from the Identity crew to validate the logic as well 😇

sdk/identity/identity/src/credentials/workloadIdentityCredential.ts

+                    throw new CredentialUnavailableError(
+                      `${credentialName}: is Unavailable. clientId and tenantId are required parameters/ environment varables - AZURE_TENANT_ID, AZURE_CLIENT_ID`
+                    );
+                  }
                   if (tenantId) {
                     checkTenantId(logger, tenantId);

Member

maorleger Apr 17, 2024

nit: you could move outside the if and remove the if entirely since you are validating tenantId exists 👍

sdk/identity/identity/src/credentials/workloadIdentityCredential.ts

-                    workloadIdentityCredentialOptions.tokenFilePath || process.env.AZURE_FEDERATED_TOKEN_FILE;
+                    workloadIdentityCredentialOptions?.tokenFilePath || process.env.AZURE_FEDERATED_TOKEN_FILE;
+                  this.serviceConnectionId = workloadIdentityCredentialOptions.serviceConnectionId;
+                  if (!tenantId || !clientId) {

Member

maorleger Apr 17, 2024

nit: I would move this to line 65 to keep it next to the code that sets clientId and tenantId

sdk/identity/identity/src/credentials/workloadIdentityCredential.ts

+                    logger.error(errorMessage);
+                    throw new CredentialUnavailableError(errorMessage);
+                  }
                   if (clientId && tenantId && this.federatedTokenFilePath) {

Member

maorleger Apr 17, 2024

clientId and tenantId should always be set by now, you can remove check for them

sdk/identity/identity/src/credentials/workloadIdentityCredential.ts

                     );
+                  } else {
+                    if (clientId && tenantId && this.serviceConnectionId) {

Member

maorleger Apr 17, 2024

clientId and tenantId should always be set by now, you can remove check for them

sdk/identity/identity/src/credentials/workloadIdentityCredential.ts

+                    if (clientId && tenantId && this.serviceConnectionId) {
+                      //Ensure all system env vars are there to form the request uri for OIDC token
+                      this.ensurePipelinesSystemVars();
+                      const oidcRequestUrl = `${process.env.SYSTEM_TEAMFOUNDATIONCOLLECTIONURI}${process.env.SYSTEM_TEAMPROJECTID}/_apis/distributedtask/hubs/build/plans/${process.env.SYSTEM_PLANID}/jobs/${process.env.SYSTEM_JOBID}/oidctoken?api-version=7.1-preview.1&serviceConnectionId=${this.serviceConnectionId}`;

Member

maorleger Apr 17, 2024

Q: should we always hit 7.1-preview.1 API version? If not how do we move forward?

Penguinwizzard reviewed

View reviewed changes

sdk/identity/identity/src/credentials/workloadIdentityCredential.ts

                   this.federatedTokenFilePath =
-                    workloadIdentityCredentialOptions.tokenFilePath || process.env.AZURE_FEDERATED_TOKEN_FILE;
+                    workloadIdentityCredentialOptions?.tokenFilePath || process.env.AZURE_FEDERATED_TOKEN_FILE;
+                  this.serviceConnectionId = workloadIdentityCredentialOptions.serviceConnectionId;

Penguinwizzard Apr 17, 2024

Should we check that this is guid-like?

sdk/identity/identity/src/credentials/workloadIdentityCredential.ts

+                  if (!process.env.SYSTEM_ACCESSTOKEN) missingEnvVars.push("SYSTEM_ACCESSTOKEN");
+                  if (missingEnvVars.length > 0)
+                    throw new CredentialUnavailableError(
+                      `${credentialName}: is unavailable. Missing system variable(s) - ${missingEnvVars.join(

Penguinwizzard Apr 17, 2024 •

edited

Loading

This isn't a great error message - yes. the variables are missing, but we have a good idea as to why they might be, and what the user should do instead. For the first set, you can generate something like The environment variables SYSTEM_JOBID and SYSTEM_PLANID aren't defined - please ensure that you're running this task in an Azure Pipeline.

We can do more for undefined/non-null check for SYSTEM_ACCESSTOKEN with a message like:

Please ensure that the system access token is available in the SYSTEM_ACCESSTOKEN value; this is often most easily achieved by adding a block to the end of your pipeline yaml for the task with:
env:
- SYSTEM_ACCESSTOKEN: $(System.AccessToken)

sdk/identity/identity/src/credentials/workloadIdentityCredentialOptions.ts

               /**
                * Options for the {@link WorkloadIdentityCredential}
                */
-              export interface WorkloadIdentityCredentialOptions
+              export type WorkloadIdentityCredentialOptions =

Penguinwizzard Apr 17, 2024

If we're doing this, why not do it by inheritance?

export interface WorkloadIdentityCredentialOptions
  extends MultiTenantTokenCredentialOptions,
    AuthorityValidationOptions {
    tenantId?: string,
    clientId?: string
}

export interface WorkloadIdentityCredentialKubernetesOptions
  extends WorkloadIdentityCredentialOptions {
    tokenFilePath?: string;
}

export interface WorkloadIdentityCredentialServiceConnectionOptions
  extends WorkloadIdentityCredentialOptions {
    serviceConnectionId?: string;
}

KarishmaGhiya mentioned this pull request

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

Closed

3 tasks

maorleger self-requested a review

April 23, 2024 00:14

KarishmaGhiya mentioned this pull request

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

Merged

5 tasks

Member Author

KarishmaGhiya commented Apr 23, 2024

Closing in favor of #29392

KarishmaGhiya closed this

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

Reviewers

Penguinwizzard Penguinwizzard left review comments

scottaddie scottaddie left review comments

bterlson Awaiting requested review from bterlson bterlson is a code owner

xirzec Awaiting requested review from xirzec

jeremymeng Awaiting requested review from jeremymeng

witemple-msft Awaiting requested review from witemple-msft

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

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

mpodwysocki Awaiting requested review from mpodwysocki mpodwysocki 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

maorleger Awaiting requested review from maorleger maorleger is a code owner

Labels