Skip to content

Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2764)" #3012

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

Merged
merged 6 commits into from
May 7, 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
7 changes: 7 additions & 0 deletions packages/firestore/src/platform/platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ export interface Platform {
/** Converts a binary string to a Base64 encoded string. */
btoa(raw: string): string;

/**
* Generates `nBytes` of random bytes.
*
* If `nBytes < 0` , an error will be thrown.
*/
randomBytes(nBytes: number): Uint8Array;

/** The Platform's 'window' implementation or null if not available. */
readonly window: Window | null;

Expand Down
21 changes: 21 additions & 0 deletions packages/firestore/src/platform_browser/browser_platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ import { Platform } from '../platform/platform';
import { Connection } from '../remote/connection';
import { JsonProtoSerializer } from '../remote/serializer';
import { ConnectivityMonitor } from './../remote/connectivity_monitor';

import { NoopConnectivityMonitor } from '../remote/connectivity_monitor_noop';
import { BrowserConnectivityMonitor } from './browser_connectivity_monitor';
import { WebChannelConnection } from './webchannel_connection';
import { debugAssert } from '../util/assert';

// Implements the Platform API for browsers and some browser-like environments
// (including ReactNative).
Expand Down Expand Up @@ -72,4 +74,23 @@ export class BrowserPlatform implements Platform {
btoa(raw: string): string {
return btoa(raw);
}

randomBytes(nBytes: number): Uint8Array {
debugAssert(nBytes >= 0, `Expecting non-negative nBytes, got: ${nBytes}`);

// Polyfills for IE and WebWorker by using `self` and `msCrypto` when `crypto` is not available.
const crypto =
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian May 4, 2020

Choose a reason for hiding this comment

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

We might also consider adding support for this shim on ReactNative: https://stackoverflow.com/questions/45301900/howto-patch-shim-crypto-getrandomvalues-for-react-native

While the fallback will make sure these environments don't break, we can possibly extend our support for crypto.

Edit: I missed that you added self && self.crypto. I blame it on the inline Linter warning which you can suppress by moving your no-any line two down :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we don't need it, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with not adding support for ReactNative until the ReactNative Firebase wrapper bundles "crypto", but note that without getting "crypto" from globals, we will always fall back to Math.random() for RN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I am filing a ticket to get crypto added for react native firestore.

Choose a reason for hiding this comment

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

react-native-firebase participant here, just cross-linking invertase/react-native-firebase#3618

Copy link
Member

Choose a reason for hiding this comment

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

Just thought I'd quickly summerize; React Native Firebase doesn't use the JS SDK, we're a thin JS wrapper around the native Android & iOS SDKs so this change has no direct affect on us.

However our JS wrapper does use an identical random id generator - so I think we may need to port this change across in some form - we'll pick this up separately, thanks!

// eslint-disable-next-line @typescript-eslint/no-explicit-any
typeof self !== 'undefined' && (self.crypto || (self as any)['msCrypto']);
const bytes = new Uint8Array(nBytes);
if (crypto) {
crypto.getRandomValues(bytes);
} else {
// Falls back to Math.random
for (let i = 0; i < nBytes; i++) {
bytes[i] = Math.floor(Math.random() * 256);
}
}
return bytes;
}
}
8 changes: 8 additions & 0 deletions packages/firestore/src/platform_node/node_platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

import { randomBytes } from 'crypto';
import { inspect } from 'util';

import { DatabaseId, DatabaseInfo } from '../core/database_info';
Expand All @@ -27,6 +28,7 @@ import { NoopConnectivityMonitor } from './../remote/connectivity_monitor_noop';

import { GrpcConnection } from './grpc_connection';
import { loadProtos } from './load_protos';
import { debugAssert } from '../util/assert';

export class NodePlatform implements Platform {
readonly base64Available = true;
Expand Down Expand Up @@ -75,4 +77,10 @@ export class NodePlatform implements Platform {
btoa(raw: string): string {
return new Buffer(raw, 'binary').toString('base64');
}

randomBytes(nBytes: number): Uint8Array {
debugAssert(nBytes >= 0, `Expecting non-negative nBytes, got: ${nBytes}`);

return randomBytes(nBytes);
}
}
23 changes: 20 additions & 3 deletions packages/firestore/src/util/misc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import { debugAssert } from './assert';
import { PlatformSupport } from '../platform/platform';

export type EventHandler<E> = (value: E) => void;
export interface Indexable {
Expand All @@ -27,11 +28,27 @@ export class AutoId {
// Alphanumeric characters
const chars =
'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
// The largest byte value that is a multiple of `char.length`.
const maxMultiple = Math.floor(256 / chars.length) * chars.length;
debugAssert(
0 < maxMultiple && maxMultiple < 256,
`Expect maxMultiple to be (0, 256), but got ${maxMultiple}`
);

let autoId = '';
for (let i = 0; i < 20; i++) {
autoId += chars.charAt(Math.floor(Math.random() * chars.length));
const targetLength = 20;
while (autoId.length < targetLength) {
const bytes = PlatformSupport.getPlatform().randomBytes(40);
for (let i = 0; i < bytes.length; ++i) {
// Only accept values that are [0, maxMultiple), this ensures they can
// be evenly mapped to indices of `chars` via a modulo operation.
if (autoId.length < targetLength && bytes[i] < maxMultiple) {
autoId += chars.charAt(bytes[i] % chars.length);
}
}
}
debugAssert(autoId.length === 20, 'Invalid auto ID: ' + autoId);
debugAssert(autoId.length === targetLength, 'Invalid auto ID: ' + autoId);

return autoId;
}
}
Expand Down
4 changes: 4 additions & 0 deletions packages/firestore/test/util/test_platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,10 @@ export class TestPlatform implements Platform {
btoa(raw: string): string {
return this.basePlatform.btoa(raw);
}

randomBytes(nBytes: number): Uint8Array {
return this.basePlatform.randomBytes(nBytes);
}
}

/** Returns true if we are running under Node. */
Expand Down