Skip to content

{Logging} Redact token headers from SDK HTTP log #17671

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.

Sign up for GitHub

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

Jump to bottom
Merged
jiasli merged 3 commits into from
Apr 22, 2021
Merged

{Logging} Redact token headers from SDK HTTP log #17671

jiasli merged 3 commits into from
Apr 22, 2021

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Apr 13, 2021 •
edited
Loading

Resolve #17625

Context

  1. Starting from azure-core 1.13.0, Authorization header is now exposed in DEBUG log (Make NetworkTraceLoggingPolicy show the auth token in plain text azure-sdk-for-python#17424).
  2. Python SDK decided not to redact x-ms-authorization-auxiliary header (x-ms-authorization-auxiliary header should be redacted azure-sdk-for-python#17271).

Changes

Create a custom policy SafeNetworkTraceLoggingPolicy to replace NetworkTraceLoggingPolicy. It by default redacts Authorization and x-ms-authorization-auxiliary. Any client factory calling prepare_client_kwargs_track2 will have this policy configured.

Testing Guide

Any Track 2 mgmt-plane command, like

> az storage account list

cli.azure.cli.core.sdk.policies: Request URL: 'https://management.azure.com/subscriptions/414af076-009b-4282-9a0a-acf75bcb037e/providers/Microsoft.Storage/storageAccounts?api-version=2021-01-01'
cli.azure.cli.core.sdk.policies: Request method: 'GET'
cli.azure.cli.core.sdk.policies: Request headers:
cli.azure.cli.core.sdk.policies:     'Authorization': '*****'

# Cross tenant auth with x-ms-authorization-auxiliary
> az network vnet peering create --name myVnetAToMyVnetB --resource-group myResourceGroupA --vnet-name myVnetA --remote-vnet /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/myResourceGroupB/providers/Microsoft.Network/VirtualNetworks/myVnetB --allow-vnet-access --debug

cli.azure.cli.core.sdk.policies: Request URL: 'https://management.azure.com/subscriptions/414af076-009b-4282-9a0a-acf75bcb037e/resourceGroups/myResourceGroupA/providers/Microsoft.Network/virtualNetworks/myVnetA/virtualNetworkPeerings/myVnetAToMyVnetB?api-version=2020-11-01'
cli.azure.cli.core.sdk.policies: Request method: 'PUT'
cli.azure.cli.core.sdk.policies: Request headers:
cli.azure.cli.core.sdk.policies:     'x-ms-authorization-auxiliary': '*****'
cli.azure.cli.core.sdk.policies:     'Authorization': '*****'

@yonzhan yonzhan requested a review from Juliehzl April 13, 2021 11:30
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 13, 2021

Logging

@yonzhan yonzhan added this to the S186 milestone Apr 13, 2021
@jiasli jiasli marked this pull request as ready for review April 14, 2021 09:30
@jiasli jiasli requested a review from houk-ms as a code owner April 14, 2021 09:30
houk-ms
houk-ms approved these changes Apr 15, 2021
View reviewed changes
Copy link
Contributor

@houk-ms houk-ms left a comment

Choose a reason for hiding this comment

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

LGTM

houk-ms
houk-ms reviewed Apr 15, 2021
View reviewed changes
src/azure-cli-core/azure/cli/core/commands/client_factory.py
*_, external_tenant_tokens = cred.get_all_tokens(*scopes)
# Hard-code scheme to 'Bearer' as _BearerTokenCredentialPolicyBase._update_headers does.
client_kwargs['headers']['x-ms-authorization-auxiliary'] = \
', '.join("Bearer {}".format(t[1]) for t in external_tenant_tokens)
Copy link
Contributor

@houk-ms houk-ms Apr 15, 2021

Choose a reason for hiding this comment

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

Just curious, always Bearer token here?

Copy link
Member Author

@jiasli jiasli Apr 16, 2021

Choose a reason for hiding this comment

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

Yes. The comment at L182 explains that:

azure-cli/src/azure-cli-core/azure/cli/core/commands/client_factory.py

Line 182 in 36e3d15

# Hard-code scheme to 'Bearer' as _BearerTokenCredentialPolicyBase._update_headers does.

jsntcy
jsntcy approved these changes Apr 19, 2021
View reviewed changes
Copy link
Member

@jsntcy jsntcy left a comment

Choose a reason for hiding this comment

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

:shipit:

@jiasli jiasli mentioned this pull request Apr 22, 2021
Merged
4 tasks
Juliehzl
Juliehzl reviewed Apr 22, 2021
View reviewed changes
src/azure-cli-core/azure/cli/core/sdk/policies.py
Comment on lines +51 to +57
if isinstance(http_request.body, types.GeneratorType):
_LOGGER.debug("File upload")
return
try:
if isinstance(http_request.body, types.AsyncGeneratorType):
_LOGGER.debug("File upload")
return
Copy link
Contributor

@Juliehzl Juliehzl Apr 22, 2021

Choose a reason for hiding this comment

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

In storage track1 data plane SDK, for such file, it will logging with file size. Could we also support it here?

Copy link
Member Author

@jiasli jiasli Apr 22, 2021

Choose a reason for hiding this comment

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

This SafeNetworkTraceLoggingPolicy is designed as a generic policy. That's why I am hesitating to apply it to all SDKs (including data-plane SDKs) at the beginning.

If storage data-plane SDK or other data-plane SDKs requires additional/special logic, we may apply SafeNetworkTraceLoggingPolicy only to ARM and let data-plane SDKs decide what policy they want.

Another solution is to define your own policy and override

azure-cli/src/azure-cli-core/azure/cli/core/commands/client_factory.py

Lines 155 to 156 in 36e3d15

from azure.cli.core.sdk.policies import SafeNetworkTraceLoggingPolicy
client_kwargs['logging_policy'] = SafeNetworkTraceLoggingPolicy()

Copy link
Contributor

@Juliehzl Juliehzl Apr 22, 2021

Choose a reason for hiding this comment

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

We could keep current design then.

@jiasli jiasli merged commit 6728379 into Azure:dev Apr 22, 2021
@jiasli jiasli deleted the logging branch April 22, 2021 05:43
@jiasli jiasli mentioned this pull request Apr 22, 2021
Merged
@jiasli jiasli changed the title {Logging} Redact headers from SDK HTTP log {Logging} Redact token headers from SDK HTTP log Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsntcy jsntcy jsntcy approved these changes

@zhoxing-ms zhoxing-ms zhoxing-ms approved these changes

@Juliehzl Juliehzl Juliehzl approved these changes

@evelyn-ys evelyn-ys Awaiting requested review from evelyn-ys evelyn-ys is a code owner

@fengzhou-msft fengzhou-msft Awaiting requested review from fengzhou-msft

+1 more reviewer

@houk-ms houk-ms houk-ms approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jiasli jiasli

Labels

None yet

Projects

None yet

Milestone

S186

Development

Successfully merging this pull request may close these issues.

Redact tokens from --debug log

6 participants

@jiasli @yonzhan @jsntcy @zhoxing-ms @Juliehzl @houk-ms