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 existing bearer token support to Azure DNS API #5276

Merged
merged 24 commits into from
Nov 13, 2024

Conversation

stbeldarborge
Copy link
Contributor

The Azure DNS API only works with provided service principal credentials, or if running on a resource with a managed identity.
If you want to run it in a pre authenticated context (e.g. on a local machine which is already authenticated with az cli or in a GitHub Action which already has authenticated with azure/login), there's no way to do this.

This PR adds support to skip the authentication by providing the Bearer token.
With az cli the bearer token can be extracted and used in Azure DNS API with this command

export AZUREDNS_BEARERTOKEN=$(az account get-access-token --query accessToken --output tsv)

Copy link

github-actions bot commented Sep 2, 2024

Welcome
First thing: don't send PR to the master branch, please send to the dev branch instead.
Please make sure you've read our DNS API Dev Guide and DNS-API-Test.
Then reply on this message, otherwise, your code will not be reviewed or merged.
We look forward to reviewing your Pull request shortly ✨
注意: 必须通过了 DNS-API-Test 才会被 review. 无论是修改, 还是新加的 dns api, 都必须确保通过这个测试.

@stbeldarborge
Copy link
Contributor Author

Welcome First thing: don't send PR to the master branch, please send to the dev branch instead. Please make sure you've read our DNS API Dev Guide and DNS-API-Test. Then reply on this message, otherwise, your code will not be reviewed or merged. We look forward to reviewing your Pull request shortly ✨ 注意: 必须通过了 DNS-API-Test 才会被 review. 无论是修改, 还是新加的 dns api, 都必须确保通过这个测试.

PR to dev, and code tested in forked repo.

@stbeldarborge
Copy link
Contributor Author

stbeldarborge commented Sep 3, 2024

While #4981 is similar, these two are not the same, as #4981 does an authentication, while this PR supports authentication outside of the acme.sh context.

@stbeldarborge
Copy link
Contributor Author

stbeldarborge commented Sep 20, 2024

@Neilpang I've ran it through shfmt locally now, so hopefully it passes shfmt this time, if you can re run the workflows please

@Neilpang
Copy link
Member

@stbeldarborge
Copy link
Contributor Author

Now that all checks have passed, can you merge it @Neilpang ? :)

@stbeldarborge
Copy link
Contributor Author

@Neilpang done!
https://github.com/stbeldarborge/acme.sh/actions/runs/11272323811

Sorry, I did not understand the DNS API test documentation earlier, I thought I'd done it.
And I misunderstood some of the secrets, so that's why there are so many commits, because I had to debug what was happening.. You could squashmerge it 😅

@maonat
Copy link

maonat commented Oct 21, 2024

@stbeldarborge Have you tested this with federated credentials on service principals + GitHub actions? I need to use acme with service principals keyless on github workflows. I'm about to test this but it might take a while.

Thanks for this PR!

Edit:
Just tried with federated credential and did work just fine.

@stbeldarborge
Copy link
Contributor Author

absolutely, that's the exact usecase I have as well and the reason why I created this 😃 but would work with any context as long as it's authenticated to ARM

@Neilpang sorry to keep bothering you, but DNS API test succeeded, and it's in "production" even for others now like @maonat.. would be nice if it could get into the official release 👼

@Neilpang
Copy link
Member

Neilpang commented Nov 3, 2024

@maonat
Copy link

maonat commented Nov 4, 2024

@Neilpang isn't this related to something else and not to the code that was implemented by @stbeldarborge? He simply merged master from this repo to his fork and that has nothing to do with his changes.

Edit: Check out... I've forked @stbeldarborge work on the commit before his latest master merge and got the following result:
CleanShot 2024-11-04 at 19 20 33@2x
The only error I get it from the DNS workflow because I'm getting an error for missing secrets (which is absolutely normal because I don't have those DNS secrets)

@EldarBorge
Copy link

@Neilpang like I mentioned, it does pass the DNS test; https://github.com/stbeldarborge/acme.sh/actions/runs/11272323811

However, you probably ran it without providing a token or a secret or any other authentication to Azure? If you ran the same workflow without authentication to Azure on master, that would fail too.

@Neilpang
Copy link
Member

Neilpang commented Nov 4, 2024

I didn't/couldnt' run the dns test , because I don't have Azure account.
what I meant was that the latest run in your fork was not success:
https://github.com/stbeldarborge/acme.sh/actions/runs/11287963136

please fix it.

@maonat
Copy link

maonat commented Nov 6, 2024

@Neilpang Here I've ran the test on my repository I forked from @stbeldarborge repository.
Here is the completed and "all green" workflow
I cannot get in touch with them nor can make the run on their repository, would this test be ok for you?

@stbeldarborge
Copy link
Contributor Author

stbeldarborge commented Nov 12, 2024

@Neilpang latest run is green; https://github.com/stbeldarborge/acme.sh/actions

@maonat yeah, sorry, had other matters to attend to, but at least I've configured the secrets and re runned it now!

This should prove the code is good to merge hopefully? 🙏

@Neilpang
Copy link
Member

would any of you guys update the usage here?

https://github.com/acmesh-official/acme.sh/wiki/dnsapi#dns_azure

@stbeldarborge
Copy link
Contributor Author

Absolutely! Updated the wiki now. 🙂

@Neilpang
Copy link
Member

write code here:
image

@stbeldarborge
Copy link
Contributor Author

Done!

@Neilpang Neilpang merged commit 532b425 into acmesh-official:dev Nov 13, 2024
13 checks passed
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.

4 participants