[ts-http-runtime] Allow insecure bearer token policy #30206

maorleger · 2024-06-27T15:31:50Z

Issues associated with this PR

N/A

Describe the problem that is addressed by this PR

For third-party services, there is a desire to allow local bearer authentication over HTTP. While this is not a pattern that Azure would ever use or recommend, it should be allowed at the discretion of a third-party SDK maintainer.

This PR extends the policy inside ts-http-runtime to add an additional option for disabling the transport check.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
It's possible to only consult the existing allowInsecureConnection on the pipeline request, but because we have a strong desire to consolidate our Azure-branded core on top of this package, I thought it best to make this switch opt-in at the policy level so we don't accidentally end up enabling it for Azure SDKs.

Are there test cases added in this PR?
Yes

Provide a list of related PRs

Builds off of #30193

#17749

maorleger · 2024-06-27T15:35:28Z

sdk/core/ts-http-runtime/src/policies/bearerTokenAuthenticationPolicy.ts

+
+  if (typeof process?.emitWarning === "function" && !emitInsecureConnectionWarning.warned) {
+    emitInsecureConnectionWarning.warned = true;
+    process.emitWarning(warning);


Kinda nice I thought

[vitest] (node:1358484) Warning: Sending bearer token over insecure transport. Assume any token issued is compromised. [vitest] (Use `node --trace-warnings ...` to show where the warning was created)

nice! I'd rather spam everytime this is being used instead of just once though :D

hehe yeah I was originally thinking the same but looks like as a best practice we're expected to only emit warning once https://nodejs.org/api/process.html#avoiding-duplicate-warnings

Figured if that was important enough to put in the docs it's probably the right approach? But I want to make some noise so I figure what we can land on is:

Emit the warning once per node's docs

log the warning every since time

So at least if you have logging configured to warn you can see it every time. Not sure if we prefer a differnet approach? I am happy to change it!

azure-sdk · 2024-06-27T15:43:36Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

@typespec/ts-http-runtime

jeremymeng · 2024-06-27T18:06:03Z

sdk/core/ts-http-runtime/src/policies/bearerTokenAuthenticationPolicy.ts

+
+  if (typeof process?.emitWarning === "function" && !emitInsecureConnectionWarning.warned) {
+    emitInsecureConnectionWarning.warned = true;
+    process.emitWarning(warning);


nice! I'd rather spam everytime this is being used instead of just once though :D

jeremymeng · 2024-06-27T18:11:10Z

sdk/core/ts-http-runtime/src/policies/bearerTokenAuthenticationPolicy.ts

+): boolean {
+  if (options.allowInsecureConnection && request.allowInsecureConnection) {
+    const url = new URL(request.url);
+    if (url.hostname === "localhost" || url.hostname === "127.0.0.1") {


Initially for Event Hub emulator we limited to localhost too, but it turns out there are non-localhost scenarios too: other hosts on the same local subnet, containers behind a reverse proxy where HTTPS is no longer used. etc. Not sure if any applies to bearer token issurers though.

is IPv6 covered?

I chose to ignore that for now, thinking that we can add IPv6 support if we encounter a need. I think most local development still uses IPv4 and/or localhost but maybe I'm just stuck in the past 😄

timovv · 2024-06-27T20:51:38Z

sdk/core/ts-http-runtime/src/client/clientHelpers.ts

@@ -45,6 +45,7 @@ export function addCredentialPipelinePolicy(
    const tokenPolicy = bearerTokenAuthenticationPolicy({
      credential,
      scopes: clientOptions?.credentials?.scopes ?? `${endpoint}/.default`,
+      allowInsecureConnection: clientOptions?.allowInsecureConnection ?? false,


Could be missing context here, but I am a tad concerned about plumbing through the option from the client options like this. This change will result in a behavioral difference between the Azure and unbranded Core packages that is difficult to reconcile -- when we get around to making the Azure packages depend on the unbranded package we will need to have a way to set allowInsecureConnection without having this affect the bearerTokenAuthenticationPolicy, in order to preserve the existing behavior. I have two ideas to resolve this issue, not sure if you have a preference or any other thoughts here:

Add another property on the client options, allowInsecureBearerTokenAuth or similar, to allow this behavior, and this additional property gets plumbed through instead, so that the behavior of allowInsecureConnection is unchanged. Azure Core can just not expose the new property.

Don't plumb the property through; if folks want to set allowInsecureConnection on the bearerTokenAuthPolicy they can remove the policy and re-add their own. This is obviously more complicated UX but perhaps that's not the worst thing honestly given we don't really want people doing this unless they truly have to?

Thanks Timo! You're quite right on all accounts here. I know the concern for (2) was that replacing the policy will not be desired for Chat Protocol, it will be a significant complication to sample code. Doesn't mean we can't start with that and see if there's still pain.

(1) does sound enticing, @bterlson what do you think? Any preferences here since you're likely closest to the folks asking for this?

2 is off the table. I'm not opposed to 1, but I would prefer to get clarity on whether we can't just have this for Azure's policy as well. I think we are assuming that we can't for reasons, but can we verify?

Yes, my comment is assuming that we want to keep the status quo for Azure -- as I said, I am lacking context... The concern appears related to being able to intercept an active AAD token due to this (1, 2). I am no security expert but the reasoning seems logical to me -- that's not to say we can't revisit it. @joheredi may have more context since he authored the original PR to enable this which was reverted?

Clearly I am missing a lot of background around the motivation for this feature and why we need it now -- it sounds like we have a customer looking to use the unbranded package and this is a requirement for them? Is there a way for me to learn more about this? Just so I can understand what problem we are trying to solve here.

Quick update here, we engaged the azsdksecurity folks and got their 👍 on the changes. I think we're good to merge this now. @timovv I'll include you in the internal thread for context.

@xirzec would appreciate your review / input on this

Are we making this change for Azure as well then?

Good Q Timo, let me get more clarity on that and get back to you

maorleger · 2024-07-01T21:57:09Z

Closing per offline conversation - once the right sec folks sign off we can reopen this PR 👍

azure-sdk · 2024-07-15T21:20:41Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

@typespec/ts-http-runtime

xirzec · 2024-07-16T18:01:47Z

sdk/core/ts-http-runtime/src/policies/bearerTokenAuthenticationPolicy.ts

+  request: PipelineRequest,
+  options: BearerTokenAuthenticationPolicyOptions,
+): boolean {
+  if (options.allowInsecureConnection && request.allowInsecureConnection) {


I think I prefer using an environment variable for this since it eliminates changing the client options between Azure and unbranded core and also is less likely to be accidentally checked in

github-actions · 2024-09-20T05:03:52Z

Hi @maorleger. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

github-actions · 2024-09-27T08:38:16Z

Hi @maorleger. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.

maorleger added 2 commits June 27, 2024 07:50

port over changes from 30193

fd5b68f

update tests and add warnings

f5b353e

github-actions bot added the Azure.Core label Jun 27, 2024

maorleger marked this pull request as ready for review June 27, 2024 15:33

maorleger requested review from bterlson, xirzec, jeremymeng, mpodwysocki, joheredi and a team as code owners June 27, 2024 15:33

maorleger mentioned this pull request Jun 27, 2024

[ts-http-runtime] Allow insecure bearer token policy #30193

Closed

maorleger commented Jun 27, 2024

View reviewed changes

plumb option through client options

178e9aa

jeremymeng reviewed Jun 27, 2024

View reviewed changes

timovv reviewed Jun 27, 2024

View reviewed changes

maorleger closed this Jul 1, 2024

maorleger reopened this Jul 15, 2024

xirzec reviewed Jul 16, 2024

View reviewed changes

github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Sep 20, 2024

github-actions bot closed this Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ts-http-runtime] Allow insecure bearer token policy #30206

[ts-http-runtime] Allow insecure bearer token policy #30206

maorleger commented Jun 27, 2024

maorleger Jun 27, 2024

jeremymeng Jun 27, 2024

maorleger Jun 27, 2024

azure-sdk commented Jun 27, 2024

jeremymeng Jun 27, 2024

jeremymeng Jun 27, 2024

jeremymeng Jun 27, 2024

maorleger Jun 27, 2024

timovv Jun 27, 2024

maorleger Jun 27, 2024 •

edited

Loading

bterlson Jun 28, 2024

timovv Jun 28, 2024

maorleger Jul 15, 2024

timovv Jul 15, 2024

maorleger Jul 16, 2024

maorleger commented Jul 1, 2024

azure-sdk commented Jul 15, 2024

xirzec Jul 16, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 27, 2024

[ts-http-runtime] Allow insecure bearer token policy #30206

[ts-http-runtime] Allow insecure bearer token policy #30206

Conversation

maorleger commented Jun 27, 2024

Issues associated with this PR

Describe the problem that is addressed by this PR

Provide a list of related PRs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-sdk commented Jun 27, 2024

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

maorleger Jun 27, 2024 • 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

maorleger commented Jul 1, 2024

azure-sdk commented Jul 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 27, 2024

maorleger Jun 27, 2024 •

edited

Loading