Skip to content
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

[KeyVault Keys] Fixed the rollup warnings #11549

Merged
merged 5 commits into from
Oct 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion sdk/keyvault/keyvault-keys/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@
],
"browser": {
"os": false,
"process": false
"process": false,
"./dist-esm/keyvault-keys/src/localCryptography/algorithms.js": "./dist-esm/keyvault-keys/src/localCryptography/algorithms.browser.js",
"./dist-esm/keyvault-keys/src/localCryptography/hash.js": "./dist-esm/keyvault-keys/src/localCryptography/hash.browser.js"
},
"scripts": {
"audit": "node ../../../common/scripts/rush-audit.js && rimraf node_modules package-lock.json && npm i --package-lock-only 2>&1 && npm audit",
Expand Down
2 changes: 1 addition & 1 deletion sdk/keyvault/keyvault-keys/rollup.base.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ export function browserConfig(test = false) {
]
};

baseConfig.external = ["fs-extra", "path", "crypto", "constants"];
baseConfig.external = ["fs-extra", "path"];
if (test) {
baseConfig.input = ["dist-esm/**/*.spec.js"];
baseConfig.plugins.unshift(multiEntry({ exports: false }));
Expand Down
13 changes: 6 additions & 7 deletions sdk/keyvault/keyvault-keys/src/cryptographyClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,7 @@ import { SDK_VERSION } from "./generated/utils/constants";
import { KeyVaultClient } from "./generated/keyVaultClient";
import { challengeBasedAuthenticationPolicy } from "../../keyvault-common/src";

import {
LocalSupportedAlgorithmName,
localSupportedAlgorithms,
LocalCryptographyOperationFunction,
isLocallySupported
} from "./localCryptography/algorithms";
import { localSupportedAlgorithms, isLocallySupported } from "./localCryptography/algorithms";

import { LocalCryptographyClient } from "./localCryptographyClient";

Expand All @@ -50,12 +45,16 @@ import {
} from "./cryptographyClientModels";
import { KeyBundle } from "./generated/models";
import { parseKeyVaultKeyId } from "./identifier";
import {
LocalCryptographyOperationFunction,
LocalSupportedAlgorithmName
} from "./localCryptography/models";

/**
* Checks whether a key can be used at that specific moment,
* by comparing the current date with the bundle's notBefore and expires values.
*/
export function checkKeyValidity(keyId?: string, keyBundle?: KeyBundle) {
export function checkKeyValidity(keyId?: string, keyBundle?: KeyBundle): void {
const attributes = keyBundle?.attributes || {};
const { notBefore, expires } = attributes;
const now = new Date();
Expand Down
2 changes: 1 addition & 1 deletion sdk/keyvault/keyvault-keys/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,9 @@ import {
WrapResult,
EncryptResult
} from "./cryptographyClientModels";
import { LocalSupportedAlgorithmName } from "./localCryptography/algorithms";

import { parseKeyVaultKeyId, KeyVaultKeyId } from "./identifier";
import { LocalSupportedAlgorithmName } from "./localCryptography/models";

export {
CryptographyClientOptions,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { LocalAssertion, LocalSupportedAlgorithmsRecord } from "./models";

/**
* This file contains the implementation of local supported algorithms for the browser.
*
* We currently don't support any cryptography operation in the browser.
*
*/

/**
* @internal
* @ignore
* The list of known assertions so far.
* Assertions verify that the requirements to execute a local cryptography operation are met.
*/
export const assertions: Record<string, LocalAssertion> = {};
Copy link
Contributor

@ramya-rao-a ramya-rao-a Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note, the below feedback is not a blocker for this PR, but we should look into this nevertheless and do any follow ups in a separate PR.

I was trying to figure out whey we need to export these assertions. We use these assertions only in the validate method in each algorithm. Since we dont have any algorithm in browser, this got me thinking as to why we even need it which then lead me to the tests in the file localCryptography.spect.ts file. So, a few questions:

  • When we run the tests in the file localCryptography.spect.ts in CI in the browser, what is assertions pointing to? If it is the assertions in the algorithms.ts file, then we are essentially not getting the coverage for "browser code". If it is the assertions in the algorithms.browser.ts file, then tests would fail because assertions is an empty object as seen above. This delves into a generic question around how do we expect our tests to work when the code is split between node and browser like how are are doing in this PR and how we have done in many other places
  • Looking at the tests themselves, it felt a little odd to me that we were testing assertions. Shouldnt we be testing that the right assertions are added to the alogrithms i.e. Wouldnt we get a more appropriate coverage by calling validate() on the different algorithms and checking if the expected error was thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like those ideas! After this release, I'll go back to this PR and make issues based on your feedback. Thank you 🌞

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't currently run cryptography tests on the browser. I can take your validate feedback and work with it on a separate issue! Here's the issue: #11779


/**
* A plain object containing all of the locally supported algorithms.
*/
export const localSupportedAlgorithms: LocalSupportedAlgorithmsRecord = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the current types as is, this contraption is necessary. As soon as we get into browser support, this will expand and be coherent.

RSA1_5: undefined,
"RSA-OAEP": undefined,
PS256: undefined,
RS256: undefined,
PS384: undefined,
RS384: undefined,
PS512: undefined,
RS512: undefined
};

/**
* Checks whether a given algorithm name is supported or not.
* @param algorithm string name of the algorithm
*/
export function isLocallySupported(_algorithm: string): boolean {
return false;
}
101 changes: 8 additions & 93 deletions sdk/keyvault/keyvault-keys/src/localCryptography/algorithms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ import { publicEncrypt, createVerify } from "crypto";
import * as constants from "constants";
import { isNode } from "@azure/core-http";
import { JsonWebKey, KeyOperation } from "../keysModels";
import { LocalCryptographyUnsupportedError } from "./models";
import {
LocalAssertion,
LocalCryptographyOperationName,
LocalCryptographyUnsupportedError,
LocalSupportedAlgorithm,
LocalSupportedAlgorithmName,
LocalSupportedAlgorithmsRecord
} from "./models";
import { createHash } from "./hash";

/**
Expand All @@ -23,19 +30,6 @@ import { createHash } from "./hash";
* we will be able to increase the support of our existing algorithms.
*/

/**
* @internal
* @ignore
* Abstract representation of a assertion.
* Assertions verify that the requirements to execute a local cryptography operation are met.
* @param key The JSON Web Key that will be used during the local operation.
* @param operationName The name of the operation, as in "encrypt", "decrypt", "sign", etc.
*/
export type LocalAssertion = (
key?: JsonWebKey,
operationName?: LocalCryptographyOperationName
) => void;

/**
* @internal
* @ignore
Expand Down Expand Up @@ -80,77 +74,6 @@ const pipeAssertions = (...assertions: LocalAssertion[]): LocalAssertion => (...
}
};

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These types were moved to localCryptography/models.ts.

* TypeScript fancy for making plain objects require at least one key-value pair of another set of key-values.
*/
export type RequireAtLeastOne<T> = {
[K in keyof T]-?: Required<Pick<T, K>> & Partial<Pick<T, Exclude<keyof T, K>>>;
}[keyof T];

/**
* Union type representing the names of the supported local cryptography operations.
*/
export type LocalCryptographyOperationName = "encrypt" | "wrapKey" | "createHash" | "verify";

/**
* Abstract representation of a Local Cryptography Operation function.
* @param keyPEM The string representation of a PEM key.
* @param data The data used on the cryptography operation, in Buffer type.
*/
export type LocalCryptographyOperationFunction = (keyPEM: string, data: Buffer) => Promise<Buffer>;

/**
* Abstract representation of a Local Cryptography Operation function, this time with an additional signature buffer.
* @param keyPEM The string representation of a PEM key.
* @param data The data used on the cryptography operation, in Buffer type.
* @param signature The signature used on the cryptography operation, in Buffer type.
*/
export type LocalCryptographyOperationFunctionWithSignature = (
keyPEM: string,
data: Buffer,
signature: Buffer
) => Promise<boolean>;

/**
* Key-value map of local cryptography operations.
*/
export type LocalCryptographyOperations = Record<
LocalCryptographyOperationName,
LocalCryptographyOperationFunction | LocalCryptographyOperationFunctionWithSignature
>;

/**
* Abstract representation of a locally supported cryptography algorithm, with its assertions,
* and its operations.
*/
export interface LocalSupportedAlgorithm {
/**
* List of assertions that need to pass in order to execute this cryptography operation.
*/
validate: LocalAssertion;
/**
* Optional algorithm used to sign or validate data.
*/
signAlgorithm?: string;
/**
* List of local cryptography operations supported by an algorithm.
*/
operations: RequireAtLeastOne<LocalCryptographyOperations>;
}

/**
* A union type representing the names of all of the locally supported algorithms.
*/
export type LocalSupportedAlgorithmName =
| "RSA1_5"
| "RSA-OAEP"
| "PS256"
| "RS256"
| "PS384"
| "RS384"
| "PS512"
| "RS512";

/**
* Local support of the RSA1_5 algorithm.
* We currently only support encrypting and wrapping keys with it.
Expand Down Expand Up @@ -212,14 +135,6 @@ const makeSigner = (signAlgorithm: SignAlgorithmName): LocalSupportedAlgorithm =
};
};

/**
* A Record containing all of the locally supported algorithms.
*/
export type LocalSupportedAlgorithmsRecord = Record<
LocalSupportedAlgorithmName,
LocalSupportedAlgorithm
>;

/**
* A plain object containing all of the locally supported algorithms.
*/
Expand Down
15 changes: 15 additions & 0 deletions sdk/keyvault/keyvault-keys/src/localCryptography/hash.browser.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { LocalCryptographyUnsupportedError } from "./models";

/**
* @internal
* @ignore
* Use the platform-local hashing functionality
*/
export async function createHash(_algorithm: string, _data: Uint8Array): Promise<Buffer> {
throw new LocalCryptographyUnsupportedError(
"Our libraries don't currently support browser hashing"
);
}
16 changes: 4 additions & 12 deletions sdk/keyvault/keyvault-keys/src/localCryptography/hash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,15 @@
// Licensed under the MIT license.

import { createHash as cryptoCreateHash } from "crypto";
import { isNode } from "@azure/core-http";
import { LocalCryptographyUnsupportedError } from "./models";

/**
* @internal
* @ignore
* Use the platform-local hashing functionality
*/
export async function createHash(algorithm: string, data: Uint8Array): Promise<Buffer> {
if (isNode) {
const hash = cryptoCreateHash(algorithm);
hash.update(Buffer.from(data));
const digest = hash.digest();
return digest;
} else {
throw new LocalCryptographyUnsupportedError(
"Our libraries don't currently support browser hashing"
);
}
const hash = cryptoCreateHash(algorithm);
hash.update(Buffer.from(data));
const digest = hash.digest();
return digest;
}
94 changes: 94 additions & 0 deletions sdk/keyvault/keyvault-keys/src/localCryptography/models.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,98 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { JsonWebKey } from "../keysModels";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These types were just moved out of the algorithms.ts file.


/**
* TypeScript fancy for making plain objects require at least one key-value pair of another set of key-values.
*/
export type RequireAtLeastOne<T> = {
[K in keyof T]-?: Required<Pick<T, K>> & Partial<Pick<T, Exclude<keyof T, K>>>;
}[keyof T];

/**
* Union type representing the names of the supported local cryptography operations.
*/
export type LocalCryptographyOperationName = "encrypt" | "wrapKey" | "createHash" | "verify";

/**
* @internal
* @ignore
* Abstract representation of a assertion.
* Assertions verify that the requirements to execute a local cryptography operation are met.
* @param key The JSON Web Key that will be used during the local operation.
* @param operationName The name of the operation, as in "encrypt", "decrypt", "sign", etc.
*/
export type LocalAssertion = (
key?: JsonWebKey,
operationName?: LocalCryptographyOperationName
) => void;

/**
* A union type representing the names of all of the locally supported algorithms.
*/
export type LocalSupportedAlgorithmName =
| "RSA1_5"
| "RSA-OAEP"
| "PS256"
| "RS256"
| "PS384"
| "RS384"
| "PS512"
| "RS512";

/**
* Abstract representation of a Local Cryptography Operation function.
* @param keyPEM The string representation of a PEM key.
* @param data The data used on the cryptography operation, in Buffer type.
*/
export type LocalCryptographyOperationFunction = (keyPEM: string, data: Buffer) => Promise<Buffer>;

/**
* Abstract representation of a Local Cryptography Operation function, this time with an additional signature buffer.
* @param keyPEM The string representation of a PEM key.
* @param data The data used on the cryptography operation, in Buffer type.
* @param signature The signature used on the cryptography operation, in Buffer type.
*/
export type LocalCryptographyOperationFunctionWithSignature = (
keyPEM: string,
data: Buffer,
signature: Buffer
) => Promise<boolean>;

/**
* Key-value map of local cryptography operations.
*/
export type LocalCryptographyOperations = Record<
LocalCryptographyOperationName,
LocalCryptographyOperationFunction | LocalCryptographyOperationFunctionWithSignature
>;

/**
* Abstract representation of a locally supported cryptography algorithm, with its assertions,
* and its operations.
*/
export interface LocalSupportedAlgorithm {
/**
* List of assertions that need to pass in order to execute this cryptography operation.
*/
validate: LocalAssertion;
/**
* Optional algorithm used to sign or validate data.
*/
signAlgorithm?: string;
/**
* List of local cryptography operations supported by an algorithm.
*/
operations: RequireAtLeastOne<LocalCryptographyOperations>;
}

/**
* A Record containing all of the locally supported algorithms.
*/
export type LocalSupportedAlgorithmsRecord = Record<
LocalSupportedAlgorithmName,
LocalSupportedAlgorithm | undefined
>;

export class LocalCryptographyUnsupportedError extends Error {}
Loading