From 56e17611344d673f3d5c63afd8fde2df1a195b7d Mon Sep 17 00:00:00 2001 From: jram Date: Tue, 30 Jan 2024 17:09:20 -0800 Subject: [PATCH] Remove internal usage of ValidateTokenExpiration. (#393) * - remove internal usage of validateTokenExpiration as it uses client's system time which can cause issues. * update SDK version --- cs/src/Connections/TunnelConnection.cs | 1 + cs/src/Connections/TunnelRelayConnection.cs | 7 ------- cs/src/Management/TunnelAccessTokenProperties.cs | 5 ++++- cs/src/Management/TunnelExtensions.cs | 1 + cs/src/Management/TunnelManagementClient.cs | 3 +-- ts/src/connections/package.json | 4 ++-- ts/src/connections/tunnelConnectionBase.ts | 6 +----- ts/src/connections/tunnelConnectionSession.ts | 12 ++++-------- ts/src/connections/tunnelSession.ts | 1 + ts/src/management/package.json | 2 +- ts/src/management/tunnelAccessTokenProperties.ts | 4 +--- ts/src/management/tunnelManagementHttpClient.ts | 1 - ts/src/management/tunnelPlanTokenProperties.ts | 2 +- 13 files changed, 18 insertions(+), 31 deletions(-) diff --git a/cs/src/Connections/TunnelConnection.cs b/cs/src/Connections/TunnelConnection.cs index 3ca8704a..20ee1b16 100644 --- a/cs/src/Connections/TunnelConnection.cs +++ b/cs/src/Connections/TunnelConnection.cs @@ -184,6 +184,7 @@ protected virtual void OnTunnelChanged() /// Validate if it is not null or empty. /// /// is thrown if the is expired. + /// Note: uses the client's system time for validation. protected virtual void ValidateAccessToken() { if (!string.IsNullOrEmpty(this.accessToken)) diff --git a/cs/src/Connections/TunnelRelayConnection.cs b/cs/src/Connections/TunnelRelayConnection.cs index 2ecc0cad..8945aed5 100644 --- a/cs/src/Connections/TunnelRelayConnection.cs +++ b/cs/src/Connections/TunnelRelayConnection.cs @@ -234,7 +234,6 @@ protected virtual async Task CreateSessionStreamAsync(CancellationToken _ => new[] { WebSocketSubProtocolV2, WebSocketSubProtocol }, }; - ValidateAccessToken(); Trace.Verbose("Connecting to {0} tunnel relay {1}", ConnectionRole, RelayUri.AbsoluteUri); var (stream, subprotocol) = await this.StreamFactory.CreateRelayStreamAsync( RelayUri, @@ -398,12 +397,6 @@ protected virtual async Task RefreshTunnelAccessTokenAsync(CancellationTok return false; } - // Access token may be null if tunnel allows anonymous access. - if (this.accessToken != null) - { - TunnelAccessTokenProperties.ValidateTokenExpiration(this.accessToken); - } - Trace.Verbose( "Refreshed tunnel access token. New token: {0}", TunnelAccessTokenProperties.GetTokenTrace(this.accessToken)); diff --git a/cs/src/Management/TunnelAccessTokenProperties.cs b/cs/src/Management/TunnelAccessTokenProperties.cs index 534ec48b..72916d9c 100644 --- a/cs/src/Management/TunnelAccessTokenProperties.cs +++ b/cs/src/Management/TunnelAccessTokenProperties.cs @@ -126,7 +126,10 @@ public override string ToString() /// Checks if the tunnel access token expiration claim is in the past. /// /// The token is expired. - /// Note this does not throw if the token is an invalid format. + /// + /// Note this does not throw if the token is an invalid format. + /// Uses the client's system time for validation. + /// public static void ValidateTokenExpiration(string token) { var t = TryParse(token); diff --git a/cs/src/Management/TunnelExtensions.cs b/cs/src/Management/TunnelExtensions.cs index b2caa7b1..497bf8ad 100644 --- a/cs/src/Management/TunnelExtensions.cs +++ b/cs/src/Management/TunnelExtensions.cs @@ -80,6 +80,7 @@ public static bool TryGetAccessToken(this Tunnel tunnel, string accessTokenScope /// The tokens are searched in Tunnel.AccessTokens dictionary where each /// key may be either a single scope or space-delimited list of scopes. /// The method only validates token expiration. It doesn't validate if the token is not JWT. It doesn't validate JWT signature or claims. + /// Uses the client's system time for validation. /// /// The tunnel to get the access token from. /// Access token scope to get the token for. diff --git a/cs/src/Management/TunnelManagementClient.cs b/cs/src/Management/TunnelManagementClient.cs index 50c90ba4..5ac87f7b 100644 --- a/cs/src/Management/TunnelManagementClient.cs +++ b/cs/src/Management/TunnelManagementClient.cs @@ -840,7 +840,6 @@ private Uri BuildTunnelUri( if (!string.IsNullOrEmpty(options?.AccessToken)) { - TunnelAccessTokenProperties.ValidateTokenExpiration(options.AccessToken); authHeader = new AuthenticationHeaderValue( TunnelAuthenticationScheme, options.AccessToken); } @@ -854,7 +853,7 @@ private Uri BuildTunnelUri( { foreach (var scope in accessTokenScopes) { - if (tunnel.TryGetValidAccessToken(scope, out string? accessToken)) + if (tunnel.TryGetAccessToken(scope, out string? accessToken)) { authHeader = new AuthenticationHeaderValue( TunnelAuthenticationScheme, accessToken); diff --git a/ts/src/connections/package.json b/ts/src/connections/package.json index 7320a5e3..e62f8633 100644 --- a/ts/src/connections/package.json +++ b/ts/src/connections/package.json @@ -18,8 +18,8 @@ "buffer": "^5.2.1", "debug": "^4.1.1", "vscode-jsonrpc": "^4.0.0", - "@microsoft/dev-tunnels-contracts": ">1.1.10", - "@microsoft/dev-tunnels-management": ">1.1.10", + "@microsoft/dev-tunnels-contracts": ">1.1.11", + "@microsoft/dev-tunnels-management": ">1.1.11", "@microsoft/dev-tunnels-ssh": "^3.11.36", "@microsoft/dev-tunnels-ssh-tcp": "^3.11.36", "uuid": "^3.3.3", diff --git a/ts/src/connections/tunnelConnectionBase.ts b/ts/src/connections/tunnelConnectionBase.ts index f178bc7d..bc590cfe 100644 --- a/ts/src/connections/tunnelConnectionBase.ts +++ b/ts/src/connections/tunnelConnectionBase.ts @@ -148,11 +148,7 @@ export class TunnelConnectionBase implements TunnelConnection { cancellation, ); this.refreshingTunnelAccessTokenEmitter.fire(event); - const result = event.tunnelAccessToken ? await event.tunnelAccessToken : undefined; - if (result) { - TunnelAccessTokenProperties.validateTokenExpiration(result); - } - return result; + return event.tunnelAccessToken ? await event.tunnelAccessToken : undefined; } /** diff --git a/ts/src/connections/tunnelConnectionSession.ts b/ts/src/connections/tunnelConnectionSession.ts index 35491533..50cf3783 100644 --- a/ts/src/connections/tunnelConnectionSession.ts +++ b/ts/src/connections/tunnelConnectionSession.ts @@ -229,11 +229,10 @@ export class TunnelConnectionSession extends TunnelConnectionBase implements Tun this.raiseReportProgress(Progress.OpeningHostConnectionToRelay); } - const accessToken = this.validateAccessToken(); this.trace(TraceLevel.Info, 0, `Connecting to ${this.connectionRole} tunnel relay ${this.relayUri}`); this.trace(TraceLevel.Verbose, 0, `Sec-WebSocket-Protocol: ${this.connectionProtocols.join(', ')}`); - if (accessToken) { - const tokenTrace = TunnelAccessTokenProperties.getTokenTrace(accessToken); + if (this.accessToken) { + const tokenTrace = TunnelAccessTokenProperties.getTokenTrace(this.accessToken); this.trace(TraceLevel.Verbose, 0, `Authorization: tunnel <${tokenTrace}>`); } @@ -246,7 +245,7 @@ export class TunnelConnectionSession extends TunnelConnectionBase implements Tun const streamAndProtocol = await this.streamFactory.createRelayStream( this.relayUri, this.connectionProtocols, - accessToken, + this.accessToken, clientConfig ); @@ -342,10 +341,6 @@ export class TunnelConnectionSession extends TunnelConnectionBase implements Tun await this.refreshTunnel(false, cancellation); } - if (this.accessToken) { - TunnelAccessTokenProperties.validateTokenExpiration(this.accessToken); - } - this.traceVerbose( `Refreshed tunnel access token. New token: ${TunnelAccessTokenProperties.getTokenTrace( this.accessToken, @@ -639,6 +634,7 @@ export class TunnelConnectionSession extends TunnelConnectionBase implements Tun /** * Validates tunnel access token if it's present. Returns the token. + * Note: uses client's system time for the validation. */ public validateAccessToken(): string | undefined { if (this.accessToken) { diff --git a/ts/src/connections/tunnelSession.ts b/ts/src/connections/tunnelSession.ts index 65ad23a7..ccacb493 100644 --- a/ts/src/connections/tunnelSession.ts +++ b/ts/src/connections/tunnelSession.ts @@ -46,6 +46,7 @@ export interface TunnelSession extends TunnelConnection { /** * Validates tunnel access token if it's present. Returns the token. + * Note: uses client's system time for the validation. */ validateAccessToken(): string | undefined; diff --git a/ts/src/management/package.json b/ts/src/management/package.json index 8f097b4b..adf211b2 100644 --- a/ts/src/management/package.json +++ b/ts/src/management/package.json @@ -18,7 +18,7 @@ "buffer": "^5.2.1", "debug": "^4.1.1", "vscode-jsonrpc": "^4.0.0", - "@microsoft/dev-tunnels-contracts": ">1.1.10", + "@microsoft/dev-tunnels-contracts": ">1.1.11", "axios": "^1.6.6" } } diff --git a/ts/src/management/tunnelAccessTokenProperties.ts b/ts/src/management/tunnelAccessTokenProperties.ts index 6a5f41a7..7a950cdf 100644 --- a/ts/src/management/tunnelAccessTokenProperties.ts +++ b/ts/src/management/tunnelAccessTokenProperties.ts @@ -64,7 +64,7 @@ export class TunnelAccessTokenProperties { /** * Checks if the tunnel access token expiration claim is in the past. - * + * Note: uses client's system time for the validation. * (Does not throw if the token is an invalid format.) */ public static validateTokenExpiration(token: string): void { @@ -126,7 +126,6 @@ export class TunnelAccessTokenProperties { /** * Gets a tunnel access token that matches any of the provided access token scopes. - * Validates token expiration if the token is found and throws an error if it's expired. * @param tunnel The tunnel to get the access tokens from. * @param accessTokenScopes What scopes the token needs to have. * @returns Tunnel access token if found; otherwise, undefined. @@ -147,7 +146,6 @@ export class TunnelAccessTokenProperties { for (const [key, accessToken] of Object.entries(tunnel.accessTokens)) { // Each key may be either a single scope or space-delimited list of scopes. if (accessToken && key.split(' ').includes(scope)) { - TunnelAccessTokenProperties.validateTokenExpiration(accessToken); return accessToken; } } diff --git a/ts/src/management/tunnelManagementHttpClient.ts b/ts/src/management/tunnelManagementHttpClient.ts index 0190f1c1..a38058e1 100644 --- a/ts/src/management/tunnelManagementHttpClient.ts +++ b/ts/src/management/tunnelManagementHttpClient.ts @@ -907,7 +907,6 @@ export class TunnelManagementHttpClient implements TunnelManagementClient { const headers: { [name: string]: string } = {}; if (options && options.accessToken) { - TunnelAccessTokenProperties.validateTokenExpiration(options.accessToken); headers[ tunnelAuthentication ] = `${TunnelAuthenticationSchemes.tunnel} ${options.accessToken}`; diff --git a/ts/src/management/tunnelPlanTokenProperties.ts b/ts/src/management/tunnelPlanTokenProperties.ts index cd702bf5..1287c7a4 100644 --- a/ts/src/management/tunnelPlanTokenProperties.ts +++ b/ts/src/management/tunnelPlanTokenProperties.ts @@ -24,7 +24,7 @@ export class TunnelPlanTokenProperties { /** * Checks if the tunnel access token expiration claim is in the past. - * + * Note: uses client's system time for the validation. * (Does not throw if the token is an invalid format.) */ public static validateTokenExpiration(token: string): void {