-
Notifications
You must be signed in to change notification settings - Fork 2.7k
AT Proof-Of-Possession #3: Browser Implementation of token signing and key storage #2154
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
Changes from 1 commit
589a22a
a12d371
028d676
fbff7dc
def300e
7046ebf
c33aa5a
fb00edf
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 |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/* | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. | ||
*/ | ||
|
||
interface IDBOpenDBRequestEvent extends Event { | ||
target: IDBOpenDBRequest & EventTarget; | ||
} | ||
|
||
interface IDBOpenOnUpgradeNeededEvent extends IDBVersionChangeEvent { | ||
target: IDBOpenDBRequest & EventTarget; | ||
} | ||
|
||
interface IDBRequestEvent extends Event { | ||
target: IDBRequest & EventTarget; | ||
} | ||
|
||
/** | ||
* Storage wrapper for IndexedDB storage in browsers: https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API | ||
*/ | ||
export class DatabaseStorage<T>{ | ||
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. Nit Pick Could we call this IndexDatabaseStorage to be more specific 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. That is leaking implementation details, which I think we should avoid doing when naming things when possible. |
||
private _db : IDBDatabase; | ||
private _dbName: string; | ||
private _tableName: string; | ||
private _version: number; | ||
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 we should avoid using underscores for private variables. 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. done. |
||
|
||
constructor(dbName: string, tableName: string, version: number) { | ||
this._dbName = dbName; | ||
this._tableName = tableName; | ||
this._version = version; | ||
} | ||
|
||
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. What about delete? 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. resolved offline. |
||
/** | ||
* Opens IndexedDB instance. | ||
*/ | ||
async open(): Promise<void> { | ||
return new Promise((resolve, reject) => { | ||
// TODO: Add timeouts? | ||
const openDB = window.indexedDB.open(this._dbName, this._version); | ||
openDB.addEventListener("upgradeneeded", (e: IDBOpenOnUpgradeNeededEvent) => { | ||
e.target.result.createObjectStore(this._tableName); | ||
}); | ||
openDB.addEventListener("success", (e: IDBOpenDBRequestEvent) => { | ||
this._db = e.target.result; | ||
resolve(); | ||
}); | ||
|
||
openDB.addEventListener("error", error => reject(error)); | ||
}); | ||
} | ||
|
||
/** | ||
* Retrieves item from IndexedDB instance. | ||
* @param key | ||
*/ | ||
async get(key: string): Promise<T> { | ||
return new Promise((resolve, reject) => { | ||
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. Do you need to pass the generic type to the 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. done. |
||
// TODO: Add timeouts? | ||
const transaction = this._db.transaction([this._tableName], "readonly"); | ||
|
||
const objectStore = transaction.objectStore(this._tableName); | ||
const dbGet = objectStore.get(key); | ||
dbGet.addEventListener("success", (e: IDBRequestEvent) => resolve(e.target.result)); | ||
dbGet.addEventListener("error", e => reject(e)); | ||
}); | ||
} | ||
|
||
/** | ||
* Adds item to IndexedDB under given key | ||
* @param key | ||
* @param payload | ||
*/ | ||
async put(key: string, payload: T): Promise<T> { | ||
return new Promise((resolve: any, reject: any) => { | ||
// TODO: Add timeouts? | ||
const transaction = this._db.transaction([this._tableName], "readwrite"); | ||
const objectStore = transaction.objectStore(this._tableName); | ||
|
||
const dbPut = objectStore.put(payload, key); | ||
dbPut.addEventListener("success", (e: IDBRequestEvent) => resolve(e.target.result)); | ||
dbPut.addEventListener("error", e => reject(e)); | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,7 +49,6 @@ export class BrowserCrypto { | |
modulusLength: keygenConfigModulusLength, | ||
publicExponent: keygenConfigPublicExponent | ||
}; | ||
console.log(this._keygenAlgorithmOptions); | ||
} | ||
|
||
/** | ||
|
@@ -96,6 +95,33 @@ export class BrowserCrypto { | |
return this.hasIECrypto() ? this.msCryptoExportKey(key, format) : window.crypto.subtle.exportKey(format, key); | ||
} | ||
|
||
/** | ||
* Imports key as given KeyFormat, can set extractable and usages. | ||
* @param key | ||
* @param format | ||
* @param extractable | ||
* @param usages | ||
*/ | ||
async importKey(key: JsonWebKey, format: KeyFormat, extractable: boolean, usages: Array<KeyUsage>): Promise<CryptoKey> { | ||
const keyString = BrowserCrypto.getJwkString(key); | ||
const keyBuffer = BrowserStringUtils.stringToArrayBuffer(keyString); | ||
|
||
return this.hasIECrypto() ? | ||
this.msCryptoImportKey(keyBuffer, format, extractable, usages) | ||
: window.crypto.subtle.importKey(format, key, this._keygenAlgorithmOptions, extractable, usages); | ||
} | ||
|
||
/** | ||
* Signs given data with given key | ||
* @param key | ||
* @param data | ||
*/ | ||
async sign(key: CryptoKey, data: ArrayBuffer): Promise<ArrayBuffer> { | ||
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. nit pick 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 was trying to keep naming consistent with the browser crypto APIs. |
||
return this.hasIECrypto() ? | ||
this.msCryptoSign(key, data) | ||
: window.crypto.subtle.sign(this._keygenAlgorithmOptions, key, data); | ||
} | ||
|
||
/** | ||
* Check whether IE crypto or other browser cryptography is available. | ||
*/ | ||
|
@@ -143,6 +169,11 @@ export class BrowserCrypto { | |
}); | ||
} | ||
|
||
/** | ||
* IE Helper function for generating a keypair | ||
* @param extractable | ||
* @param usages | ||
*/ | ||
private async msCryptoGenerateKey(extractable: boolean, usages: Array<KeyUsage>): Promise<CryptoKeyPair> { | ||
return new Promise((resolve: any, reject: any) => { | ||
const msGenerateKey = window["msCrypto"].subtle.generateKey(this._keygenAlgorithmOptions, extractable, usages); | ||
|
@@ -157,7 +188,7 @@ export class BrowserCrypto { | |
} | ||
|
||
/** | ||
* IE Helper function for exporting keys | ||
* IE Helper function for exportKey | ||
* @param key | ||
* @param format | ||
*/ | ||
|
@@ -187,6 +218,44 @@ export class BrowserCrypto { | |
}); | ||
} | ||
|
||
/** | ||
* IE Helper function for importKey | ||
* @param key | ||
* @param format | ||
* @param extractable | ||
* @param usages | ||
*/ | ||
private async msCryptoImportKey(keyBuffer: ArrayBuffer, format: KeyFormat, extractable: boolean, usages: Array<KeyUsage>): Promise<CryptoKey> { | ||
return new Promise((resolve: any, reject: any) => { | ||
const msImportKey = window["msCrypto"].subtle.importKey(format, keyBuffer, this._keygenAlgorithmOptions, extractable, usages); | ||
msImportKey.addEventListener("complete", (e: { target: { result: CryptoKey | PromiseLike<CryptoKey>; }; }) => { | ||
resolve(e.target.result); | ||
}); | ||
|
||
msImportKey.addEventListener("error", (error: any) => { | ||
reject(error); | ||
}); | ||
}); | ||
} | ||
|
||
/** | ||
* IE Helper function for sign JWT | ||
* @param key | ||
* @param data | ||
*/ | ||
private async msCryptoSign(key: CryptoKey, data: ArrayBuffer): Promise<ArrayBuffer> { | ||
return new Promise((resolve: any, reject: any) => { | ||
const msSign = window["msCrypto"].subtle.sign(this._keygenAlgorithmOptions, key, data); | ||
msSign.addEventListener("complete", (e: { target: { result: ArrayBuffer | PromiseLike<ArrayBuffer>; }; }) => { | ||
resolve(e.target.result); | ||
}); | ||
|
||
msSign.addEventListener("error", (error: any) => { | ||
reject(error); | ||
}); | ||
}); | ||
} | ||
|
||
/** | ||
* Returns stringified jwk. | ||
* @param jwk | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,22 @@ | |
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. | ||
*/ | ||
import { ICrypto, PkceCodes } from "@azure/msal-common"; | ||
import { ICrypto, PkceCodes, SignedHttpRequest } from "@azure/msal-common"; | ||
import { GuidGenerator } from "./GuidGenerator"; | ||
import { Base64Encode } from "../encode/Base64Encode"; | ||
import { Base64Decode } from "../encode/Base64Decode"; | ||
import { PkceGenerator } from "./PkceGenerator"; | ||
import { BrowserCrypto, KeyFormat } from "./BrowserCrypto"; | ||
import { DatabaseStorage } from "../cache/DatabaseStorage"; | ||
import { BrowserStringUtils } from "../utils/BrowserStringUtils"; | ||
|
||
type CachedKeyPair = { | ||
publicKey: CryptoKey, | ||
privateKey: CryptoKey, | ||
requestMethod: string, | ||
requestUri: string | ||
}; | ||
|
||
/** | ||
* This class implements MSAL's crypto interface, which allows it to perform base64 encoding and decoding, generating cryptographically random GUIDs and | ||
* implementing Proof Key for Code Exchange specs for the OAuth Authorization Code Flow using PKCE (rfc here: https://tools.ietf.org/html/rfc7636). | ||
|
@@ -26,23 +34,20 @@ export class CryptoOps implements ICrypto { | |
private static EXTRACTABLE: boolean = true; | ||
private static POP_HASH_LENGTH = 43; // 256 bit digest / 6 bits per char = 43 | ||
|
||
private static DB_VERSION = 1; | ||
private static DB_NAME = "msal.db"; | ||
private static TABLE_NAME =`${CryptoOps.DB_NAME}.keys`; | ||
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. These should be in constants 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. These aren't used outside the context of the crypto APIs, so I chose to keep them here. |
||
private _cache: DatabaseStorage<CachedKeyPair>; | ||
|
||
constructor() { | ||
// Browser crypto needs to be validated first before any other classes can be set. | ||
this.browserCrypto = new BrowserCrypto(); | ||
this.b64Encode = new Base64Encode(); | ||
this.b64Decode = new Base64Decode(); | ||
this.guidGenerator = new GuidGenerator(this.browserCrypto); | ||
this.pkceGenerator = new PkceGenerator(this.browserCrypto); | ||
} | ||
|
||
async getPublicKeyThumprint(): Promise<string> { | ||
const keyPair = await this.browserCrypto.generateKeyPair(CryptoOps.EXTRACTABLE, CryptoOps.POP_KEY_USAGES); | ||
// TODO: Store keypair | ||
const publicKeyJwk: JsonWebKey = await this.browserCrypto.exportKey(keyPair.publicKey, KeyFormat.jwk); | ||
const publicJwkString: string = BrowserCrypto.getJwkString(publicKeyJwk); | ||
const publicJwkBuffer: ArrayBuffer = await this.browserCrypto.sha256Digest(publicJwkString); | ||
const publicJwkDigest: string = this.b64Encode.urlEncodeArr(new Uint8Array(publicJwkBuffer)); | ||
return this.base64Encode(publicJwkDigest).substr(0, CryptoOps.POP_HASH_LENGTH); | ||
this._cache = new DatabaseStorage(CryptoOps.DB_NAME, CryptoOps.TABLE_NAME, CryptoOps.DB_VERSION); | ||
this._cache.open(); | ||
} | ||
|
||
/** | ||
|
@@ -75,4 +80,53 @@ export class CryptoOps implements ICrypto { | |
async generatePkceCodes(): Promise<PkceCodes> { | ||
return this.pkceGenerator.generateCodes(); | ||
} | ||
|
||
/** | ||
* Generates a keypair, stores it and returns a thumbprint | ||
* @param resourceRequestMethod | ||
* @param resourceRequestUri | ||
*/ | ||
async getPublicKeyThumprint(resourceRequestMethod: string, resourceRequestUri: string): Promise<string> { | ||
const keyPair = await this.browserCrypto.generateKeyPair(CryptoOps.EXTRACTABLE, CryptoOps.POP_KEY_USAGES); | ||
const publicKeyJwk: JsonWebKey = await this.browserCrypto.exportKey(keyPair.publicKey, KeyFormat.jwk); | ||
const privateKeyJwk: JsonWebKey = await this.browserCrypto.exportKey(keyPair.privateKey, KeyFormat.jwk); | ||
const publicJwkString: string = BrowserCrypto.getJwkString(publicKeyJwk); | ||
const publicJwkBuffer: ArrayBuffer = await this.browserCrypto.sha256Digest(publicJwkString); | ||
const publicJwkDigest: string = this.b64Encode.urlEncodeArr(new Uint8Array(publicJwkBuffer)); | ||
const unextractablePrivateKey: CryptoKey = await this.browserCrypto.importKey(privateKeyJwk, KeyFormat.jwk, false, ["sign"]); | ||
const publicKeyHash = this.base64Encode(publicJwkDigest).substr(0, CryptoOps.POP_HASH_LENGTH); | ||
this._cache.put(publicKeyHash, { | ||
privateKey: unextractablePrivateKey, | ||
publicKey: keyPair.publicKey, | ||
requestMethod: resourceRequestMethod, | ||
requestUri: resourceRequestUri | ||
}); | ||
return publicKeyHash; | ||
} | ||
|
||
/** | ||
* Signs the given object as a jwt payload with private key retrieved by given kid. | ||
* @param payload | ||
* @param kid | ||
*/ | ||
async signJwt(payload: SignedHttpRequest, kid: string): Promise<string> { | ||
const cachedKeyPair: CachedKeyPair = await this._cache.get(kid); | ||
const publicKeyJwk = await this.browserCrypto.exportKey(cachedKeyPair.publicKey, KeyFormat.jwk); | ||
const publicKeyJwkString = BrowserCrypto.getJwkString(publicKeyJwk); | ||
|
||
const header = { | ||
alg: publicKeyJwk.alg, | ||
type: KeyFormat.jwk, | ||
jwk: JSON.parse(publicKeyJwkString) | ||
}; | ||
|
||
const encodedHeader = this.b64Encode.urlEncode(JSON.stringify(header)); | ||
const encodedPayload = this.b64Encode.urlEncode(JSON.stringify(payload)); | ||
const tokenString = `${encodedHeader}.${encodedPayload}`; | ||
const tokenBuffer = BrowserStringUtils.stringToArrayBuffer(tokenString); | ||
const signatureBuffer = await this.browserCrypto.sign(cachedKeyPair.privateKey, tokenBuffer); | ||
const encodedSignature = this.b64Encode.urlEncode(BrowserStringUtils.utf8ArrToString(new Uint8Array(signatureBuffer))); | ||
|
||
return `${tokenString}.${encodedSignature}`; | ||
} | ||
} |
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.
Since someone might want to use IndexedDB as a storage option in the future, should we have this be an implementation of CacheManager? I understand there's complexities around async storage but curious if others think it's worth the investment to figure it out now instead of refactoring later?
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.
I think async storage will be a separate discussion. For now I think I will leave this as a specific implementation in our lib.