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 1 commit
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
43 changes: 38 additions & 5 deletions extensions/amp-analytics/0.1/crypto-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,62 @@ import {getService} from '../../../src/service';
export class Crypto {

constructor(win) {
this.win = win;
if (win.crypto) {
this.subtle = win.crypto.subtle || win.crypto.webkitSubtle;
Copy link
Member

Choose a reason for hiding this comment

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

Make private and @const and always set it (Possibly to null). Add type.

Copy link
Contributor

Choose a reason for hiding this comment

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

add typedef?

Copy link
Member

Choose a reason for hiding this comment

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

Closure externs should have webCrypto.SubtleCrypto as a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I check that, are externs listed anywhere?
Didn't see here

Copy link
Member

Choose a reason for hiding this comment

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

I used Google's internal code search :)

}
}

/**
* 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))
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these can take a UInt8Array, not just a ArrayBuffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

// [].slice.call(Unit8Array) is a shim for Array.from(Unit8Array)
.then(buffer => [].slice.call(new Uint8Array(buffer)),
() => lib.sha384(str));
Copy link
Member

Choose a reason for hiding this comment

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

dev.error?
Add comment about fallback.

} catch (e) {
// ignore, fallback to closure lib.
Copy link
Member

Choose a reason for hiding this comment

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

dev.error? When would this fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potentially any browser doesn't follow the standard API can throw.
For example,

  • Chrome 37 throws because it has a different method signature.
  • IE11 doesn't return Promise (though already excluded as it's in window.msCrypto)

}
}
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.

});
}
}

// 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 ArrayBuffer(str.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe creating a new Uint8Array(length) will create an associated buffer at bufView.buffer.

const bufView = new Uint8Array(buf);
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]');
}
bufView[i] = str.charCodeAt(i);
}
return buf;
}

export function installCryptoService(win) {
return getService(win, 'crypto', () => {
return new Crypto(win);
Expand Down
66 changes: 48 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 @@ -18,28 +18,58 @@ import {Crypto} from '../crypto-impl';

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);
});
});

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

beforeEach(() => {
crypto = new Crypto({});
});
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');
});
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('should hash "foobar" in sha384Base64', () => {
return expect(crypto.sha384Base64('foobar')).to.eventually.equal(
'PJww2fZl501RXIQpYNSkUcg6ASX9Pec5LXs3IxrxDHLqWK7fzfiaV2W_kCr5Ps8G');
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.deep.equal(results[1]);
Copy link
Member

Choose a reason for hiding this comment

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

I prefer jsonEqual. I trust it more and it gives much better errors.

});
});
});
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