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

auth: Implementation fo AzureSub provider that leverages federated credentials #1723

Merged
merged 2 commits into from
May 6, 2024

Conversation

nturinski
Copy link
Member

I've explained this quite a bit offline, but just some context for the PR.

We are using federated credentials (service principals) in order to log in to Azure. This token is used in place of VS Code sessions and passed to our subscription client. The resources extension will need to swap which sub provider it is using based on whether or not it is in a testing environment, but there should be no code changes beyond that.

Still need to add the README. We should translate Hossam's document from OneNote to markdown and include it as a README.

@nturinski nturinski requested a review from a team as a code owner May 6, 2024 18:18
alexweininger
alexweininger previously approved these changes May 6, 2024
auth/src/AzureDevOpsSubscriptionProvider.ts Show resolved Hide resolved

public async getTenants(): Promise<TenantIdDescription[]> {
return [{
tenantId: this._tokenCredential?.tenantId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there can only ever be one tenant associated with the token? I also don't see where the tenantId property is ever set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it probably can have multiple tenants, but right now it doesn't. Right now the token being used only returns one subscription back, so we're just using that tenant automatically.

I could add support now, though might be easier for me to validate with Megan's tenant work.

Comment on lines +162 to +163
public onDidSignIn: Event<void> = () => { return new Disposable(() => { /*empty*/ }) };
public onDidSignOut: Event<void> = () => { return new Disposable(() => { /*empty*/ }) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably add support for these pretty easily

@nturinski nturinski merged commit 3df656f into main May 6, 2024
4 checks passed
@nturinski nturinski deleted the auth/nat/automatedProvider branch May 6, 2024 22:55
`);
}

const oidcRequestUrl = `${teamFoundationCollectionUri}${teamProjectId}/_apis/distributedtask/hubs/build/plans/${planId}/jobs/${jobId}/oidctoken?api-version=7.1-preview.1&serviceConnectionId=${serviceConnectionId}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: How did you figure out the exact format for the oidcRequestUrl? Was this referenced in the MS Learn docs somewhere or by manually inspecting a job that was spun up?

Copy link
Contributor

@MicroFish91 MicroFish91 May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nevermind, disregard I'm assuming it was pulled from here:
https://learn.microsoft.com/en-us/rest/api/azure/devops/distributedtask/oidctoken/create

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. We might want to add that link to a comment above this.

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

Successfully merging this pull request may close these issues.

None yet

4 participants