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

[CryptoService] Leverage browser native Crypto API to hash strings. #3850

Merged
merged 6 commits into from
Jul 1, 2016
Merged
Show file tree
Hide file tree
Changes from 5 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
56 changes: 51 additions & 5 deletions extensions/amp-analytics/0.1/crypto-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,77 @@

import * as lib from '../../../third_party/closure-library/sha384-generated';
import {getService} from '../../../src/service';
import {dev} from '../../../src/log';

/** @const {string} */
const TAG = 'Crypto';

export class Crypto {

constructor(win) {
this.win = win;
/** @private @const {?SubtleCrypto} */
this.subtle_ = getSubtle(win);
}

/**
* Returns the SHA-384 hash of the input string in an ArrayBuffer.
* Returns the SHA-384 hash of the input string in a number array.
* Input string cannot contain chars out of range [0,255].
* @param {string} str
* @returns {!Promise<!ArrayBuffer>}
* @returns {!Promise<!Array<number>>}
* @throws {!Error} when input string contains chars out of range [0,255]
*/
sha384(str) {
if (this.subtle_) {
try {
return this.subtle_.digest('SHA-384', str2ab(str))
// [].slice.call(Unit8Array) is a shim for Array.from(Unit8Array)
.then(buffer => [].slice.call(new Uint8Array(buffer)),
e => {
dev.info(TAG, 'Crypto digest promise has rejected, ' +
'fallback to closure lib.', e);
return lib.sha384(str);
});
} catch (e) {
dev.info(TAG, 'Crypto digest has thrown, fallback to closure lib.', e);
}
}
return Promise.resolve(lib.sha384(str));
}

/**
* Returns the SHA-384 hash of the input string in the format of web safe
* base64 string (using -_. instead of +/=).
* base64 (using -_. instead of +/=).
* Input string cannot contain chars out of range [0,255].
* @param {string} str
* @returns {!Promise<string>}
* @throws {!Error} when input string contains chars out of range [0,255]
Copy link
Member

Choose a reason for hiding this comment

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

It rejects, right? I don't think @throws is a thing in JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An abandoned baby? https://en.wikipedia.org/wiki/JSDoc
;-)

Copy link
Member

Choose a reason for hiding this comment

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

There is a long complicated relationship with the other things called JSDoc

*/
sha384Base64(str) {
return Promise.resolve(lib.sha384Base64(str));
return this.sha384(str).then(buffer => {
return lib.base64(buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to see this hand written. We had something similar in the A4A PR, I believe.

const websafe = {
  '+': '-',
  '/': '_',
  '=': '.'
};
function base64(bytes) {
  const array = Array(bytes.length);
  for (let i = 0; i < bytes.length; i++) {
    array[i] = String.fromCharCode(bytes[i]);
  }
  return btoa(array.join('')).replace(/(\+|\/|=)/g, (char) => websafe[char]));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually maybe yes, when we split off the closure lib for lazy load.

But now, I don't see much benefit of hand written here. The closure lib's implementation looks far more efficient then using regex.

Copy link
Contributor

@jridgewell jridgewell Jun 30, 2016

Choose a reason for hiding this comment

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

It is a little more efficient (we're coming off a hashing function here, so that won't matter), and much larger. As I said, we're already moving to this in the A4A library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said, the plan is

  • Remove the dependency on closure lib for base64 function (might hand write here)
  • Exclude base64 code from closure lib, and lazy load the lib only on old browsers

Both things have to happen to gain us benefit. Having an extra implementation here only decrease efficiency and increase code size.

It's out of the scope of this PR though, tracked here: #3690

On the other hand, we should be able to share the code in someway. Copying the same code here doesn't sound right.

});
}
}

function getSubtle(win) {
if (!win.crypto) {
return null;
}
return win.crypto.subtle || win.crypto.webkitSubtle || null;
}

// A shim for TextEncoder
function str2ab(str) {
Copy link
Member

Choose a reason for hiding this comment

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

Add JSDoc.

const buf = new Uint8Array(str.length);
for (let i = 0; i < str.length; i++) {
// Apply the same check as in closure lib:
// https://github.com/google/closure-library/blob/master/closure/goog/crypt/sha2_64bit.js#L169
if (str.charCodeAt(i) > 255) {
throw Error('Characters must be in range [0,255]');
}
buf[i] = str.charCodeAt(i);
}
return buf;
}

export function installCryptoService(win) {
Expand Down
91 changes: 73 additions & 18 deletions extensions/amp-analytics/0.1/test/test-crypto-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,86 @@
*/

import {Crypto} from '../crypto-impl';
import {Platform} from '../../../../src/platform';
import * as lib from '../../../../third_party/closure-library/sha384-generated';
import * as sinon from 'sinon';

describe('crypto-impl', () => {

let crypto;
function testSuite(descption, crypto) {
describe(descption, () => {
it('should hash "abc" in sha384', () => {
return crypto.sha384('abc').then(buffer => {
expect(buffer.length).to.equal(48);
expect(buffer[0]).to.equal(203);
expect(buffer[1]).to.equal(0);
expect(buffer[47]).to.equal(167);
});
});

beforeEach(() => {
crypto = new Crypto({});
});
it('should hash "abc" in sha384Base64', () => {
return expect(crypto.sha384Base64('abc')).to.eventually.equal(
'ywB1P0WjXou1oD1pmsZQBycsMqsO3tFjGotgWkP_W-2AhgcroefMI1i67KE0yCWn');
});

it('should hash "foobar" in sha384Base64', () => {
return expect(crypto.sha384Base64('foobar')).to.eventually.equal(
'PJww2fZl501RXIQpYNSkUcg6ASX9Pec5LXs3IxrxDHLqWK7fzfiaV2W_kCr5Ps8G');
});

it('should hash "abc" in sha384', () => {
return crypto.sha384('abc').then(buffer => {
expect(buffer.length).to.equal(48);
expect(buffer[0]).to.equal(203);
expect(buffer[1]).to.equal(0);
expect(buffer[47]).to.equal(167);
it('should throw when input contains chars out of range [0,255]', () => {
expect(() => crypto.sha384('abc今')).to.throw();
expect(() => crypto.sha384Base64('abc今')).to.throw();
});
});
});
}

it('should hash "abc" in sha384Base64', () => {
return expect(crypto.sha384Base64('abc')).to.eventually.equal(
'ywB1P0WjXou1oD1pmsZQBycsMqsO3tFjGotgWkP_W-2AhgcroefMI1i67KE0yCWn');
});
function isModernChrome() {
const platform = new Platform(window);
return platform.isChrome() && platform.getMajorVersion() >= 45;
}

it('should hash "foobar" in sha384Base64', () => {
return expect(crypto.sha384Base64('foobar')).to.eventually.equal(
'PJww2fZl501RXIQpYNSkUcg6ASX9Pec5LXs3IxrxDHLqWK7fzfiaV2W_kCr5Ps8G');
testSuite('with native crypto API', new Crypto(window));
testSuite('with crypto lib', new Crypto({}));
testSuite('with native crypto API rejects', new Crypto({
crypto: {
subtle: {
digest: () => Promise.reject('Operation not supported'),
},
},
}));
testSuite('with native crypto API throws', new Crypto({
crypto: {
subtle: {
digest: () => {
throw new Error();
},
},
},
}));

it('native API result should exactly equal to crypto lib result', () => {
return Promise
Copy link
Member

Choose a reason for hiding this comment

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

Can you make sure that native API is actually called? E.g. by counting calls into fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test case runs only under Chrome48+.

.all([new Crypto(window).sha384('abc'), new Crypto({}).sha384('abc')])
.then(results => {
expect(results[0]).to.jsonEqual(results[1]);
});
});

// Run this test only on browsers that we're confident about the existence
// of native Crypto API.
if (isModernChrome()) {
it('should not call closure lib when native API is available', () => {
const sandbox = sinon.sandbox.create();
const nativeApiSpy = sandbox.spy(window.crypto.subtle, 'digest');
const libSpy = sandbox.spy(lib, 'sha384');
return new Crypto(window).sha384Base64('abc').then(hash => {
expect(hash).to.equal(
'ywB1P0WjXou1oD1pmsZQBycsMqsO3tFjGotgWkP_W-2AhgcroefMI1i67KE0yCWn');
expect(nativeApiSpy).to.have.been.calledOnce;
expect(libSpy).to.not.have.been.called;
});
sandbox.restore();
});
}
});
3 changes: 2 additions & 1 deletion third_party/closure-library/compile.sh
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ java -jar $1 \
--js "$2/goog/debug/error.js" \
--js "$2/goog/dom/nodetype.js" \
--js "$2/goog/math/long.js" \
--js "$2/goog/reflect/reflect.js" \
--js "$2/goog/string/string.js" \
--js_output_file "sha384-generated.js" \
--output_wrapper "/* Generated from closure library commit $GIT_COMMIT */%output%;export function sha384Base64(input) { return ampSha384(input) }; export function sha384(input) { return ampSha384Digest(input) };" \
--output_wrapper "/* Generated from closure library commit $GIT_COMMIT */%output%;export function base64(input) { return ampBase64(input) }; export function sha384(input) { return ampSha384Digest(input) };" \
--manage_closure_dependencies \
--process_closure_primitives \
--use_types_for_optimization \
Expand Down
Loading