Support Azure CLI (az) version 2.73.0 and newer.#55374
Conversation
| } | ||
|
|
||
| func (m *AzureMSIMiddleware) msiEndpoint(rw http.ResponseWriter, req *http.Request) error { | ||
| func (m *AzureTokenMiddleware) handleEndpoint(rw http.ResponseWriter, req *http.Request, identityRequest bool) error { |
There was a problem hiding this comment.
As far as I can tell identityRequest is true when we are serving the updated protocol. The name doesn't quite reflect that, so perhaps we can rename is slightly to clarify that fact and add a comment about it as well.
There was a problem hiding this comment.
alternatively, for better readability, avoid the bool and make two separate functions like handleMSIEndpoint, handleIdentityEndpoint.
There was a problem hiding this comment.
I've updated this function to receive the field name and secret, so we don't need any conditions for this function.
| } | ||
|
|
||
| endpoint := os.Getenv("MSI_ENDPOINT") | ||
| if endpoint == "" { |
There was a problem hiding this comment.
Should we look for IDENTITY_ENDPOINT too? I know we will overwrite it, but perhaps checking its existence might be worth it for debugging purposes?
There was a problem hiding this comment.
Having only IDENTITY_HEADER set shouldn't be effective (as per MSAL definition). So I updated this function to follow this too.
greedy52
left a comment
There was a problem hiding this comment.
Thanks for the quick fix! lets also update the docs (here or separately)
| } | ||
|
|
||
| func (m *AzureMSIMiddleware) msiEndpoint(rw http.ResponseWriter, req *http.Request) error { | ||
| func (m *AzureTokenMiddleware) handleEndpoint(rw http.ResponseWriter, req *http.Request, identityRequest bool) error { |
There was a problem hiding this comment.
alternatively, for better readability, avoid the bool and make two separate functions like handleMSIEndpoint, handleIdentityEndpoint.
| // TODO(gabrielcorado): use the cluster name. This value must match the | ||
| // one used by the proxy. |
There was a problem hiding this comment.
i doubt we will ever fix this. if we do, how do we plan to preserve backwards compatibility? is this jwt verification even necessary?
There was a problem hiding this comment.
I haven't looked into too many details on this, either. I just updated the comment because now it is confusing why we use the MSI endpoint here, so I wanted to clarify that it is just a placeholder.
We can leave here and look later. If the JWT verification is not required, we can also simplify this code.
There was a problem hiding this comment.
I'd drop the TODO. Using the cluster name would be helpful for validation purposes, but the other validation element is the CA that signed the JWT. It is safe to assume that CA is cluster-specific, so we don't necessarily need the cluster name to be different; it won't validate in the other cluster.
|
@gabrielcorado See the table below for backport results.
|
* refactor(tsh): support `az login` with MSAL * chore(tsh): fix lint * refactor: code review suggestions * test(tsh): fix wrong env names
* refactor(tsh): support `az login` with MSAL * chore(tsh): fix lint * refactor: code review suggestions * test(tsh): fix wrong env names
* refactor(tsh): support `az login` with MSAL * chore(tsh): fix lint * refactor: code review suggestions * test(tsh): fix wrong env names
* refactor(tsh): support `az login` with MSAL * chore(tsh): fix lint * refactor: code review suggestions * test(tsh): fix wrong env names
Related to #55298
After version
2.73.0of theaz(Azure CLI), the managed identity flows were updated to use Microsoft Authentication Library (MSAL) by default.Effective for our usage, that meant two breaking changes happened:
az login--username/-uflag was deprecated when using managed identities (--identity). We should use--resource-idinstead.MSI_ENDPOINTis set, instead it also requires aMSI_SECRETset, however the new implementation is not backwards compatible and lacks documentation. Instead of using MSI endpoint, we can rely in the Identity (App Service) endpoint which is better documented and supported (for example, by Azure functions). This API works the same way as the MSI one, with the exception of a few fields that were renamed.To support the newer version of
azCLI, we've updated:--resource-idflag when on a newer version of the CLI. Otherwise, fallback into using the--username/-uflag.AzureMSIMiddleware(renamed toAzureTokenMiddleware) to be able to process Identity endpoint requests. There was no flow change, only a few fields changed and tests were added.Most of this PR changes are in the test files, which were updated to test the usage of both MSI and Identity endpoints.
changelog: Added support to
tshApp Access commands for Azure CLI (az) version2.73.0and newer.