Skip to content

Commit

Permalink
Remove internal usage of ValidateTokenExpiration. (#393)
Browse files Browse the repository at this point in the history
* - remove internal usage of validateTokenExpiration as it uses client's system time which can cause issues.

* update SDK version
  • Loading branch information
jramsay authored Jan 31, 2024
1 parent c1bf142 commit 56e1761
Show file tree
Hide file tree
Showing 13 changed files with 18 additions and 31 deletions.
1 change: 1 addition & 0 deletions cs/src/Connections/TunnelConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ protected virtual void OnTunnelChanged()
/// Validate <see cref="accessToken"/> if it is not null or empty.
/// </summary>
/// <exception cref="UnauthorizedAccessException">is thrown if the <see cref="accessToken"/> is expired.</exception>
/// <remarks>Note: uses the client's system time for validation.</remarks>
protected virtual void ValidateAccessToken()
{
if (!string.IsNullOrEmpty(this.accessToken))
Expand Down
7 changes: 0 additions & 7 deletions cs/src/Connections/TunnelRelayConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,6 @@ protected virtual async Task<Stream> 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,
Expand Down Expand Up @@ -398,12 +397,6 @@ protected virtual async Task<bool> 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));
Expand Down
5 changes: 4 additions & 1 deletion cs/src/Management/TunnelAccessTokenProperties.cs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,10 @@ public override string ToString()
/// Checks if the tunnel access token expiration claim is in the past.
/// </summary>
/// <exception cref="UnauthorizedAccessException">The token is expired.</exception>
/// <remarks>Note this does not throw if the token is an invalid format.</remarks>
/// <remarks>
/// Note this does not throw if the token is an invalid format.
/// Uses the client's system time for validation.
/// </remarks>
public static void ValidateTokenExpiration(string token)
{
var t = TryParse(token);
Expand Down
1 change: 1 addition & 0 deletions cs/src/Management/TunnelExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public static bool TryGetAccessToken(this Tunnel tunnel, string accessTokenScope
/// The tokens are searched in <c>Tunnel.AccessTokens</c> 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.
/// </remarks>
/// <param name="tunnel">The tunnel to get the access token from.</param>
/// <param name="accessTokenScope">Access token scope to get the token for.</param>
Expand Down
3 changes: 1 addition & 2 deletions cs/src/Management/TunnelManagementClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,6 @@ private Uri BuildTunnelUri(

if (!string.IsNullOrEmpty(options?.AccessToken))
{
TunnelAccessTokenProperties.ValidateTokenExpiration(options.AccessToken);
authHeader = new AuthenticationHeaderValue(
TunnelAuthenticationScheme, options.AccessToken);
}
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions ts/src/connections/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 1 addition & 5 deletions ts/src/connections/tunnelConnectionBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down
12 changes: 4 additions & 8 deletions ts/src/connections/tunnelConnectionSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}>`);
}

Expand All @@ -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
);

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions ts/src/connections/tunnelSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion ts/src/management/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
4 changes: 1 addition & 3 deletions ts/src/management/tunnelAccessTokenProperties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand All @@ -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;
}
}
Expand Down
1 change: 0 additions & 1 deletion ts/src/management/tunnelManagementHttpClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
Expand Down
2 changes: 1 addition & 1 deletion ts/src/management/tunnelPlanTokenProperties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 56e1761

Please sign in to comment.