Cannot use azure.PublicCloud.KeyVaultEndpoint as an OAuth resource

[chessman]

azure.PublicCloud.KeyVaultEndpoint has a trailing slash. If I get authorizer with this resource, key vault client fails. But it works with https://vault.azure.net.

I think, it should be tolerant to trailing slashes.

Reproduction is here: https://github.com/chessman/azure-kv-slash-bug.

[salameer]

Thanks again @chessman, @jhendrixMSFT will be looking into this

[marstr]

Some additional context from a conversation I had with @devigned last night:

The formatting of this string is getting in the way because we are needing to authenticate against a Resource ID, which happens to look like the KeyVault endpoint, but could be any arbitrary string. For that reason, whether or not the slash is included is important information to send over the wire. As per this bug, we seem to be choosing poorly for KeyVault. To make matters somewhat more confusing, other resource IDs seem to match their endpoints.

One thing that should help out this situation is that the 401 that is returned should tell us exactly how to respond.

Now that still leaves us a couple of action items we could pursue here:

• Expose Resource IDs in addition to Endpoints.
• Update our Retry logic to respond to 401s in a manner that corrects for these sorts of mistakes.

[jhendrixMSFT]

This has been resolved in PR Azure/go-autorest#156, with this new functionality you can write the following authenticator.

	kv := keyvault.New()

	auth := autorest.NewBearerAuthorizerCallback(kv.Sender, func(tenantID, resource string) (*autorest.BearerAuthorizer, error) {
		oauthConfig, err := adal.NewOAuthConfig(azure.PublicCloud.ActiveDirectoryEndpoint, tenantID)
		if err != nil {
			return nil, err
		}

		spt, err := adal.NewServicePrincipalToken(*oauthConfig, clientID, "secret", resource)
		if err != nil {
			return nil, err
		}

		return autorest.NewBearerAuthorizer(spt), nil
	})

	kv.Authorizer = auth

[yangl900]

I see this issue has been closed and it will work. However I think the fix rely on client sending a request and get 401 response, then correct the resource string. Why don't we just fix the KV resource string?

KeyVaultEndpoint: "https://vault.azure.net/",

https://github.com/Azure/go-autorest/blob/1f7cd6cfe0adea687ad44a512dfe76140f804318/autorest/azure/environments.go#L70

[jhendrixMSFT]

@yangl900 KeyVaultEndpoint is the URL endpoint, not the resource ID. Sadly they are almost identical which is what made this confusing (and even more confusing, for some RPs the endpoint and resource ID are identical). Removing the trailing '/' would likely break a lot of other code so we can't do that.

[yangl900]

Possible we define separate KeyVaultResourceUri in the SDK? I would be good if we can avoid making 401s. I understand each service may have different authentication logic, some cares about the trailing slash and some doesn't. But I'm hoping for the core Azure services, we just make it right for user.

[jhendrixMSFT]

We could do that, please open an issue in this repo to track that (and of course we accept PRs too :)).

[karataliu]

@jhendrixMSFT @yangl900
Can you take a review: https://github.com/Azure/go-autorest/pull/367/files