Skip to content

feat: hmac authentication strategy and response verification #8262

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.

Sign up for GitHub

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

Draft
bitgoAaron wants to merge 1 commit into
base: master
Choose a base branch
from
+1,784 −56
Draft

feat: hmac authentication strategy and response verification #8262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions modules/sdk-api/src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,20 +217,20 @@ export function setRequestQueryString(req: superagent.SuperAgentRequest): void {
* Verify that the response received from the server is signed correctly.
* Right now, it is very permissive with the timestamp variance.
*/
export function verifyResponse(
export async function verifyResponse(
bitgo: BitGoAPI,
token: string | undefined,
method: VerifyResponseOptions['method'],
req: superagent.SuperAgentRequest,
response: superagent.Response,
authVersion: AuthVersion
): superagent.Response {
): Promise<superagent.Response> {
// we can't verify the response if we're not authenticated
if (!req.isV2Authenticated || !req.authenticationToken) {
return response;
}

const verificationResponse = bitgo.verifyResponse({
const verificationResponse = await bitgo.verifyResponseAsync({
url: req.url,
hmac: response.header.hmac,
statusCode: response.status,
Expand All @@ -242,7 +242,6 @@ export function verifyResponse(
});

if (!verificationResponse.isValid) {
// calculate the HMAC
const receivedHmac = response.header.hmac;
const expectedHmac = verificationResponse.expectedHmac;
const signatureSubject = verificationResponse.signatureSubject;
Expand Down
139 changes: 87 additions & 52 deletions modules/sdk-api/src/bitgoAPI.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
sanitizeLegacyPath,
} from '@bitgo/sdk-core';
import * as sdkHmac from '@bitgo/sdk-hmac';
import { DefaultHmacAuthStrategy, type IHmacAuthStrategy } from '@bitgo/sdk-hmac';
import * as utxolib from '@bitgo/utxo-lib';
import { bip32, ECPairInterface } from '@bitgo/utxo-lib';
import * as bitcoinMessage from 'bitcoinjs-message';
Expand Down Expand Up @@ -134,6 +135,7 @@
private _customProxyAgent?: Agent;
private _requestIdPrefix?: string;
private getAdditionalHeadersCb?: AdditionalHeadersCallback;
protected _hmacAuthStrategy: IHmacAuthStrategy;

constructor(params: BitGoAPIOptions = {}) {
this.getAdditionalHeadersCb = params.getAdditionalHeadersCb;
Expand Down Expand Up @@ -309,6 +311,7 @@
}

this._customProxyAgent = params.customProxyAgent;
this._hmacAuthStrategy = params.hmacAuthStrategy ?? new DefaultHmacAuthStrategy();

// Only fetch constants from constructor if clientConstants was not provided
if (!clientConstants) {
Expand Down Expand Up @@ -423,9 +426,12 @@
// Set the request timeout to just above 5 minutes by default
req.timeout((process.env.BITGO_TIMEOUT as any) * 1000 || 305 * 1000);

// if there is no token, and we're not logged in, the request cannot be v2 authenticated
// The strategy may have its own signing material (e.g. a CryptoKey
// restored from IndexedDB) independent of this._token.
const strategyAuthenticated = this._hmacAuthStrategy.isAuthenticated?.() ?? false;

req.isV2Authenticated = true;
req.authenticationToken = this._token;
req.authenticationToken = this._token ?? (strategyAuthenticated ? 'strategy-authenticated' : undefined);
// some of the older tokens appear to be only 40 characters long
if ((this._token && this._token.length !== 67 && this._token.indexOf('v2x') !== 0) || req.forceV1Auth) {
// use the old method
Expand All @@ -439,51 +445,66 @@
req.set('BitGo-Auth-Version', this._authVersion === 3 ? '3.0' : '2.0');

const data = serializeRequestData(req);
if (this._token) {
setRequestQueryString(req);

const requestProperties = this.calculateRequestHeaders({
url: req.url,
token: this._token,
method,
text: data || '',
authVersion: this._authVersion,
});
req.set('Auth-Timestamp', requestProperties.timestamp.toString());

// we're not sending the actual token, but only its hash
req.set('Authorization', 'Bearer ' + requestProperties.tokenHash);
debug('sending v2 %s request to %s with token %s', method, url, this._token?.substr(0, 8));

// set the HMAC
req.set('HMAC', requestProperties.hmac);
}
const sendWithHmac = (async () => {
if (this._token || strategyAuthenticated) {
setRequestQueryString(req);

const requestProperties = await this._hmacAuthStrategy.calculateRequestHeaders({
url: req.url,
token: this._token ?? '',
method,
text: data || '',
authVersion: this._authVersion,
});
req.set('Auth-Timestamp', requestProperties.timestamp.toString());

req.set('Authorization', 'Bearer ' + requestProperties.tokenHash);
debug(
'sending v2 %s request to %s with token %s',
method,
url,
this._token?.substr(0, 8) ?? '(strategy-managed)'

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.

Copilot Autofix

AI 1 day ago

In general, to fix log injection, any user-controlled value that is written to logs should be sanitized to remove characters that can break the log format (such as \r and \n) or otherwise be encoded/escaped. For plain-text logs, a simple and effective approach is to strip carriage-return and newline characters from the value before interpolation, while keeping the semantics the same for non-malicious inputs.

In this specific case, the only problematic usage is in modules/sdk-api/src/bitgoAPI.ts, where this._token?.substr(0, 8) ?? '(strategy-managed)' is interpolated into a debug() call. The best minimal fix is to compute a local safeTokenPrefix that is based on the same 8-character prefix but with any \r or \n characters removed, and then log that sanitized prefix instead of the raw substring. This does not change functional behavior (the token is not used for logic, only for display) but ensures that no unexpected newlines can be injected into the log. No new imports or external libraries are required; we can use String.prototype.replace with a simple regex.

Concretely, in requestPatch where debug('sending v2 %s request to %s with token %s', ...) is called, we will introduce a local const safeTokenPrefix = this._token ? this._token.substr(0, 8).replace(/[\r\n]/g, '') : '(strategy-managed)'; and then pass safeTokenPrefix as the third format argument to debug. This confines the change to the immediate logging context and preserves the existing logging format.

Suggested changeset 1
modules/sdk-api/src/bitgoAPI.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts
--- a/modules/sdk-api/src/bitgoAPI.ts
+++ b/modules/sdk-api/src/bitgoAPI.ts
@@ -460,12 +460,11 @@
           req.set('Auth-Timestamp', requestProperties.timestamp.toString());
 
           req.set('Authorization', 'Bearer ' + requestProperties.tokenHash);
-          debug(
-            'sending v2 %s request to %s with token %s',
-            method,
-            url,
-            this._token?.substr(0, 8) ?? '(strategy-managed)'
-          );
+          const safeTokenPrefix =
+            this._token !== undefined && this._token !== null
+              ? this._token.substr(0, 8).replace(/[\r\n]/g, '')
+              : '(strategy-managed)';
+          debug('sending v2 %s request to %s with token %s', method, url, safeTokenPrefix);
 
           req.set('HMAC', requestProperties.hmac);
         }
EOF
@@ -460,12 +460,11 @@
req.set('Auth-Timestamp', requestProperties.timestamp.toString());

req.set('Authorization', 'Bearer ' + requestProperties.tokenHash);
debug(
'sending v2 %s request to %s with token %s',
method,
url,
this._token?.substr(0, 8) ?? '(strategy-managed)'
);
const safeTokenPrefix =
this._token !== undefined && this._token !== null
? this._token.substr(0, 8).replace(/[\r\n]/g, '')
: '(strategy-managed)';
debug('sending v2 %s request to %s with token %s', method, url, safeTokenPrefix);

req.set('HMAC', requestProperties.hmac);
}
Copilot is powered by AI and may make mistakes. Always verify output.
);

req.set('HMAC', requestProperties.hmac);
}

if (this.getAdditionalHeadersCb) {
const additionalHeaders = this.getAdditionalHeadersCb(method, url, data);
for (const { key, value } of additionalHeaders) {
req.set(key, value);
if (this.getAdditionalHeadersCb) {
const additionalHeaders = this.getAdditionalHeadersCb(method, url, data);
for (const { key, value } of additionalHeaders) {
req.set(key, value);
}
}
}

/**
* Verify the response before calling the original onfulfilled handler,
* and make sure onrejected is called if a verification error is encountered
*/
const newOnFulfilled = onfulfilled
? (response: superagent.Response) => {
// HMAC verification is only allowed to be skipped in certain environments.
// This is checked in the constructor, but checking it again at request time
// will help prevent against tampering of this property after the object is created
if (!this._hmacVerification && !common.Environments[this.getEnv()].hmacVerificationEnforced) {
return onfulfilled(response);
/**
* Verify the response before calling the original onfulfilled handler,
* and make sure onrejected is called if a verification error is encountered
*/
const newOnFulfilled = onfulfilled
? async (response: superagent.Response) => {
// HMAC verification is only allowed to be skipped in certain environments.
// This is checked in the constructor, but checking it again at request time
// will help prevent against tampering of this property after the object is created
if (!this._hmacVerification && !common.Environments[this.getEnv()].hmacVerificationEnforced) {
return onfulfilled(response);
}

const verifiedResponse = await verifyResponse(
this,
this._token,
method,
req,
response,
this._authVersion
);
return onfulfilled(verifiedResponse);
}
: null;
return originalThen(newOnFulfilled);
})();

const verifiedResponse = verifyResponse(this, this._token, method, req, response, this._authVersion);
return onfulfilled(verifiedResponse);
}
: null;
return originalThen(newOnFulfilled).catch(onrejected);
return sendWithHmac.catch(onrejected);
};
return toBitgoRequest(req);
}
Expand Down Expand Up @@ -545,12 +566,21 @@
}

/**
* Verify the HMAC for an HTTP response
* Verify the HMAC for an HTTP response (synchronous, uses sdk-hmac directly).
* Kept for backward compatibility with external callers.
*/
verifyResponse(params: VerifyResponseOptions): VerifyResponseInfo {
return sdkHmac.verifyResponse({ ...params, authVersion: this._authVersion });
}

/**
* Verify the HMAC for an HTTP response via the configured strategy (async).
* Used internally by the request pipeline.
*/
verifyResponseAsync(params: VerifyResponseOptions): Promise<VerifyResponseInfo> {
return this._hmacAuthStrategy.verifyResponse({ ...params, authVersion: this._authVersion });
}

/**
* Fetch useful constant values from the BitGo server.
* These values do change infrequently, so they need to be fetched,
Expand Down Expand Up @@ -772,7 +802,7 @@
* Process the username, password and otp into an object containing the username and hashed password, ready to
* send to bitgo for authentication.
*/
preprocessAuthenticationParams({
async preprocessAuthenticationParams({
username,
password,
otp,
Expand All @@ -782,7 +812,7 @@
forReset2FA,
initialHash,
fingerprintHash,
}: AuthenticateOptions): ProcessedAuthenticationOptions {
}: AuthenticateOptions): Promise<ProcessedAuthenticationOptions> {
if (!_.isString(username)) {
throw new Error('expected string username');
}
Expand All @@ -793,7 +823,7 @@

const lowerName = username.toLowerCase();
// Calculate the password HMAC so we don't send clear-text passwords
const hmacPassword = this.calculateHMAC(lowerName, password);
const hmacPassword = await this._hmacAuthStrategy.calculateHMAC(lowerName, password);

const authParams: ProcessedAuthenticationOptions = {
email: lowerName,
Expand Down Expand Up @@ -944,7 +974,7 @@
}

const forceV1Auth = !!params.forceV1Auth;
const authParams = this.preprocessAuthenticationParams(params);
const authParams = await this.preprocessAuthenticationParams(params);
const password = params.password;

if (this._token) {
Expand Down Expand Up @@ -981,7 +1011,7 @@
this._ecdhXprv = responseDetails.ecdhXprv;

// verify the response's authenticity
verifyResponse(this, responseDetails.token, 'post', request, response, this._authVersion);
await verifyResponse(this, responseDetails.token, 'post', request, response, this._authVersion);

// add the remaining component for easier access
response.body.access_token = this._token;
Expand Down Expand Up @@ -1111,15 +1141,15 @@

/**
*/
verifyPassword(params: VerifyPasswordOptions = {}): Promise<any> {
async verifyPassword(params: VerifyPasswordOptions = {}): Promise<any> {
if (!_.isString(params.password)) {
throw new Error('missing required string password');
}

if (!this._user || !this._user.username) {
throw new Error('no current user');
}
const hmacPassword = this.calculateHMAC(this._user.username, params.password);
const hmacPassword = await this._hmacAuthStrategy.calculateHMAC(this._user.username, params.password);

return this.post(this.url('/user/verifypassword')).send({ password: hmacPassword }).result('valid');
}
Expand Down Expand Up @@ -1269,7 +1299,7 @@
}

// verify the authenticity of the server's response before proceeding any further
verifyResponse(this, this._token, 'post', request, response, this._authVersion);
await verifyResponse(this, this._token, 'post', request, response, this._authVersion);

const responseDetails = this.handleTokenIssuance(response.body);
response.body.token = responseDetails.token;
Expand Down Expand Up @@ -1924,12 +1954,17 @@
const v1KeychainUpdatePWResult = await this.keychains().updatePassword(updateKeychainPasswordParams);
const v2Keychains = await this.coin(coin).keychains().updatePassword(updateKeychainPasswordParams);

const [hmacOldPassword, hmacNewPassword] = await Promise.all([
this._hmacAuthStrategy.calculateHMAC(user.username, oldPassword),
this._hmacAuthStrategy.calculateHMAC(user.username, newPassword),
]);

const updatePasswordParams = {
keychains: v1KeychainUpdatePWResult.keychains,
v2_keychains: v2Keychains,
version: v1KeychainUpdatePWResult.version,
oldPassword: this.calculateHMAC(user.username, oldPassword),
password: this.calculateHMAC(user.username, newPassword),
oldPassword: hmacOldPassword,
password: hmacNewPassword,
};

// Calculate payload size in KB
Expand Down
5 changes: 5 additions & 0 deletions modules/sdk-api/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,19 @@ export {
CalculateHmacSubjectOptions,
CalculateRequestHeadersOptions,
CalculateRequestHmacOptions,
IHmacAuthStrategy,
RequestHeaders,
supportedRequestMethods,
VerifyResponseInfo,
VerifyResponseOptions,
} from '@bitgo/sdk-hmac';

import type { IHmacAuthStrategy } from '@bitgo/sdk-hmac';

export interface BitGoAPIOptions {
accessToken?: string;
authVersion?: 2 | 3;
hmacAuthStrategy?: IHmacAuthStrategy;
clientConstants?:
| Record<string, any>
| {
Expand Down
Loading