-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[ts-http-runtime] Allow insecure bearer token policy #30206
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,12 @@ | |
// Licensed under the MIT license. | ||
|
||
import { AccessToken, GetTokenOptions, TokenCredential } from "../auth/tokenCredential.js"; | ||
import { TypeSpecRuntimeLogger } from "../logger/logger.js"; | ||
import { PipelineRequest, PipelineResponse, SendRequest } from "../interfaces.js"; | ||
|
||
import { PipelinePolicy } from "../pipeline.js"; | ||
import { createTokenCycler } from "../util/tokenCycler.js"; | ||
import { TypeSpecRuntimeLogger } from "../logger/logger.js"; | ||
import { logger as coreLogger } from "../log.js"; | ||
import { createTokenCycler } from "../util/tokenCycler.js"; | ||
|
||
/** | ||
* The programmatic identifier of the bearerTokenAuthenticationPolicy. | ||
|
@@ -100,6 +101,11 @@ export interface BearerTokenAuthenticationPolicyOptions { | |
* A logger can be sent for debugging purposes. | ||
*/ | ||
logger?: TypeSpecRuntimeLogger; | ||
/** | ||
* Allows for connecting to HTTP endpoints instead of enforcing HTTPS. | ||
* CAUTION: Never use this option in production. | ||
*/ | ||
allowInsecureConnection?: boolean; | ||
} | ||
|
||
/** | ||
|
@@ -130,6 +136,47 @@ function getChallenge(response: PipelineResponse): string | undefined { | |
return; | ||
} | ||
|
||
/** | ||
* Checks if the request is allowed to be sent over an insecure connection. | ||
* | ||
* A request is allowed to be sent over an insecure connection when: | ||
* - The `allowInsecureConnection` option is set to `true`. | ||
* - The request has the `allowInsecureConnection` property set to `true`. | ||
* - The request is being sent to `localhost` or `127.0.0.1` | ||
*/ | ||
function allowInsecureConnection( | ||
request: PipelineRequest, | ||
options: BearerTokenAuthenticationPolicyOptions, | ||
): boolean { | ||
if (options.allowInsecureConnection && request.allowInsecureConnection) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
const url = new URL(request.url); | ||
if (url.hostname === "localhost" || url.hostname === "127.0.0.1") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is IPv6 covered? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/** | ||
* Logs a warning about sending a bearer token over an insecure connection. | ||
* | ||
* This function will emit a node warning once, but log the warning every time. | ||
*/ | ||
function emitInsecureConnectionWarning(): void { | ||
const warning = | ||
"Sending bearer token over insecure transport. Assume any token issued is compromised."; | ||
|
||
coreLogger.warning(warning); | ||
|
||
if (typeof process?.emitWarning === "function" && !emitInsecureConnectionWarning.warned) { | ||
emitInsecureConnectionWarning.warned = true; | ||
process.emitWarning(warning); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kinda nice I thought
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! I'd rather spam everytime this is being used instead of just once though :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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! |
||
} | ||
} | ||
|
||
emitInsecureConnectionWarning.warned = false; // Prime TypeScript to allow the property. Used to only emit warning once. | ||
|
||
/** | ||
* A policy that can request a token from a TokenCredential implementation and | ||
* then apply it to the Authorization header of a request as a Bearer token. | ||
|
@@ -171,9 +218,13 @@ export function bearerTokenAuthenticationPolicy( | |
*/ | ||
async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> { | ||
if (!request.url.toLowerCase().startsWith("https://")) { | ||
throw new Error( | ||
"Bearer token authentication is not permitted for non-TLS protected (non-https) URLs.", | ||
); | ||
if (allowInsecureConnection(request, options)) { | ||
emitInsecureConnectionWarning(); | ||
} else { | ||
throw new Error( | ||
"Bearer token authentication is not permitted for non-TLS protected (non-https) URLs.", | ||
); | ||
} | ||
} | ||
|
||
await callbacks.authorizeRequest({ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thebearerTokenAuthenticationPolicy
, 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:allowInsecureBearerTokenAuth
or similar, to allow this behavior, and this additional property gets plumbed through instead, so that the behavior ofallowInsecureConnection
is unchanged. Azure Core can just not expose the new property.allowInsecureConnection
on thebearerTokenAuthPolicy
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we making this change for Azure as well then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Q Timo, let me get more clarity on that and get back to you