-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[identity] Support Federated Identity for Service connections - new credential #29392
Changes from all commits
8413a7a
ed09ef6
8143362
b947d1d
88eaf27
20b337f
a40dcd5
41f1d6f
515d715
add42df
adb3ce4
d1b03bb
d60c560
b2fb6fd
9ea3e81
7851664
987b0c6
03747da
c909ac4
da72605
8f80a2f
7b7a949
f5e5bee
320b1ee
74fa416
42742e4
f91d627
f66b82f
4a700e9
606d70e
5075748
9b39405
45da28e
9b42986
e084c7b
1ca059d
07d908e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,180 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { AccessToken, GetTokenOptions, TokenCredential } from "@azure/core-auth"; | ||
import { ClientAssertionCredential } from "./clientAssertionCredential"; | ||
import { AuthenticationError, CredentialUnavailableError } from "../errors"; | ||
import { credentialLogger } from "../util/logging"; | ||
import { checkTenantId } from "../util/tenantIdUtils"; | ||
import { | ||
createDefaultHttpClient, | ||
createHttpHeaders, | ||
createPipelineRequest, | ||
} from "@azure/core-rest-pipeline"; | ||
import { AzurePipelinesServiceConnectionCredentialOptions } from "./azurePipelinesServiceConnectionCredentialOptions"; | ||
|
||
const credentialName = "AzurePipelinesServiceConnectionCredential"; | ||
const OIDC_API_VERSION = "7.1"; | ||
const logger = credentialLogger(credentialName); | ||
|
||
/** | ||
* This credential is designed to be used in ADO Pipelines with service connections | ||
* as a setup for workload identity federation. | ||
*/ | ||
export class AzurePipelinesServiceConnectionCredential implements TokenCredential { | ||
private clientAssertionCredential: ClientAssertionCredential | undefined; | ||
private serviceConnectionId: string | undefined; | ||
|
||
/** | ||
* AzurePipelinesServiceConnectionCredential supports Federated Identity on Azure Pipelines through Service Connections. | ||
* @param tenantId - tenantId associated with the service connection | ||
* @param clientId - clientId associated with the service connection | ||
* @param serviceConnectionId - id for the service connection | ||
* @param options - The identity client options to use for authentication. | ||
*/ | ||
constructor( | ||
tenantId: string, | ||
clientId: string, | ||
serviceConnectionId: string, | ||
options?: AzurePipelinesServiceConnectionCredentialOptions, | ||
) { | ||
if (!clientId || !tenantId || !serviceConnectionId) { | ||
throw new CredentialUnavailableError( | ||
`${credentialName}: is unavailable. tenantId, clientId, and serviceConnectionId are required parameters.`, | ||
); | ||
} | ||
|
||
checkTenantId(logger, tenantId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we throw if clientId is nullish? Same for tenantId? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the constructor won't allow a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also tenantId validation is done by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah 100% - for TypeScript users the type system will not allow null / undefined values there. But I think for JS users we should still have runtime validation. What do you think? Do other credentials throw if clientId or tenantid are nullish?
Right, that makes sense! The error message in that function is pretty clear as well. So just clientId should be validated I suppose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not blocking this PR if you prefer to make an issue and get a dev-build going |
||
logger.info( | ||
`Invoking AzurePipelinesServiceConnectionCredential with tenant ID: ${tenantId}, clientId: ${clientId} and service connection id: ${serviceConnectionId}`, | ||
); | ||
|
||
if (clientId && tenantId && serviceConnectionId) { | ||
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=${OIDC_API_VERSION}&serviceConnectionId=${this.serviceConnectionId}`; | ||
const systemAccessToken = `${process.env.SYSTEM_ACCESSTOKEN}`; | ||
logger.info( | ||
`Invoking ClientAssertionCredential with tenant ID: ${tenantId}, clientId: ${clientId} and service connection id: ${serviceConnectionId}`, | ||
); | ||
this.clientAssertionCredential = new ClientAssertionCredential( | ||
tenantId, | ||
clientId, | ||
this.requestOidcToken.bind(this, oidcRequestUrl, systemAccessToken), | ||
options, | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Authenticates with Microsoft Entra ID and returns an access token if successful. | ||
* If authentication fails, a {@link CredentialUnavailableError} or {@link AuthenticationError} will be thrown with the details of the failure. | ||
* | ||
* @param scopes - The list of scopes for which the token will have access. | ||
* @param options - The options used to configure any requests this | ||
* TokenCredential implementation might make. | ||
*/ | ||
public async getToken( | ||
scopes: string | string[], | ||
options?: GetTokenOptions, | ||
): Promise<AccessToken> { | ||
if (!this.clientAssertionCredential) { | ||
const errorMessage = `${credentialName}: is unavailable. tenantId, clientId, and serviceConnectionId are required parameters. | ||
To use Federation Identity in Azure Pipelines, these are required as inputs / env variables - | ||
tenantId, | ||
clientId, | ||
serviceConnectionId, | ||
"SYSTEM_TEAMFOUNDATIONCOLLECTIONURI" && | ||
"SYSTEM_TEAMPROJECTID" && | ||
"SYSTEM_PLANID" && | ||
"SYSTEM_JOBID" && | ||
"SYSTEM_ACCESSTOKEN" | ||
See the troubleshooting guide for more information: https://aka.ms/azsdk/js/identity/troubleshoot`; | ||
logger.error(errorMessage); | ||
throw new CredentialUnavailableError(errorMessage); | ||
} | ||
logger.info("Invoking getToken() of Client Assertion Credential"); | ||
return this.clientAssertionCredential.getToken(scopes, options); | ||
} | ||
|
||
/** | ||
* | ||
* @param oidcRequestUrl - oidc request url | ||
* @param systemAccessToken - system access token | ||
* @returns OIDC token from Azure Pipelines | ||
*/ | ||
private async requestOidcToken( | ||
oidcRequestUrl: string, | ||
systemAccessToken: string, | ||
): Promise<string> { | ||
logger.info("Requesting OIDC token from Azure Pipelines..."); | ||
logger.info(oidcRequestUrl); | ||
|
||
const httpClient = createDefaultHttpClient(); | ||
|
||
const request = createPipelineRequest({ | ||
url: oidcRequestUrl, | ||
method: "POST", | ||
headers: createHttpHeaders({ | ||
"Content-Type": "application/json", | ||
Authorization: `Bearer ${systemAccessToken}`, | ||
}), | ||
}); | ||
|
||
const response = await httpClient.sendRequest(request); | ||
const text = response.bodyAsText; | ||
if (!text) { | ||
throw new AuthenticationError( | ||
response.status, | ||
`${credentialName}: Authenticated Failed. Received null token from OIDC request.`, | ||
); | ||
} | ||
const result = JSON.parse(text); | ||
if (result?.oidcToken) { | ||
return result.oidcToken; | ||
} else { | ||
throw new AuthenticationError( | ||
response.status, | ||
`${credentialName}: Authentication Failed. oidcToken field not detected in the response. Response = ${JSON.stringify( | ||
result, | ||
)}`, | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Ensures all system env vars are there to form the request uri for OIDC token | ||
* @returns void | ||
* @throws CredentialUnavailableError | ||
*/ | ||
private ensurePipelinesSystemVars(): void { | ||
if ( | ||
process.env.SYSTEM_TEAMFOUNDATIONCOLLECTIONURI && | ||
process.env.SYSTEM_TEAMPROJECTID && | ||
process.env.SYSTEM_PLANID && | ||
process.env.SYSTEM_JOBID && | ||
process.env.SYSTEM_ACCESSTOKEN | ||
) { | ||
return; | ||
} | ||
const missingEnvVars = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit (feel free to ignore): a loop would be easier to follow and modify in the future Pseudocode:
|
||
let errorMessage = ""; | ||
if (!process.env.SYSTEM_TEAMFOUNDATIONCOLLECTIONURI) { | ||
missingEnvVars.push("SYSTEM_TEAMFOUNDATIONCOLLECTIONURI"); | ||
} | ||
if (!process.env.SYSTEM_TEAMPROJECTID) missingEnvVars.push("SYSTEM_TEAMPROJECTID"); | ||
if (!process.env.SYSTEM_PLANID) missingEnvVars.push("SYSTEM_PLANID"); | ||
if (!process.env.SYSTEM_JOBID) missingEnvVars.push("SYSTEM_JOBID"); | ||
if (!process.env.SYSTEM_ACCESSTOKEN) { | ||
errorMessage += | ||
"\nPlease 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:\n env: \n- SYSTEM_ACCESSTOKEN: $(System.AccessToken)"; | ||
missingEnvVars.push("SYSTEM_ACCESSTOKEN"); | ||
} | ||
if (missingEnvVars.length > 0) { | ||
throw new CredentialUnavailableError( | ||
`${credentialName}: is unavailable. Ensure that you're running this task in an Azure Pipeline, so that following missing system variable(s) can be defined- ${missingEnvVars.join( | ||
", ", | ||
)}.${errorMessage}`, | ||
); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { AuthorityValidationOptions } from "./authorityValidationOptions"; | ||
import { CredentialPersistenceOptions } from "./credentialPersistenceOptions"; | ||
import { MultiTenantTokenCredentialOptions } from "./multiTenantTokenCredentialOptions"; | ||
|
||
/** | ||
* Optional parameters for the {@link AzurePipelinesServiceConnectionCredential} class. | ||
*/ | ||
export interface AzurePipelinesServiceConnectionCredentialOptions | ||
extends MultiTenantTokenCredentialOptions, | ||
CredentialPersistenceOptions, | ||
AuthorityValidationOptions {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { AzurePipelinesServiceConnectionCredential } from "../../../src"; | ||
import { env, isLiveMode } from "@azure-tools/test-recorder"; | ||
import { assert } from "@azure-tools/test-utils"; | ||
|
||
describe("AzurePipelinesServiceConnectionCredential", function () { | ||
const scope = "https://vault.azure.net/.default"; | ||
const tenantId = env.IDENTITY_SP_TENANT_ID || env.AZURE_TENANT_ID!; | ||
// const clientId = env.IDENTITY_SP_CLIENT_ID || env.AZURE_CLIENT_ID!; | ||
|
||
it("authenticates with a valid service connection", async function () { | ||
if (!isLiveMode()) { | ||
this.skip(); | ||
} | ||
// this serviceConnection corresponds to the Azure SDK Test Resources - LiveTestSecrets service | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
const existingServiceConnectionId = "0dec29c2-a766-4121-9c2e-1894f5aca5cb"; | ||
// clientId for above service connection | ||
const clientId = "203c27cb-6778-4ecc-9bfd-9f03a61f3408"; | ||
const credential = new AzurePipelinesServiceConnectionCredential( | ||
tenantId, | ||
clientId, | ||
existingServiceConnectionId, | ||
); | ||
try { | ||
const token = await credential.getToken(scope); | ||
assert.ok(token?.token); | ||
assert.isDefined(token?.expiresOnTimestamp); | ||
if (token?.expiresOnTimestamp) assert.ok(token?.expiresOnTimestamp > Date.now()); | ||
} catch (e) { | ||
console.log(e); | ||
} | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think that
AzurePipelinesCredential
with a constructor that takes a requiredserviceConnectionId
should be in consideration.I know we've gone back and forth on this design and will support this current design, but I think
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I am proposing renaming this credential to AzurePipelineCredential -otherwise the constructor LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plural :D