azcore 1.8.0 prevents the usage of the sdk against non-https test endpoint

[serbrech]

Bug Report

in azcore 1.8.0, a new Policy was added in azcore to ensure BearerToken are not sent to an http endpoint.
That's a great addition to ensure we don't leak tokens.

However, we are also unable to configure the clients to not set a bearer token on the outgoing request.
that makes us unable to use the SDK in internal integration testing where we use HTTP enpoints to avoid the overhead of certificate setup.

Would you consider adding an option to make this possible?

I don't think we should allow sending a BearerToken to an httpendpoint, but if we could configure the sdk with a NoAuthIdentity type or remove the BearerTokenPolicy from the clients, that would allow us to do this, without the risk of sending a token over insecure channel.

Our current workaround is to wrap the transport, and strip the fake bearer token from the header, but that is really hacky, and has some complexity caused by Poller, and Paging on List calls.

[serbrech]

/cc @jim-minter @FumingZhang

[jim-minter]

remove the BearerTokenPolicy from the clients

azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.go

Lines 32 to 38 in 5d4f342

	authPolicy := NewBearerTokenPolicy(cred, &armpolicy.BearerTokenOptions{
	AuxiliaryTenants: options.AuxiliaryTenants,
	Scopes: []string{conf.Audience + "/.default"},
	})
	perRetry := make([]azpolicy.Policy, len(plOpts.PerRetry), len(plOpts.PerRetry)+1)
	copy(perRetry, plOpts.PerRetry)
	plOpts.PerRetry = append(perRetry, authPolicy, exported.PolicyFunc(httpTraceNamespacePolicy))

I wonder whether something like wrapping the above with if cred != nil might do the trick? I think that would prevent a BearerTokenPolicy being added to the client if no credential is passed in, which would then avoid the error if an http endpoint is being used in test?

+1 that the gymnastics we're having to pull to workaround this are really awful.

[jhendrixMSFT]

That would work. However, I think what we should settle on is the overall pattern on how unit tests should work WRT credentials across all SDKs.

• pass nil which skips adding the authentication policy
• provide a TokenCredential type in azcore that does the right thing (e.g. fake.TokenCredential)
• have some other opt-out mechanism (not a fan of this)

When you write unit tests, which is easier for you? Passing nil or some fake credential type?

[jhendrixMSFT]

CC @JeffreyRichter

[jhendrixMSFT]

I will add that passing nil to prevent inclusion of the authentication policy means that clients in unit tests now behave slightly different from their production counterparts. Maybe this is ok, but I do think there's value in having unit tests behave as close to production as possible.

[jim-minter]

Note: it's more of an integration test scenario; it's not a unit test scenario.

It seems logical to me to pass a nil cred if no Authorization header is wanted, but our integration scenario is a rare use case, and if that flow is enabled, it might become a common error case for new SDK users to pass a nil cred and then be confused when they get a 401 back from production ARM?

It's not clear to me that fake.TokenCredential helps much here. FWIW at the moment we're having to pass something equivalent to fake.TokenCredential (our azcore version isn't yet new enough that fake.TokenCredential is available) because clients can't cope with a nil credential. But then the BearerTokenPolicy would need a special case not to warn if the credential came from a fake.TokenCredential. And then we might still be sending an Authorization header over HTTP (unless that's hacked up too in BearerTokenPolicy?), which, while presumably not being a live credential, is still a code smell.

Reducing this to enabling a nil cred effectively solves two problems for us - we no longer need the fake.TokenCredential equivalent, and as no Authorization header is sent we don't need to workaround the BearerToken error.

[jhendrixMSFT]

CC @chlowell for awareness

At present, the BearerTokenPolicy will panic by design if the TokenCredential is nil. We've used this as an enforcing mechanism to ensure that client constructors that require a TokenCredential provide one (I don't think we've explicitly documented this though). This is true for other credential types (e.g. SharedKeyCredential in the storage SDKs). In addition, per our design guidelines, if a service accepts unauthorized access, then SDKs are to provide a NewClientWithNoCredential constructor variant.

You are correct that if fake.TokenCredential is to become the thing that we'd have to update the BearerTokenPolicy to not reject the fake token it returns. Off-the-cuff it would likely check for the magic token value fake_token and bypass the http check (agreed it's not very elegant).

[jhendrixMSFT]

Would you mind expanding on how these tests work? What I'm wondering is if our fakes (which GA in November) will be a better way to solve this issue.

[jim-minter]

I see, we could manually implement New*ClientWithNoCredential() functions in our SDKs, but if we were to do this it would probably convenient for azcore to have arm.NewClientWithNoCredential() and armruntime.NewPipelineWithNoCredential() analogs. I think that something like that could potentially work.

[jhendrixMSFT]

This was introduced by #21673

[jhendrixMSFT]

The fix (accepting a nil credential value) will go out in November's azcore@v1.9.0 release.

[serbrech]

Thank you @jhendrixMSFT ! Appreciate the quick turn around