[core-http] Proxy - Basic authentication support for storage-blob #5009

HarshaNalluru · 2019-09-05T21:41:39Z

ProxySettings may have username & password, but getDefaultProxySettings doesn't parse them. Storage Explorer current proxy settings need them.

This PR updates the core-http's getDefaultProxySettings. With this, basic authentication with username/password works.

jeremymeng · 2019-09-05T21:49:03Z

It would be ideal if URLBuilder support urls with username and password. The change won't be simple though. Could you please log an issue for the future consideration?

HarshaNalluru · 2019-09-05T21:51:18Z

Ideally, the updates to core-http should be in the master branch.
Once this is reviewed,

I'll make a PR to master scoping the changes to core-http and
then merge the master branch into feature/storage and
then another PR to feature/storage branch for the rest of the changes in this PR related to storage SDKs.

HarshaNalluru · 2019-09-05T21:58:07Z

It would be ideal if URLBuilder support urls with username and password. The change won't be simple though. Could you please log an issue for future consideration?

Created an issue - #5011

sophiajt · 2019-09-05T23:18:49Z

sdk/core/core-http/lib/policies/proxyPolicy.ts

+  proxyUrl?: string,
+  username?: string,
+  password?: string
+): ProxySettings | undefined {
  if (!proxyUrl) {
    proxyUrl = loadEnvironmentProxyValue();


How does this work with environment proxies?

Implementation of loadEnvironmentProxyValue()

azure-sdk-for-js/sdk/core/core-http/lib/policies/proxyPolicy.ts

Lines 16 to 32 in 87a1397

function loadEnvironmentProxyValue(): string | undefined {

if (!process) {

return undefined;

}

if (process.env[Constants.HTTPS_PROXY]) {

return process.env[Constants.HTTPS_PROXY];

} else if (process.env[Constants.HTTPS_PROXY.toLowerCase()]) {

return process.env[Constants.HTTPS_PROXY.toLowerCase()];

} else if (process.env[Constants.HTTP_PROXY]) {

return process.env[Constants.HTTP_PROXY];

} else if (process.env[Constants.HTTP_PROXY.toLowerCase()]) {

return process.env[Constants.HTTP_PROXY.toLowerCase()];

}

return undefined;

}

It is not obvious on how(or why) someone would set username/password as environment variables to load them from the environment with loadEnvironmentProxyValue.

We can go for some syntax like http://username:password@ipaddress:port for the proxyUrl variable and parse it properly.

Or we can avoid using getDefaultProxySettings like mentioned below - #5009 (comment).

Thoughts?

Looking at the .NET tests they do support urls with credentials.

the relevant .NET commit

Supporting parsing http://username:password@ipaddress:port into a ProxySettings object seems like the right approach here.

It is not obvious on how(or why) someone would set username/password as environment variables to load them from the environment with loadEnvironmentProxyValue.

username/password are simply part of the proxy URL. One scenario I've seen many times is the proxy settings aren't controlled by the app developer but instead by the ops folks who control the production/staging environment. Devs just send requests like normal, ops folks ensure the environment variable is set properly, and everything "just works (tm)".

I agree with @bterlson, but would keep parsing username/password from the url out of this PR and out of the current preview.

sophiajt · 2019-09-05T23:19:23Z

sdk/storage/storage-blob/samples/typescript/proxyAuth.ts

-      proxy: { url: "http://localhost:3128" }
+      proxy: {
+        // username and password can be passed as additional arguments
+        url: "http://localhost:3128"


You may want to show actually passing a username and password

Basic authentication works fine with current state of core-http if we pass in the ProxySettings as follows:

{ host: "127.0.0.1", port: 3128, username: "xxx", password: "xxx" }

to the proxyPolicy() out of the box.

But if we wanted to load these values from the environment variables instead then we can have related changes in. Technically the loading can be done from SDK as well.

@ramya0820 Good idea. I think host should also include http:// or https://.

And ProxyOptions from storage sdks can be removed in favour of ProxySettings from core-http.

We can directly pass proxySettings to the proxyPolicy() method and avoid using getDefaultProxySettings.

azure-sdk-for-js/sdk/storage/storage-blob/src/Pipeline.ts

Lines 224 to 230 in 87a1397

if (isNode) {

// ProxyPolicy is only avaiable in Node.js runtime, not in browsers

factories.push(

proxyPolicy(

getDefaultProxySettings(pipelineOptions.proxy ? pipelineOptions.proxy.url : undefined)

)

);

Thoughts, anybody ?

Since the url to access is present on the request, believe core-http attempts to infer the protocol from it.
Came across something about username:password@host being deprecated eventually.
Overall, if ProxySettings is exposed to user to pass in, that should be sufficient I think.
cc: @bterlson @chradek @ramya-rao-a

Personal opinion here, I defer to @bterlson for the final call:

I would expect an sdk to use getDefaultProxySettings if the user has provided no details on the proxy and we are expected to infer from the environment.

If the user is indeed providing proxy details, then I would prefer them to pass in the ProxySettings instead of the proxy url (with host + port).

ProxyOptions should not exist and be replaced with ProxySettings.

The maximally flexible signature would take either a ProxySettings or a string proxy URL which is parsed into a ProxySettings. Given the late hour, I'm ok with punting on parsing string proxy URLs for now.

I would suggest something like ramya-rao-a@7dfbce1

Another option is removing the options, and solely relying on the environment variables? It's Node-only anyway.

…xySupportStorage

Allow configuring username password in proxy

…alluru/azure-sdk-for-js into HTTPSProxySupportStorage

ramya-rao-a · 2019-09-06T21:57:52Z

sdk/core/core-http/lib/policies/proxyPolicy.ts

+export function getDefaultProxySettings(
+  proxyUrl?: string,
+  username?: string,
+  password?: string


@HarshaNalluru With the new set of changes, we should revert the changes to this file

In progress. Pushing the changes.

HarshaNalluru · 2019-09-06T22:35:31Z

sdk/core/core-http/lib/coreHttp.ts

@@ -13,7 +13,7 @@ export { OperationArguments } from "./operationArguments";
 export { OperationParameter, OperationQueryParameter, OperationURLParameter } from "./operationParameter";
 export { OperationResponse } from "./operationResponse";
 export { OperationSpec } from "./operationSpec";
-export { ServiceClient, ServiceClientOptions, flattenResponse } from "./serviceClient";
+export { ServiceClient, ServiceClientOptions, flattenResponse, ProxySettings } from "./serviceClient";


This is the only change related to core-http in this PR.

Made a separate PR to master - #5043.

I will then merge the master branch into feature/storage(#5046) and then merge this PR(basic auth support for storage-blob).

HarshaNalluru added 4 commits September 3, 2019 14:32

proxyPolicy - parse username-password

f7ee370

enable basic auth in core-http

825b4fb

simplify proxySettings object

1387b64

update sample with a comment

87a1397

HarshaNalluru requested review from daviwil, jeremymeng, jiacfan, vinjiang and XiaoningLiu as code owners September 5, 2019 21:41

HarshaNalluru changed the title ~~[core-http] Proxy Basic authentication support for storage-blob~~ [core-http] Proxy - Basic authentication support for storage-blob Sep 5, 2019

HarshaNalluru self-assigned this Sep 5, 2019

HarshaNalluru added Azure.Core Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Sep 5, 2019

HarshaNalluru requested review from bterlson, ramya-rao-a and sophiajt and removed request for vinjiang and jiacfan September 5, 2019 21:42

HarshaNalluru mentioned this pull request Sep 5, 2019

[core-http] URLBuilder in proxy policy doesn't support username/password #5011

Closed

ramya0820 mentioned this pull request Sep 5, 2019

[core-http] [Service Bus] Enable and verify proxy support for HTTP requests in Node and Browser #5018

Closed

sophiajt reviewed Sep 5, 2019

View reviewed changes

ramya-rao-a removed the request for review from daviwil September 6, 2019 03:06

ramya-rao-a mentioned this pull request Sep 6, 2019

Release Storage Track 2 Library Preview 3 for JavaScript #4252

Closed

14 tasks

ramya-rao-a and others added 3 commits September 6, 2019 12:55

Allow configuring username password in proxy

7dfbce1

Merge remote-tracking branch 'upstream/feature/storage' into HTTPSPro…

16a2225

…xySupportStorage

resolve conflict in the sample

5825042

HarshaNalluru and others added 4 commits September 6, 2019 14:39

Merge pull request #20 from HarshaNalluru/storage-proxy

e85c798

Allow configuring username password in proxy

remove ProxyOptions

52bedff

update sample

e094978

Merge branch 'HTTPSProxySupportStorage' of https://github.com/HarshaN…

7a964b3

…alluru/azure-sdk-for-js into HTTPSProxySupportStorage

ramya-rao-a reviewed Sep 6, 2019

View reviewed changes

HarshaNalluru added 3 commits September 6, 2019 15:06

revert changes to getDefaultProxySettings

87629d0

revert chnages to core-http

45a0f0c

export ProxySettings from core-http

30cfcfe

HarshaNalluru mentioned this pull request Sep 6, 2019

[core-http] export ProxySettings from core-http #5043

Merged

HarshaNalluru commented Sep 6, 2019

View reviewed changes

jeremymeng approved these changes Sep 6, 2019

View reviewed changes

ramya-rao-a approved these changes Sep 6, 2019

View reviewed changes

bterlson approved these changes Sep 6, 2019

View reviewed changes

merge feature/storage

c35c794

HarshaNalluru merged commit f59cf18 into Azure:feature/storage Sep 6, 2019

HarshaNalluru mentioned this pull request Sep 7, 2019

[Storage] Basic authentication support for storage-file and storage-queue #5050

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core-http] Proxy - Basic authentication support for storage-blob #5009

[core-http] Proxy - Basic authentication support for storage-blob #5009

HarshaNalluru commented Sep 5, 2019 •

edited

Loading

jeremymeng commented Sep 5, 2019

HarshaNalluru commented Sep 5, 2019 •

edited

Loading

HarshaNalluru commented Sep 5, 2019

sophiajt Sep 5, 2019

HarshaNalluru Sep 6, 2019

jeremymeng Sep 6, 2019

jeremymeng Sep 6, 2019

bterlson Sep 6, 2019

ramya-rao-a Sep 6, 2019

sophiajt Sep 5, 2019

ramya0820 Sep 6, 2019

HarshaNalluru Sep 6, 2019 •

edited

Loading

ramya0820 Sep 6, 2019

ramya-rao-a Sep 6, 2019

bterlson Sep 6, 2019

ramya-rao-a Sep 6, 2019

jeremymeng Sep 6, 2019

ramya-rao-a Sep 6, 2019

HarshaNalluru Sep 6, 2019

HarshaNalluru Sep 6, 2019 •

edited

Loading

	function loadEnvironmentProxyValue(): string \| undefined {
	if (!process) {
	return undefined;
	}

	if (process.env[Constants.HTTPS_PROXY]) {
	return process.env[Constants.HTTPS_PROXY];
	} else if (process.env[Constants.HTTPS_PROXY.toLowerCase()]) {
	return process.env[Constants.HTTPS_PROXY.toLowerCase()];
	} else if (process.env[Constants.HTTP_PROXY]) {
	return process.env[Constants.HTTP_PROXY];
	} else if (process.env[Constants.HTTP_PROXY.toLowerCase()]) {
	return process.env[Constants.HTTP_PROXY.toLowerCase()];
	}

	return undefined;
	}

	if (isNode) {
	// ProxyPolicy is only avaiable in Node.js runtime, not in browsers
	factories.push(
	proxyPolicy(
	getDefaultProxySettings(pipelineOptions.proxy ? pipelineOptions.proxy.url : undefined)
	)
	);

[core-http] Proxy - Basic authentication support for storage-blob #5009

[core-http] Proxy - Basic authentication support for storage-blob #5009

Conversation

HarshaNalluru commented Sep 5, 2019 • edited Loading

jeremymeng commented Sep 5, 2019

HarshaNalluru commented Sep 5, 2019 • edited Loading

HarshaNalluru commented Sep 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

HarshaNalluru commented Sep 5, 2019 •

edited

Loading

HarshaNalluru commented Sep 5, 2019 •

edited

Loading

HarshaNalluru Sep 6, 2019 •

edited

Loading

HarshaNalluru Sep 6, 2019 •

edited

Loading