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

Allow requests to opt out of HTTPS enforcement #9821

Merged
merged 4 commits into from
Mar 20, 2020

Conversation

chlowell
Copy link
Member

Our logic for authenticating a multipart request currently fails because preparing one entails passing requests with schemeless URLs to BearerTokenCredentialPolicy, which duly raises because these URLs don't start with "https".

As a workaround for this and other cases in which enforcing HTTPS is unnecessary or undesirable, this change allows individual requests to opt out of https enforcement via an enforce_https context option (i.e. a keyword argument to pipeline.run).

@chlowell chlowell self-assigned this Feb 11, 2020
@chlowell chlowell added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Feb 11, 2020
Copy link
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

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

We need to handle retry case (enforce_https is already popped)

@bryevdv
Copy link
Contributor

bryevdv commented Feb 12, 2020

Would it be valuable to be more precise? Allow schema-less URLs (behind a flag, presumably), but still never allow URLs that start with "http://" ?

@chlowell
Copy link
Member Author

Allowing schemeless URLs is a reasonable way to solve the problem for storage but I think it's a weird special case for the general purpose bearer auth policy we have in azure-core. So, I think that (reasonable) solution belongs in the storage libraries.

I landed on allowing opting out of the enforcement entirely because it solves the problem at hand in a more general way I think will be useful when we find another similar scenario we want to support. For example, I'm running Azure Stack on a network I trust, and I care about TLS overhead.

@bryevdv
Copy link
Contributor

bryevdv commented Feb 12, 2020

@chlowell I didn't mean to suggest moving this, only making the "lax" mode here a little less lax

@chlowell
Copy link
Member Author

I follow you, what I meant is that I think configuration to disable validation entirely is more generally useful and less weird than configuration to allow malformed URLs. If we don't want the more general configuration (which is okay, no one's asked for it), I think storage clients should implement a custom policy.

@adxsdk6
Copy link

adxsdk6 commented Mar 13, 2020

Can one of the admins verify this patch?

@chlowell chlowell requested a review from xiangyan99 March 13, 2020 22:55
@lmazuel lmazuel merged commit 81eb35b into Azure:master Mar 20, 2020
@chlowell chlowell deleted the optional-https branch March 21, 2020 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants