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 new format for AKS OIDC issuer #971

Merged
merged 2 commits into from
Jan 24, 2023
Merged

Add new format for AKS OIDC issuer #971

merged 2 commits into from
Jan 24, 2023

Conversation

yorinasub17
Copy link
Contributor

@yorinasub17 yorinasub17 commented Jan 20, 2023

Closes #970

Summary

This adds the newly detected OIDC Issuer URL format for AKS clusters as an additional meta issuer. I added a new one instead of replacing the existing one in case that issuer URL is being used under different circumstances.

Release Note

  • Support the new format of AKS OIDC Issuers

Documentation

N/A

@@ -52,6 +52,10 @@ data:
"https://oidc.prod-aks.azure.com/*": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any documentation from Azure about this? Is there a migration ongoing? I would assume this has to do with providing regionalized OIDC providers.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few references in the Azure org to this new URL - https://cs.github.com/?scopeName=All+repos&scope=&q=oic.prod-aks.azure.com+org%3Aazure

Contrast to https://cs.github.com/?scopeName=All+repos&scope=&q=https%3A%2F%2Foidc.prod-aks.azure.com+org%3Aazure, with more references in code.

I can't find any public documentation about this, but seems fine to me.

Copy link
Contributor Author

@yorinasub17 yorinasub17 Jan 20, 2023

Choose a reason for hiding this comment

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

Unfortunately I couldn't find any documentation on this. I actually don't have much more information than what you could find (I'm not associated with Azure/Microsoft).

The only thing I do know is that:

  • I created the AKS cluster on December 20th in the East US region, following the same steps referenced in Add AKS as a meta issuer #384 (these docs)
  • The OIDC issuer URL that I got is different from what's currently configured: https://eastus.oic.prod-aks.azure.com/****
  • When I tried to use cosign in keyless mode with the projected service account token, it failed with a 401, and I suspected this was the reason.

I posted a question on their discussion forum (https://learn.microsoft.com/en-us/answers/questions/1162598/what-is-the-canonical-format-of-the-oidc-issuer-ur) asking about this. Hopefully someone from Microsoft can answer and give us some concrete info we can rely on!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also posted on their main AKS issue for the feature (since that pings the right team): Azure/AKS#1480 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPDATE: I got a response from Microsoft support on the learn Q&A ticket which isn't definitive, but is another data point that the format appears to be https://REGION.oic.prod-aks.azure.com/***.

Is that sufficient data for adding this in? If not, is there something else I can do to unblock myself (e.g., is there a way for me to run a custom fulcio instance and link with the public rekor. I think in theory that's possible but not sure...)?

FWIW, I've also opened a ticket on the Microsoft docs page requesting for the format to be documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is, thanks for checking!
Can you fix the failing test? I believe you need to update another file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah didn't realize I needed to update another place! Done!

Can you kick off another build please? Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2023

Codecov Report

Merging #971 (3289328) into main (ce7e325) will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #971      +/-   ##
==========================================
+ Coverage   53.37%   53.50%   +0.12%     
==========================================
  Files          37       37              
  Lines        2325     2325              
==========================================
+ Hits         1241     1244       +3     
+ Misses        992      990       -2     
+ Partials       92       91       -1     
Impacted Files Coverage Δ
pkg/ca/fileca/load.go 68.96% <0.00%> (+10.34%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>
…erated

Signed-off-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>
@yorinasub17
Copy link
Contributor Author

@haydentherapper Looks like the build failed again, and I see what I did wrong, so this time I generated the file locally and pushed the diff (3289328). I believe this should cause the test to pass now!

@haydentherapper
Copy link
Contributor

Thanks! I'll get this checked in and rolled out to prod soon after.

@haydentherapper haydentherapper merged commit 16b975b into sigstore:main Jan 24, 2023
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.

AKS issuer URL is different from configured format
3 participants