Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
195983024:
Add a WebCrypto API for getting digests by chunk. (Closed)
|
Created:
6 years, 9 months ago by jww Modified:
6 years, 9 months ago CC:
blink-reviews, jamesr, dglazkov+blink, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd a WebCrypto API for getting digests by chunk.
Currently the WebCrypto API only has support for one-shot digests. This CL
provides the necessary API scaffolding for methods to generate digests one chunk
at a time.
BUG=349514
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169991
Patch Set 1 #Patch Set 2 : Updated digest-by-chunk API to use a Digestor object #
Total comments: 11
Patch Set 3 : Simplify Digestor APi #Patch Set 4 : Simplified WebCryptoDigestor to more of an interface #
Total comments: 8
Patch Set 5 : Fixed nits #
Total comments: 5
Patch Set 6 : Removed unnecessary include #Messages
Total messages: 31 (0 generated)
As per several of the comments in https://codereview.chromium.org/189373010/, it seems like it would be useful to have digest-by-chunk WebCrypto functions.
Thanks for chipping away at this! I'll bet you didn't expect it would be this
much work at the onset :)
So I think there is an opportunity for a more wholesome API here, along these
lines:
WebCryptoDigestor digestor =
Platform::getCurrent()->crypto()->startSynchronousDigest(WebCryptoAlgorithmIdSha256);
digestor.consume(buffer, length);
...
digestor.consume(buffer, length);
...
digestor.finish();
Vector result(digestor.resultData(), digestor.resultSize());
This allows the digest implementation to allocate its own buffer for the result
and hence avoids having to operate on some other WebArrayBuffer/WebVector.
Another important trait of the API is to make the memory management explicit, by
having the WebCryptoDigestor be stack-allocated.
I think this can be achieved by defining an interface like
WebCryptoDigestAlgorithm {
virtual consume(const uint8*, unsigned);
virtual finish();
};
And having WebCryptoDigestor be a really simple wrapper which adopts the pointer
and forwards methods to it.
What do you think? Thanks for considering the feedback!
On 2014/03/14 23:30:33, eroman wrote:
> Thanks for chipping away at this! I'll bet you didn't expect it would be this
> much work at the onset :)
>
> So I think there is an opportunity for a more wholesome API here, along these
> lines:
>
> WebCryptoDigestor digestor =
>
Platform::getCurrent()->crypto()->startSynchronousDigest(WebCryptoAlgorithmIdSha256);
>
> digestor.consume(buffer, length);
> ...
> digestor.consume(buffer, length);
> ...
>
> digestor.finish();
>
> Vector result(digestor.resultData(), digestor.resultSize());
>
>
> This allows the digest implementation to allocate its own buffer for the
result
> and hence avoids having to operate on some other WebArrayBuffer/WebVector.
>
> Another important trait of the API is to make the memory management explicit,
by
> having the WebCryptoDigestor be stack-allocated.
>
> I think this can be achieved by defining an interface like
> WebCryptoDigestAlgorithm {
> virtual consume(const uint8*, unsigned);
> virtual finish();
> };
>
> And having WebCryptoDigestor be a really simple wrapper which adopts the
pointer
> and forwards methods to it.
>
> What do you think? Thanks for considering the feedback!
Yup, that's a heck of a lot cleaner than my API :-P I'll probably get on this
early next week; it's about closing time here.
On 2014/03/14 23:36:35, jww wrote:
> On 2014/03/14 23:30:33, eroman wrote:
> > Thanks for chipping away at this! I'll bet you didn't expect it would be
this
> > much work at the onset :)
> >
> > So I think there is an opportunity for a more wholesome API here, along
these
> > lines:
> >
> > WebCryptoDigestor digestor =
> >
>
Platform::getCurrent()->crypto()->startSynchronousDigest(WebCryptoAlgorithmIdSha256);
> >
> > digestor.consume(buffer, length);
> > ...
> > digestor.consume(buffer, length);
> > ...
> >
> > digestor.finish();
> >
> > Vector result(digestor.resultData(), digestor.resultSize());
> >
> >
> > This allows the digest implementation to allocate its own buffer for the
> result
> > and hence avoids having to operate on some other WebArrayBuffer/WebVector.
> >
> > Another important trait of the API is to make the memory management
explicit,
> by
> > having the WebCryptoDigestor be stack-allocated.
> >
> > I think this can be achieved by defining an interface like
> > WebCryptoDigestAlgorithm {
> > virtual consume(const uint8*, unsigned);
> > virtual finish();
> > };
> >
> > And having WebCryptoDigestor be a really simple wrapper which adopts the
> pointer
> > and forwards methods to it.
> >
> > What do you think? Thanks for considering the feedback!
>
> Yup, that's a heck of a lot cleaner than my API :-P I'll probably get on this
> early next week; it's about closing time here.
So I've hacked away at this, and implemented the suggested API, only to realize
that I don't think it will actually work as intended :-P Long story short, I've
confirmed with Blink folks that there's really no way to pass a scoped ptr
between Blink land and content, so the hope of putting the WebCryptoDigestor on
the stack will not work (because any copy will fail because the pointer has to
be an Own ptr).
I still prefer the Digestor object, though, so I'm going to go ahead and rework
this into a simpler Digestor object but that content gets a pointer to, unless,
eroman@, you strongly object.
eroman@, this is the new version of the API. As I mentioned, it's still not as clean as we'd like, but I definitely like it better with the WebCryptoDigestor object.
https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypto.h File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:103: WebCryptoDigestor(blink::WebCryptoAlgorithmId algorithmId) { } Remove this constructor https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:106: virtual bool consume(const unsigned char* data, unsigned dataSize) { return false; } I think it is worth describing what the return value is. For instance, is it expected that consume() can be called again after it returns false? https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:107: virtual bool finish(WebArrayBuffer& result) { return false; } I actually think that rather than filling a WebArrayBuffer, it would be advantageous to have this: // The data is valid until the WebCryptoDigestor is destroyed. // consume should not be called after calling finish. virtual bool finish(unsigned char*& resultData, unsigned int& resultDataSize); This way the implementation can stack-allocate the digest result (the digest output size is known once the operation is started), and saves copying around, and the inconvenience of having to return a WebArrayBuffer type. In the modules/crypto wrapper (which will adopt the raw pointer), it can provide helpers to return the result into a WTF::Vector or whatever too. https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:205: virtual bool digestSynchronous(const WebCryptoAlgorithmId algorithmId, const unsigned char* data, unsigned dataSize, WebArrayBuffer& result) { return false; } Is this version already in use, or can it be deleted? https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:206: virtual WebCryptoDigestor* digestorSynchronous(const WebCryptoAlgorithmId algorithmId) { return 0; } I would say drop the const.
https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypto.h File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:103: WebCryptoDigestor(blink::WebCryptoAlgorithmId algorithmId) { } On 2014/03/19 19:06:21, eroman wrote: > Remove this constructor This is actually the constructor that the implementation wants to use; why should we remove it? More specifically, I want to make the default constructor private because any implementation needs to specific a WebCryptoAlgorithmId at creation time to function. Thus we need some other constructor to be specified in the superclass so the subclass can be created. https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:106: virtual bool consume(const unsigned char* data, unsigned dataSize) { return false; } On 2014/03/19 19:06:21, eroman wrote: > I think it is worth describing what the return value is. For instance, is it > expected that consume() can be called again after it returns false? Done. https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:107: virtual bool finish(WebArrayBuffer& result) { return false; } On 2014/03/19 19:06:21, eroman wrote: > I actually think that rather than filling a WebArrayBuffer, it would be > advantageous to have this: > > // The data is valid until the WebCryptoDigestor is destroyed. > // consume should not be called after calling finish. > virtual bool finish(unsigned char*& resultData, unsigned int& resultDataSize); > > > This way the implementation can stack-allocate the digest result (the digest > output size is known once the operation is started), and saves copying around, > and the inconvenience of having to return a WebArrayBuffer type. > > In the modules/crypto wrapper (which will adopt the raw pointer), it can provide > helpers to return the result into a WTF::Vector or whatever too. In my new version, I've implemented it pretty much as you suggest. I think it's a bit ugly on the the platform_crypto_[nss/openssl] side, but let me know. https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:205: virtual bool digestSynchronous(const WebCryptoAlgorithmId algorithmId, const unsigned char* data, unsigned dataSize, WebArrayBuffer& result) { return false; } On 2014/03/19 19:06:21, eroman wrote: > Is this version already in use, or can it be deleted? This is in use. Of course, it's not that hard to rewrite the other call sites to use the Digestor version, so I'll go ahead and do that if you prefer; let me know either way. https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:206: virtual WebCryptoDigestor* digestorSynchronous(const WebCryptoAlgorithmId algorithmId) { return 0; } On 2014/03/19 19:06:21, eroman wrote: > I would say drop the const. Done.
https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypto.h File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... public/platform/WebCrypto.h:103: WebCryptoDigestor(blink::WebCryptoAlgorithmId algorithmId) { } On 2014/03/20 23:02:50, jww wrote: > On 2014/03/19 19:06:21, eroman wrote: > > Remove this constructor > > This is actually the constructor that the implementation wants to use; why > should we remove it? > > More specifically, I want to make the default constructor private because any > implementation needs to specific a WebCryptoAlgorithmId at creation time to > function. Thus we need some other constructor to be specified in the superclass > so the subclass can be created. This is just an interface class, there doesn't need to be any public constructor. Just add a dummy protected constructor. Only the Chromium side will be allocating WebCryptoDigestor, in response to the digestorSynchronous() call. There shouldn't be an algorithId parameter in this base class constructor, since it does nothing with it. In fact the Chromium side might just allocate a different class for the various digest algorithms, and have a fixed size array for the output.
On 2014/03/20 23:14:02, eroman wrote: > https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypto.h > File public/platform/WebCrypto.h (right): > > https://codereview.chromium.org/195983024/diff/20001/public/platform/WebCrypt... > public/platform/WebCrypto.h:103: WebCryptoDigestor(blink::WebCryptoAlgorithmId > algorithmId) { } > On 2014/03/20 23:02:50, jww wrote: > > On 2014/03/19 19:06:21, eroman wrote: > > > Remove this constructor > > > > This is actually the constructor that the implementation wants to use; why > > should we remove it? > > > > More specifically, I want to make the default constructor private because any > > implementation needs to specific a WebCryptoAlgorithmId at creation time to > > function. Thus we need some other constructor to be specified in the > superclass > > so the subclass can be created. > > This is just an interface class, there doesn't need to be any public > constructor. > Just add a dummy protected constructor. > > Only the Chromium side will be allocating WebCryptoDigestor, in response to the > digestorSynchronous() call. > > There shouldn't be an algorithId parameter in this base class constructor, since > it does nothing with it. In fact the Chromium side might just allocate a > different class for the various digest algorithms, and have a fixed size array > for the output. Got it and thanks for the clarification. Let me know how the latest version looks.
LGTM after these comments. Apologies for delay, was out in vacation Friday (still doesn't explain full latency.. but a start :) https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypto.h File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypt... public/platform/WebCrypto.h:106: // partially generated digest. It will return |false when that fails. After missing "|false" --> "|false|" https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypt... public/platform/WebCrypto.h:110: // finish() will return |true| if the digest has been successfully computed nit: add a newline https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypt... public/platform/WebCrypto.h:207: // outlined above. digestorSynchronous must provide the result via the WebCryptoDigestor object synchronously. Please also mention that 0 can be returned on failure. And that the allocated object must be freed by the caller. https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypt... public/platform/WebCrypto.h:211: virtual WebCryptoDigestor* digestorSynchronous(WebCryptoAlgorithmId algorithmId) { return 0; } I propose calling this "createDigestor()". And leave off the synchronous part.
No problem, thanks for the review! abarth, would you mind taking the OWNER look at this? https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypto.h File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypt... public/platform/WebCrypto.h:106: // partially generated digest. It will return |false when that fails. After On 2014/03/25 17:37:02, eroman wrote: > missing "|false" --> "|false|" Done. https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypt... public/platform/WebCrypto.h:110: // finish() will return |true| if the digest has been successfully computed On 2014/03/25 17:37:02, eroman wrote: > nit: add a newline Done. https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypt... public/platform/WebCrypto.h:207: // outlined above. digestorSynchronous must provide the result via the WebCryptoDigestor object synchronously. On 2014/03/25 17:37:02, eroman wrote: > Please also mention that 0 can be returned on failure. And that the allocated > object must be freed by the caller. Done. https://codereview.chromium.org/195983024/diff/60001/public/platform/WebCrypt... public/platform/WebCrypto.h:211: virtual WebCryptoDigestor* digestorSynchronous(WebCryptoAlgorithmId algorithmId) { return 0; } On 2014/03/25 17:37:02, eroman wrote: > I propose calling this "createDigestor()". And leave off the synchronous part. Done.
lgtm https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... public/platform/WebCrypto.h:114: virtual bool finish(unsigned char*& resultData, unsigned& resultDataSize) { return false; } How long is the returned char* valid? Until the WebCryptoDigestor is destroyed?
https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... public/platform/WebCrypto.h:114: virtual bool finish(unsigned char*& resultData, unsigned& resultDataSize) { return false; } On 2014/03/25 21:06:36, abarth wrote: > How long is the returned char* valid? Until the WebCryptoDigestor is destroyed? Yes, until the WebCryptoDigestor is destroyed. I tried to convey that in the last sentence of the comment; do you think that needs further clarification?
https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... public/platform/WebCrypto.h:114: virtual bool finish(unsigned char*& resultData, unsigned& resultDataSize) { return false; } On 2014/03/25 21:12:07, jww wrote: > On 2014/03/25 21:06:36, abarth wrote: > > How long is the returned char* valid? Until the WebCryptoDigestor is > destroyed? > > Yes, until the WebCryptoDigestor is destroyed. I tried to convey that in the > last sentence of the comment; do you think that needs further clarification? So you did!
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/195983024/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/195983024/100001
https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... public/platform/WebCrypto.h:37: #include "WebPrivateOwnPtr.h" This is unused, delete it.
The CQ bit was unchecked by jww@chromium.org
https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195983024/diff/100001/public/platform/WebCryp... public/platform/WebCrypto.h:37: #include "WebPrivateOwnPtr.h" On 2014/03/25 23:28:09, eroman wrote: > This is unused, delete it. Yikes, good catch!
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/195983024/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_rel
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/195983024/250001
Message was sent while issue was closed.
Change committed as 169991 |
