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

x-ms-authorization-auxiliary header should be redacted #17271

Closed
jiasli opened this issue Mar 11, 2021 · 2 comments
Closed

x-ms-authorization-auxiliary header should be redacted #17271

jiasli opened this issue Mar 11, 2021 · 2 comments
Assignees
Labels
Azure.Core bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library.
Milestone

Comments

@jiasli
Copy link
Member

jiasli commented Mar 11, 2021

Is your feature request related to a problem? Please describe.

x-ms-authorization-auxiliary header's value is logged by NetworkTraceLoggingPolicy, because only authorization header is redacted.

Even though x-ms-authorization-auxiliary is not supported by Track 2 SDK yet (#8313), Azure CLI is manually adding this header to support cross-tenant auth (Azure/azure-cli#16797).

Describe the solution you'd like

Make an ARMNetworkTraceLoggingPolicy, just like ARMHttpLoggingPolicy. SDKs should use ARMNetworkTraceLoggingPolicy instead of NetworkTraceLoggingPolicy, so that x-ms-authorization-auxiliary can also be redacted.

NetworkTraceLoggingPolicy is created in xxxClientConfiguration, like NetworkManagementClientConfiguration (which belongs to a specific SDK):

self.logging_policy = kwargs.get('logging_policy') or policies.NetworkTraceLoggingPolicy(**kwargs)

We should change this line to

self.logging_policy = kwargs.get('logging_policy') or policies.ARMNetworkTraceLoggingPolicy(**kwargs)

but this of course requires all existing SDKs to be regenerated.

Describe alternatives you've considered

Add the redacting x-ms-authorization-auxiliary behavior directly to NetworkTraceLoggingPolicy from azure-core.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Mar 11, 2021
@xiangyan99 xiangyan99 added Azure.Core bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. labels Mar 11, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Mar 11, 2021
@xiangyan99 xiangyan99 added this to the Backlog milestone Mar 11, 2021
@lmazuel lmazuel modified the milestones: Backlog, [2021] April Mar 11, 2021
@jiasli
Copy link
Member Author

jiasli commented Mar 12, 2021

Another possible solution:

  1. Keep all headers except Authorization, so that there is no breaking change and it is by default secure. x-ms-authorization-auxiliary is Azure ARM specific, so keep it.

  2. Then expose the blocklist to the caller, so that Azure CLI can make a config like:

    az config set logging.incude_credential=True
    

    Currently it is hard coded:

    if header.lower() == 'authorization':
    value = '*****'

  3. If so, CLI can simply set the blocklist to []. If not, CLI can also simply set the blocklist to ['Authorization', 'x-ms-authorization-auxiliary']

In this way, users can still log sensitive headers explicitly.

@xiangyan99
Copy link
Member

As we discussed offline, this is by design.

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this issue Jan 20, 2022
[Hub Generated] Review request for Microsoft.RecoveryServices to add version stable/2021-12-01 (Azure#17271)

* Adds base for updating Microsoft.RecoveryServices from version stable/2021-10-01 to version 2021-12-01

* Updates readme

* Updates API version in new specs and examples

* - Adding support for creation of new 'Enhanced' policy for IaaSVM backup
- This policy type will additionally support hourly backups along with daily and weekly supported by current policy
- Added respective examples for create/update, Get and List policy api

* - prettier fixes

* updating readme.md to new api

* - correcting the path of new example files

* - fixing examples

* - more fixes in examples

* fixing spell check error

* fixing semantic errors

* - semantic errors fix
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

3 participants