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

Successfully Authenticate AzureChina with an Azure Public Credential #18508

Closed
xqi-aviatrix opened this issue Jun 30, 2022 · 9 comments
Closed
Assignees
Labels
Azure.Identity customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-author-feedback Workflow: More information is needed from author to address the issue. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@xqi-aviatrix
Copy link

func NewClientSecretCredential(tenantID string, clientID string, clientSecret string, options *ClientSecretCredentialOptions) (*ClientSecretCredential, error) {

I try to authenticate Azure_China with an Azure_Public credential. I expect authentication errors returned, but no error returns and authentication succeeded as if the authentication endpoint was Azure_Public

Screen Shot 2022-06-30 at 4 43 33 PM

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jun 30, 2022
@chlowell chlowell self-assigned this Jun 30, 2022
@chlowell
Copy link
Member

chlowell commented Jul 1, 2022

Authentication happens in the credential's GetToken method, not its constructor, so you won't see an error in a case like this until the first GetToken call. I don't see anything in your screenshot that would call GetToken. Does your program work when you add this?

import "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"

// normally you don't need to think about this, because
// Azure SDK clients call GetToken automatically as needed
tokenOptions := policy.TokenRequestOptions{
    Scopes: []string{"https://management.core.chinacloudapi.cn//.default",
}
tk, err := cred.GetToken(ctx, tokenOptions)
if err != nil {
    // client IDs and secrets are validated by Azure AD.
    // If they're incorrect, you'll land here.
}

@chlowell chlowell added Azure.Identity needs-author-feedback Workflow: More information is needed from author to address the issue. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Jul 1, 2022
@xqi-aviatrix
Copy link
Author

xqi-aviatrix commented Jul 1, 2022

Thanks for the quick reply. Yes, after adding the code above, I got the error stating the application cannot be found

However, In my original code I could use the new created cred to create a client like below and then read resource under Azure public with the client, althought the credential is created with Azure China option. Not sure why the clients (I tried armcompute.VirtualMachinesClient and armmonitor.ActivityLogsClient) skip the checkings

  `credential_options := &azidentity.ClientSecretCredentialOptions{ClientOptions: policy.ClientOptions{Cloud: cloud.AzureChina}}
cred, err := azidentity.NewClientSecretCredential(
	tenant_id_auzre_public,
	client_id_auzre_public,
	client_secret_auzre_public,
	credential_options)
if err != nil {
	log.Fatalf("Authentication Failure: %#v", err)
	return
}
ctx := context.Background()
    alClient, err := armmonitor.NewActivityLogsClient(currentSubscription, cred, nil)
if err != nil {
	log.Fatalf("activity log client creation failed %s", err)
	return
}`

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Jul 1, 2022
@chlowell
Copy link
Member

chlowell commented Jul 1, 2022

Clients are similar to credentials in that they don't attempt to authenticate until necessary. You won't get an error about your invalid service principal configuration until you call a method that sends a request to the service (methods that do this have a Context parameter).

@chlowell chlowell added needs-author-feedback Workflow: More information is needed from author to address the issue. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Jul 1, 2022
@xqi-aviatrix
Copy link
Author

xqi-aviatrix commented Jul 1, 2022

I also try the method that have Context parameter (e.g. NextPage method) from the client created with this problematic cred. It returns Azure Public resource without returning error
is it the same method with Context parameter as you mentioned? I am sure the method is called since 429 events returned in my code and they are distributed across 3 pages

`var selectFilter string = "eventTimestamp,operationName,resourceId,resourceType,resourceGroupName,status,level,category"

eventsCollectionPager := alClient.NewListPager("eventTimestamp ge 2022-06-27T00:00:00.6407898Z",
	&armmonitor.ActivityLogsClientListOptions{Select: &selectFilter})

totalEventCnt := 0

eventListStack := make([]*armmonitor.ActivityLogsClientListResponse, 0)

for eventsCollectionPager.More() {
	eventList, err := eventsCollectionPager.NextPage(ctx)
	if err != nil {
		log.Fatalf("activity log event next page failed %s", err)
	}
	eventListStack = append(eventListStack, &eventList)
}`

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Jul 1, 2022
@xqi-aviatrix
Copy link
Author

xqi-aviatrix commented Jul 1, 2022

I also try VirualMachinesClient which has method with Context parameter directly (e.g vmClient.Get). The same behavior that Azure public resources returned but not error returned

`
vmClient, err := armcompute.NewVirtualMachinesClient(currentSubscription, cred, nil)

if err != nil {
log.Fatalf("virtual machine client creation failed %s", err)
return
}

statusOnly := "true"
var instanceViewExpand armcompute.InstanceViewTypes = "instanceView"
pager := vmClient.NewListAllPager(&armcompute.VirtualMachinesClientListAllOptions{StatusOnly: &statusOnly})
for pager.More() {
	nextResult, err := pager.NextPage(ctx)
	if err != nil {
		log.Fatalf("failed to advance page: %v", err)
	}
	for _, vm := range nextResult.Value {
		// log.Println(*vm.ID)
		stringSlice := strings.Split(*vm.ID, "/")
		instanceView, err := vmClient.Get(ctx,
			stringSlice[4],
			*vm.Name,
			&armcompute.VirtualMachinesClientGetOptions{Expand: &instanceViewExpand})
		if err != nil {
			log.Fatalf("failed to get vm: %v", err)
		}

}
}`

@chlowell
Copy link
Member

chlowell commented Jul 1, 2022

I also try VirualMachinesClient which has method with Context parameter directly (e.g vmClient.Get). The same behavior that Azure public resources returned but not error returned

Sounds like you have the credential and client configured for the public cloud (which is the default). Can you share more of the code you're running, showing credential and client construction?

@chlowell chlowell added needs-author-feedback Workflow: More information is needed from author to address the issue. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Jul 1, 2022
@xqi-aviatrix
Copy link
Author

Hi, here are most of my sample code without credential details. currentSubscription is subscription under an Azure Public tenant

`credential_options := &azidentity.ClientSecretCredentialOptions{ClientOptions: policy.ClientOptions{Cloud: cloud.AzureChina}}
cred, err := azidentity.NewClientSecretCredential(
tenant_id_auzre_public,
client_id_auzre_public,
client_secret_auzre_public,
credential_options)
if err != nil {
log.Fatalf("Authentication Failure: %#v", err)
return
}
ctx := context.Background()

vmClient, err := armcompute.NewVirtualMachinesClient(currentSubscription, cred, nil)
if err != nil {
	log.Fatalf("virtual machine client creation failed %s", err)
	return
}

statusOnly := "true"
var instanceViewExpand armcompute.InstanceViewTypes = "instanceView"
pager := vmClient.NewListAllPager(&armcompute.VirtualMachinesClientListAllOptions{StatusOnly: &statusOnly})
for pager.More() {
	nextResult, err := pager.NextPage(ctx)
	if err != nil {
		log.Fatalf("failed to advance page: %v", err)
	}
	for _, vm := range nextResult.Value {
		// log.Println(*vm.ID)
		stringSlice := strings.Split(*vm.ID, "/")
		instanceView, err := vmClient.Get(ctx,
			stringSlice[4],
			*vm.Name,
			&armcompute.VirtualMachinesClientGetOptions{Expand: &instanceViewExpand})
		if err != nil {
			log.Fatalf("failed to get vm: %v", err)
		}
		log.Printf("VM ID is %s", *instanceView.ID)
		log.Printf("VM Name is %s", *instanceView.Name)
		log.Printf("VM status: %s", *instanceView.Properties.InstanceView.Statuses[1].DisplayStatus)
		log.Printf("VM image info: %s", *instanceView.Properties.InstanceView.OSName+*instanceView.Properties.InstanceView.OSVersion)
		log.Printf("region: %s", *instanceView.Location)
		log.Printf("tags: %d", len(instanceView.Tags))

	}
}

alClient, err := armmonitor.NewActivityLogsClient(currentSubscription, cred, nil)
if err != nil {
	log.Fatalf("activity log client creation failed %s", err)
	return
}
//eventTimestamp le '2022-06-15T23:59:59.6407898Z' and
var selectFilter string = "eventTimestamp,operationName,resourceId,resourceType,resourceGroupName,status,level,category"
eventsCollectionPager := alClient.NewListPager("eventTimestamp ge 2022-06-02T00:00:00.6407898Z",
	&armmonitor.ActivityLogsClientListOptions{Select: &selectFilter})

totalEventCnt := 0
eventListStack := make([]*armmonitor.ActivityLogsClientListResponse, 0)

for eventsCollectionPager.More() {
	eventList, err := eventsCollectionPager.NextPage(ctx)
	if err != nil {
		log.Fatalf("activity log event next page failed %s", err)
	}
	eventListStack = append(eventListStack, &eventList)
}

for eventListIndex := len(eventListStack) - 1; eventListIndex >= 0; eventListIndex-- {
	eventsCollection := eventListStack[eventListIndex].Value
	if err != nil {
		log.Fatalf("activity log event next page failed %s", err)
		return
	}

	totalEventCnt = totalEventCnt + len(eventsCollection)
	for eventIndex := len(eventsCollection) - 1; eventIndex >= 0; eventIndex-- {
		event := eventsCollection[eventIndex]
		if event.ResourceType.Value != nil && *event.ResourceType.Value == "Microsoft.Network/networkSecurityGroups/securityRules" {
			if event.Status.Value != nil && *event.Status.Value == "Succeeded" {
				log.Printf("EventTime is %s, OperationName is %s, Resource Id is %s, Resource Type is %s, ResourceGroup is %s, Status is %s, Event ID is %s, severity level is %s, category %s",
					*event.EventTimestamp, *event.OperationName.Value, *event.ResourceID,
					*event.ResourceType.Value, *event.ResourceGroupName, *event.Status.Value,
					*event.ID, *event.Level, *event.Category.Value)

			}
		}
	}
	log.Printf("current event count is %d", len(eventsCollection))
}

log.Printf("total event count is %d", totalEventCnt)`

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Jul 1, 2022
@chlowell
Copy link
Member

chlowell commented Jul 6, 2022

Like the credential, client configuration defaults to public cloud. They need explicit configuration for Azure China. The API is the same for all clients, so you can reuse cloud options like this:

import (
    "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
    "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
    "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
)

+++clientOpts := policy.ClientOptions{Cloud: cloud.AzureChina}
+++credential_options := &azidentity.ClientSecretCredentialOptions{ClientOptions: clientOpts}
cred, err := azidentity.NewClientSecretCredential(
    tenant_id_auzre_public,
    client_id_auzre_public,
    client_secret_auzre_public,
    credential_options)

...

+++armOpts := &arm.ClientOptions{ClientOptions: clientOpts}
+++vmClient, err := armcompute.NewVirtualMachinesClient(currentSubscription, cred, armOpts)

...

+++alClient, err := armmonitor.NewActivityLogsClient(currentSubscription, cred, armOpts)

As for why authentication succeeds, that's because the credential doesn't simply send requests to Azure China, which would respond with an error because the tenant and client IDs are invalid so far as it's concerned. When a client first requests a token, the credential asks Azure AD where to send token requests for your tenant. The credential then requests tokens from a public cloud endpoint, which works because the client ID and secret are valid for that (public cloud) tenant. The credential can't get a token for Azure China, but that's okay because your clients are configured (by default) for the public cloud. Similarly, your client calls succeed because currentSubscription exists in the public cloud.

@chlowell chlowell added needs-author-feedback Workflow: More information is needed from author to address the issue. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Jul 6, 2022
@xqi-aviatrix
Copy link
Author

Thanks a lot for your investigation.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Identity customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-author-feedback Workflow: More information is needed from author to address the issue. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

2 participants