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 disable-instance-discovery option in interactive pop mode #593

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/book/src/cli/get-token.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ ZURE_CLIENT_CERTIFICATE_PASSWORD environment variable
--client-id string AAD client application ID. It may be specified in AAD_SERVICE_PRINCIPAL_CLIENT_ID or AZURE_CLIENT_ID environment variable
--client-secret string AAD client application secret. Used in spn login. It may be specified in AAD_SERVICE_PRINCIPAL_CLIENT_SECRET or AZURE_CLIENT_S
ECRET environment variable
--disable-instance-discovery set to true to disable instance discovery in environments with their own simple Identity Provider (not AAD) that do not have instance metadata discovery endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by simple?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
--disable-instance-discovery set to true to disable instance discovery in environments with their own simple Identity Provider (not AAD) that do not have instance metadata discovery endpoint.
--disable-instance-discovery set to true to disable instance discovery in environments with their own Identity Provider (not Entra ID/AAD) that does not have instance metadata discovery endpoint.

-e, --environment string Azure environment name (default "AzurePublicCloud")
--federated-token-file string Workload Identity federated token file. It may be specified in AZURE_FEDERATED_TOKEN_FILE environment variable
-h, --help help for get-token
Expand Down
43 changes: 24 additions & 19 deletions pkg/internal/pop/msal_public.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,24 @@ import (
"github.com/AzureAD/microsoft-authentication-library-for-go/apps/public"
)

type PublicClientOptions struct {
Authority string
ClientID string
DisableInstanceDiscovery bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont we also need TenantId here? wondering as I introduced MsalClientOptions in my other PR and the possibility of reusing it here..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options *azcore.ClientOptions
}

// AcquirePoPTokenInteractive acquires a PoP token using MSAL's interactive login flow.
// Requires user to authenticate via browser
func AcquirePoPTokenInteractive(
context context.Context,
popClaims map[string]string,
scopes []string,
authority,
clientID string,
options *azcore.ClientOptions,
pcOptions *PublicClientOptions,
) (string, int64, error) {
var client *public.Client
var err error
client, err = getPublicClient(authority, clientID, options)
client, err = getPublicClient(pcOptions)
if err != nil {
return "", -1, err
}
Expand Down Expand Up @@ -53,13 +58,11 @@ func AcquirePoPTokenByUsernamePassword(
context context.Context,
popClaims map[string]string,
scopes []string,
authority,
clientID,
username,
password string,
options *azcore.ClientOptions,
pcOptions *PublicClientOptions,
) (string, int64, error) {
client, err := getPublicClient(authority, clientID, options)
client, err := getPublicClient(pcOptions)
if err != nil {
return "", -1, err
}
Expand Down Expand Up @@ -88,23 +91,25 @@ func AcquirePoPTokenByUsernamePassword(
}

// getPublicClient returns an instance of the msal `public` client based on the provided options
func getPublicClient(
authority,
clientID string,
options *azcore.ClientOptions,
) (*public.Client, error) {
// The instance discovery will be disable on private cloud
func getPublicClient(pcOptions *PublicClientOptions) (*public.Client, error) {
var client public.Client
var err error
if options != nil && options.Transport != nil {
if pcOptions == nil {
return nil, fmt.Errorf("unable to create public client: publicClientOptions is empty")
}
if pcOptions.Options != nil && pcOptions.Options.Transport != nil {
client, err = public.New(
clientID,
public.WithAuthority(authority),
public.WithHTTPClient(options.Transport.(*http.Client)),
pcOptions.ClientID,
public.WithAuthority(pcOptions.Authority),
public.WithHTTPClient(pcOptions.Options.Transport.(*http.Client)),
public.WithInstanceDiscovery(!pcOptions.DisableInstanceDiscovery),
)
} else {
client, err = public.New(
clientID,
public.WithAuthority(authority),
pcOptions.ClientID,
public.WithAuthority(pcOptions.Authority),
public.WithInstanceDiscovery(!pcOptions.DisableInstanceDiscovery),
)
}
if err != nil {
Expand Down
52 changes: 29 additions & 23 deletions pkg/internal/pop/msal_public_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,13 @@ func TestAcquirePoPTokenByUsernamePassword(t *testing.T) {
ctx,
tc.p.popClaims,
scopes,
authority,
tc.p.clientID,
tc.p.username,
tc.p.password,
&clientOpts,
&PublicClientOptions{
Authority: authority,
ClientID: tc.p.clientID,
Options: &clientOpts,
},
)
defer vcrRecorder.Stop()
if tc.expectedError != nil {
Expand Down Expand Up @@ -156,35 +158,43 @@ func TestGetPublicClient(t *testing.T) {

testCase := []struct {
testName string
authority string
options *azcore.ClientOptions
pcOptions *PublicClientOptions
expectedError error
}{
{
// Test using custom HTTP transport
testName: "TestGetPublicClientWithCustomTransport",
authority: authority,
options: &azcore.ClientOptions{
Cloud: cloud.AzurePublic,
Transport: httpClient,
testName: "TestGetPublicClientWithCustomTransport",
pcOptions: &PublicClientOptions{
Authority: authority,
ClientID: testutils.ClientID,
Options: &azcore.ClientOptions{
Cloud: cloud.AzurePublic,
Transport: httpClient,
},
},
expectedError: nil,
},
{
// Test using default HTTP transport
testName: "TestGetPublicClientWithDefaultTransport",
authority: authority,
options: &azcore.ClientOptions{
Cloud: cloud.AzurePublic,
testName: "TestGetPublicClientWithDefaultTransport",
pcOptions: &PublicClientOptions{
Authority: authority,
ClientID: testutils.ClientID,
Options: &azcore.ClientOptions{
Cloud: cloud.AzurePublic,
},
},
expectedError: nil,
},
{
// Test using incorrectly formatted authority
testName: "TestGetPublicClientWithBadAuthority",
authority: "login.microsoft.com",
options: &azcore.ClientOptions{
Cloud: cloud.AzurePublic,
testName: "TestGetPublicClientWithBadAuthority",
pcOptions: &PublicClientOptions{
Authority: "login.microsoft.com",
ClientID: testutils.ClientID,
Options: &azcore.ClientOptions{
Cloud: cloud.AzurePublic,
},
},
expectedError: fmt.Errorf("unable to create public client"),
},
Expand All @@ -195,11 +205,7 @@ func TestGetPublicClient(t *testing.T) {

for _, tc := range testCase {
t.Run(tc.testName, func(t *testing.T) {
client, err = getPublicClient(
tc.authority,
testutils.ClientID,
tc.options,
)
client, err = getPublicClient(tc.pcOptions)

if tc.expectedError != nil {
if !testutils.ErrorContains(err, tc.expectedError.Error()) {
Expand Down
40 changes: 23 additions & 17 deletions pkg/internal/token/interactive.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,17 @@ import (
)

type InteractiveToken struct {
clientID string
resourceID string
tenantID string
oAuthConfig adal.OAuthConfig
popClaims map[string]string
clientID string
resourceID string
tenantID string
oAuthConfig adal.OAuthConfig
popClaims map[string]string
disableInstanceDiscovery bool
}

// newInteractiveTokenProvider returns a TokenProvider that will fetch a token for the user currently logged into the Interactive.
// Required arguments include an oAuthConfiguration object and the resourceID (which is used as the scope)
func newInteractiveTokenProvider(oAuthConfig adal.OAuthConfig, clientID, resourceID, tenantID string, popClaims map[string]string) (TokenProvider, error) {
func newInteractiveTokenProvider(oAuthConfig adal.OAuthConfig, clientID, resourceID, tenantID string, popClaims map[string]string, disableInstanceDiscovery bool) (TokenProvider, error) {
if clientID == "" {
return nil, errors.New("clientID cannot be empty")
}
Expand All @@ -37,11 +38,12 @@ func newInteractiveTokenProvider(oAuthConfig adal.OAuthConfig, clientID, resourc
}

return &InteractiveToken{
clientID: clientID,
resourceID: resourceID,
tenantID: tenantID,
oAuthConfig: oAuthConfig,
popClaims: popClaims,
clientID: clientID,
resourceID: resourceID,
tenantID: tenantID,
oAuthConfig: oAuthConfig,
popClaims: popClaims,
disableInstanceDiscovery: disableInstanceDiscovery,
}, nil
}

Expand Down Expand Up @@ -73,18 +75,22 @@ func (p *InteractiveToken) TokenWithOptions(ctx context.Context, options *azcore
ctx,
p.popClaims,
scopes,
authorityFromConfig.String(),
p.clientID,
&clientOpts,
&pop.PublicClientOptions{
Authority: authorityFromConfig.String(),
ClientID: p.clientID,
DisableInstanceDiscovery: p.disableInstanceDiscovery,
Options: &clientOpts,
},
)
if err != nil {
return emptyToken, fmt.Errorf("failed to create PoP token using interactive login: %w", err)
}
} else {
cred, err := azidentity.NewInteractiveBrowserCredential(&azidentity.InteractiveBrowserCredentialOptions{
ClientOptions: clientOpts,
TenantID: p.tenantID,
ClientID: p.clientID,
ClientOptions: clientOpts,
TenantID: p.tenantID,
ClientID: p.clientID,
DisableInstanceDiscovery: p.disableInstanceDiscovery,
})
if err != nil {
return emptyToken, fmt.Errorf("unable to create credential. Received: %w", err)
Expand Down
1 change: 1 addition & 0 deletions pkg/internal/token/interactive_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ func TestNewInteractiveToken(t *testing.T) {
tc.resourceID,
tc.tenantID,
tc.popClaims,
false,
)

if tc.expectedError != "" {
Expand Down
42 changes: 22 additions & 20 deletions pkg/internal/token/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,27 @@ import (
)

type Options struct {
LoginMethod string
ClientID string
ClientSecret string
ClientCert string
ClientCertPassword string
Username string
Password string
ServerID string
TenantID string
Environment string
IsLegacy bool
Timeout time.Duration
TokenCacheDir string
tokenCacheFile string
IdentityResourceID string
FederatedTokenFile string
AuthorityHost string
UseAzureRMTerraformEnv bool
IsPoPTokenEnabled bool
PoPTokenClaims string
LoginMethod string
ClientID string
ClientSecret string
ClientCert string
ClientCertPassword string
Username string
Password string
ServerID string
TenantID string
Environment string
IsLegacy bool
Timeout time.Duration
TokenCacheDir string
tokenCacheFile string
IdentityResourceID string
FederatedTokenFile string
AuthorityHost string
UseAzureRMTerraformEnv bool
IsPoPTokenEnabled bool
PoPTokenClaims string
DisableInstanceDiscovery bool
}

const (
Expand Down Expand Up @@ -108,6 +109,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
fs.DurationVar(&o.Timeout, "timeout", 30*time.Second,
fmt.Sprintf("Timeout duration for Azure CLI token requests. It may be specified in %s environment variable", "AZURE_CLI_TIMEOUT"))
fs.StringVar(&o.PoPTokenClaims, "pop-claims", o.PoPTokenClaims, "contains a comma-separated list of claims to attach to the pop token in the format `key=val,key2=val2`. At minimum, specify the ARM ID of the cluster as `u=ARM_ID`")
fs.BoolVar(&o.DisableInstanceDiscovery, "disable-instance-discovery", o.DisableInstanceDiscovery, "set to true to disable instance discovery in environments with their own simple Identity Provider (not AAD) that do not have instance metadata discovery endpoint.")
}

func (o *Options) Validate() error {
Expand Down
4 changes: 2 additions & 2 deletions pkg/internal/token/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ func NewTokenProvider(o *Options) (TokenProvider, error) {
case DeviceCodeLogin:
return newDeviceCodeTokenProvider(*oAuthConfig, o.ClientID, o.ServerID, o.TenantID)
case InteractiveLogin:
return newInteractiveTokenProvider(*oAuthConfig, o.ClientID, o.ServerID, o.TenantID, popClaimsMap)
return newInteractiveTokenProvider(*oAuthConfig, o.ClientID, o.ServerID, o.TenantID, popClaimsMap, o.DisableInstanceDiscovery)
case ServicePrincipalLogin:
if o.IsLegacy {
return newLegacyServicePrincipalToken(*oAuthConfig, o.ClientID, o.ClientSecret, o.ClientCert, o.ClientCertPassword, o.ServerID, o.TenantID)
}
return newServicePrincipalTokenProvider(cloudConfiguration, o.ClientID, o.ClientSecret, o.ClientCert, o.ClientCertPassword, o.ServerID, o.TenantID, popClaimsMap)
case ROPCLogin:
return newResourceOwnerTokenProvider(*oAuthConfig, o.ClientID, o.Username, o.Password, o.ServerID, o.TenantID, popClaimsMap)
return newResourceOwnerTokenProvider(*oAuthConfig, o.ClientID, o.Username, o.Password, o.ServerID, o.TenantID, popClaimsMap, o.DisableInstanceDiscovery)
case MSILogin:
return newManagedIdentityToken(o.ClientID, o.IdentityResourceID, o.ServerID)
case AzureCLILogin:
Expand Down
Loading
Loading