feat: Add support for GitHub Actions OIDC via oidc_request_url and oidc_request_token fields #421
Conversation
JenGoldstrich
changed the title
JenGoldstrich
force-pushed
the
github_oidc
branch
from
8e955be to
9a63f91
Compare
…st pass through these values to be consistent, its feasible that someone would run the chroot builder in GHA to be fair, but I am not spending too much time testing it
JenGoldstrich
force-pushed
the
github_oidc
branch
from
f19e9e3 to
b1d6ef9
Compare
JenGoldstrich
changed the title
JenGoldstrich
changed the title
| @@ -98,6 +98,8 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook) | |||
| ClientCertPassword: b.config.ClientConfig.ClientCertPassword, | |||
| TenantID: b.config.ClientConfig.TenantID, | |||
| SubscriptionID: b.config.ClientConfig.SubscriptionID, | |||
| OidcRequestUrl: b.config.ClientConfig.OidcRequestURL, | |||
| OidcRequestToken: b.config.ClientConfig.OidcRequestToken, | |||
There was a problem hiding this comment.
I wonder, why aren't we using the azure_client_set.go's New function for this? It seems those options are reified in this place
There was a problem hiding this comment.
azure_client_set is only used by chroot, each builder needs to pass the options into the AuthOptions block
There was a problem hiding this comment.
This is slightly confusing given the fact that it was created in the common package, I imagine the idea was to eventually migrate to that but I kind of like having the clients separate personally
There was a problem hiding this comment.
I guess given both builders are using the same SDK, if the code is the same, might as well use the common implementation. Or if there's a good reason to keep them separate, drop the common one as it's not used
There was a problem hiding this comment.
That makes sense to me, like we talked about off GitHub I'll follow up with this change on another PR, thanks for this suggestion, we'll unify everything to use the common azure client
Add
oidc_request_urlandoidc_request_tokenfields, I used the same field names https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/guides/service_principal_oidc here in the TF Provider, but its worth noting that these fields are primarily for GitHub OIDC authentication, if other OIDC providers mirror GH's OIDC setup this should work but it is mainly supported from an sdk perspective on GitHubCloses #385 -> This issue had to do with OIDC token expiration, from chatting with the go-azure-sdk maintainers this is cause we were providing a one time JWT, rather than providing the authorizer with the URL/request token that allows them to request their own access tokens, so the SDK couldn't refresh the token, example run in CI here https://github.com/hashicorp/packer-plugin-azure/actions/runs/9370160599/job/25796435754
Because the client config was shared with the chroot builder and DTL builder it made sense to just add support for this to the those builders too, but I did not heavily test it due to the relatively much lower usage of DTL/chroot