feat: hmac authentication strategy and response verification#8262
feat: hmac authentication strategy and response verification#8262bitgoAaron wants to merge 2 commits intomasterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
bitgoAaron
force-pushed
the
aloe/hmacWebCryptoStrategy
branch
2 times, most recently
from
729a786 to
0cbe167
Compare
bitgoAaron
force-pushed
the
aloe/hmacWebCryptoStrategy
branch
from
0cbe167 to
49529e2
Compare
- Updated function to be asynchronous, allowing for better handling of HMAC verification. - Introduced for default HMAC handling and added support for custom strategies. - Integrated for browser compatibility, enabling HMAC signing and verification using the Web Crypto API. - Enhanced to utilize the new HMAC strategies for request signing and response verification. - Added unit tests for the new HMAC strategies and their integration with the BitGoAPI. - Updated web demo to include a new component for WebCrypto authentication. Ticket: CE-10122
- use timingSafeEqual for comparing hmac values - enhance web demo to support both auth versions Ticket: CE-10122
bitgoAaron
force-pushed
the
aloe/hmacWebCryptoStrategy
branch
from
49529e2 to
ea9dd57
Compare
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| * indicates the response is invalid or outside the acceptable time window. | ||
| */ | ||
| export function verifyResponse( | ||
| function assertVerificationResponse( |
There was a problem hiding this comment.
👍 good naming pattern
| if (!req.isV2Authenticated || !req.authenticationToken) { | ||
| return response; | ||
| } |
There was a problem hiding this comment.
can we maybe move this to the caller? it's a bit unclear in this context here why we would want to skip these reqs
| if (!req.isV2Authenticated || !req.authenticationToken) { | ||
| return response; | ||
| } |
| // set the HMAC | ||
| req.set('HMAC', requestProperties.hmac); | ||
| } | ||
| const sendWithHmac = (async () => { |
There was a problem hiding this comment.
clarity: this deserves to be a method
| let method = params.method; | ||
| if (method === 'del') { | ||
| method = 'delete'; | ||
| } |
There was a problem hiding this comment.
Looks like we need to normalize the method here, maybe we should go further and introduce a more strict type. Unclear what the callers are but maybe we need to lowercase for instance.
| if (!hasIndexedDB()) return; | ||
| const db = await openCryptoDb(); | ||
| try { | ||
| await new Promise<void>((resolve, reject) => { | ||
| const tx = db.transaction(CRYPTO_STORE_NAME, 'readwrite'); | ||
| tx.objectStore(CRYPTO_STORE_NAME).delete(CRYPTO_RECORD_KEY); | ||
| tx.oncomplete = () => resolve(); | ||
| tx.onerror = () => reject(tx.error); | ||
| }); | ||
| } finally { | ||
| db.close(); | ||
| } |
There was a problem hiding this comment.
the early return, try/catch/finally pattern could be a withDb wrapper
| } | ||
|
|
||
| async function persistCryptoSigning(signing: CryptoSigning): Promise<void> { | ||
| if (!hasIndexedDB()) return; |
There was a problem hiding this comment.
it would be a little nicer to have a class CryptoStore with a db: IDBDatabase property with some methods
| token: '', | ||
| method: params.method as CalculateRequestHeadersOptions['method'], | ||
| text: params.body ?? '', |
There was a problem hiding this comment.
we often seem to pass empty strings to token and text - better make them optional params instead and normalize in the impl
| const MOCK_TIMESTAMP = 1672531200000; | ||
|
|
||
| /** | ||
| * In-memory token store for testing (IndexedDB is not available in Node.js). |
There was a problem hiding this comment.
it may be worthwhile pulling in fake-indexeddb as a dev dep
| */ | ||
| function buildHmacSubject(params: { | ||
| urlPath: string; | ||
| text: string; |
There was a problem hiding this comment.
this seems actually optional
3 participants
Ticket: CE-10122