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

Add federated credentials instructions #1207

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NanaXiong00
Copy link

Overview/Summary

Add federated credentials instructions.

This PR fixes/adds/changes/removes

Add secret setup instructions for SP using federated credentials.

Breaking Changes

none

As part of this Pull Request I have

  • Read the Contribution Guide and ensured this PR is compliant with the guide
  • Checked for duplicate Pull Requests
  • Associated it with relevant GitHub Issues or ADO Work Items (Internal Only)
  • Ensured my code/branch is up-to-date with the latest changes in the main branch
  • Ensured PR tests are passing
  • Updated relevant and associated documentation (e.g. Contribution Guide, Docs etc.)

@vhvb1989 for notification.

@NanaXiong00 NanaXiong00 requested a review from a team as a code owner July 18, 2024 10:42
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Triage 🔍 Maintainers need to triage still label Jul 18, 2024
Copy link
Contributor

@AlexanderSehr AlexanderSehr left a comment

Choose a reason for hiding this comment

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

Presumably, this PR is dependent on Azure/bicep-registry-modules#2701

Just creating this comment to make sure the PR is not accidently merged :)

cc: @eriqua

@v-xuto
Copy link
Member

v-xuto commented Jul 19, 2024

@vhvb1989 Please help review this PR.

@AlexanderSehr AlexanderSehr added Type: Documentation 📄 Improvements or additions to documentation Language: Bicep 💪 This is related to the Bicep IaC language labels Jul 19, 2024
@vhvb1989
Copy link
Member

Presumably, this PR is dependent on Azure/bicep-registry-modules#2701

Just creating this comment to make sure the PR is not accidently merged :)

cc: @eriqua

@AlexanderSehr , Azure/bicep-registry-modules#2701 seems weird. It looks like it is creating a list of modules which takes too long for OIDC. I assume that's because the token gets expired and azure/login@v2 doesn't auto-refresh?

@AlexanderSehr
Copy link
Contributor

Presumably, this PR is dependent on Azure/bicep-registry-modules#2701
Just creating this comment to make sure the PR is not accidently merged :)
cc: @eriqua

@AlexanderSehr , Azure/bicep-registry-modules#2701 seems weird. It looks like it is creating a list of modules which takes too long for OIDC. I assume that's because the token gets expired and azure/login@v2 doesn't auto-refresh?

'Weird' is a strong word 😄 It's a draft PR and @eriqua is working hard to get OIDC working in it 😉

And yes, it seems that if you use a Service Principal for federated credentials, you run into a timeout after 1h. However, this may not be the same if using a managed identity which is being tested in a different branch.

I don't have the details as I've not been working on the code myself, so let's give @eriqua some time to figure the best option out 💪

@vhvb1989
Copy link
Member

Presumably, this PR is dependent on Azure/bicep-registry-modules#2701
Just creating this comment to make sure the PR is not accidently merged :)
cc: @eriqua

@AlexanderSehr , Azure/bicep-registry-modules#2701 seems weird. It looks like it is creating a list of modules which takes too long for OIDC. I assume that's because the token gets expired and azure/login@v2 doesn't auto-refresh?

'Weird' is a strong word 😄 It's a draft PR and @eriqua is working hard to get OIDC working in it 😉

And yes, it seems that if you use a Service Principal for federated credentials, you run into a timeout after 1h. However, this may not be the same if using a managed identity which is being tested in a different branch.

I don't have the details as I've not been working on the code myself, so let's give @eriqua some time to figure the best option out 💪

You are totally right. I shouldn't have said weird. My apologies.

I've added some comments to that PR. I think you pointed out the same thing that I was curious about. The use of OIDC and client-secret for the same gh-action called my attention as an interesting approach. I wonder if there could be a way to use only OIDC for all.

@AlexanderSehr
Copy link
Contributor

Presumably, this PR is dependent on Azure/bicep-registry-modules#2701
Just creating this comment to make sure the PR is not accidently merged :)
cc: @eriqua

@AlexanderSehr , Azure/bicep-registry-modules#2701 seems weird. It looks like it is creating a list of modules which takes too long for OIDC. I assume that's because the token gets expired and azure/login@v2 doesn't auto-refresh?

'Weird' is a strong word 😄 It's a draft PR and @eriqua is working hard to get OIDC working in it 😉
And yes, it seems that if you use a Service Principal for federated credentials, you run into a timeout after 1h. However, this may not be the same if using a managed identity which is being tested in a different branch.
I don't have the details as I've not been working on the code myself, so let's give @eriqua some time to figure the best option out 💪

You are totally right. I shouldn't have said weird. My apologies.

I've added some comments to that PR. I think you pointed out the same thing that I was curious about. The use of OIDC and client-secret for the same gh-action called my attention as an interesting approach. I wonder if there could be a way to use only OIDC for all.

Hey @vhvb1989,
there absolutely is. As said, @eriqua was working on the topic in the background and opened this draft PR after working her way through some misleading docs and performing some much needed tests: Azure/bicep-registry-modules#2757

Aside from some AZ CLI issue that causes the logic to fail refreshing the token, it seems to work very well with a managed identity.

We had a longer chat about the matter yesterday, and it's likely that we'll merge the change in as soon as @eriqua is back (she's currenlty out of office) and enable both the current credential-based as well as the new OIDC approach for authentication (to avoid breaking changes etc.) and after a grace period cut over completely 💪

@eriqua
Copy link
Contributor

eriqua commented Aug 31, 2024

Moving to draft as this would require alignment with the actual implementation (still under testing). Ref Azure/bicep-registry-modules#2792 for latest development

@eriqua eriqua marked this pull request as draft August 31, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language: Bicep 💪 This is related to the Bicep IaC language Needs: Triage 🔍 Maintainers need to triage still Type: Documentation 📄 Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants