Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
145083006:
[webcrypto] Add error messages for failed operations. (Closed)
|
Created:
6 years, 11 months ago by eroman Modified:
6 years, 10 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, padolph Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[webcrypto] Add error messages for failed operations.
BUG=245025
BUG=331665
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248062
Patch Set 1 : INCOMPLETE (needs OpenSSL file, and error-specific tests) #Patch Set 2 : Check the Status in unittests #
Total comments: 24
Patch Set 3 : address rsleevi comments #Patch Set 4 : Add webcrypto:: prefix to Status #Patch Set 5 : Revert webcrypto:: prefixing #Patch Set 6 : Add openssl file #Patch Set 7 : Add comments about the status, rename some errors, and change a few #Patch Set 8 : Add unittests for verification failure with empty signature #Patch Set 9 : Remove CompletedWithError() and use a different pattern #Patch Set 10 : Fix compiler warning on windows #
Total comments: 27
Patch Set 11 : Address sleevi comments #Patch Set 12 : Try again to upload.. #
Total comments: 1
Patch Set 13 : Change parens #Patch Set 14 : Rebase #
Messages
Total messages: 19 (0 generated)
This isn't quite ready for review yet, but sending it out early to get high level feedback.
This is ready for review now, PTAL.
I've not reviewed all of the error cases and conditions. My biggest concern with this CL is the fiddling with the braces on the if statements - it does so inconsistently, and if it resolved inconsistencies, great - except, it doesn't. So I don't understand the motivation for that unrelated style change. A few more high-level notes below. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:23: using webcrypto::Status; I thought this was discouraged in Chromium side. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:29: bool completeWithError(const Status& status, blink::WebCryptoResult* result) { naming: I find it counter-intuitive that completeWithError takes a negative result (an error), and will return true if it was an error, because the phrase "complete with error" suggests "perform this task", not "is this an error" if (AbortOnErrorStatus(status, &result)) return; style: completeWithError -> CompleteWithError https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:115: using webcrypto::Status; ditto re: using https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:299: } I'm not sure why you added braces here, especially considering lines like 291/292 or 335/336. I agree there's inconsistency, but if the goal is to resolve that, this addition doesn't. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:309: if (tag_length_bits > 128 || tag_length_bits % 8 != 0) { nit: add parens for clarity (tag_length_bits % 8) != 0 https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:847: return Status::ErrorUnexpected(); Note: This strikes me as a dangerous pattern (even if pre-existing), because it leaves me unclear what the status of |*buffer| is (in this case, it refers to a buffer I'm supposed to free, even though things failed). Is it actually freed? https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:20: // |Status| indicates whether an operation completed successfully, or with an nit: No | | - this is not a variable, it's a class nit: "an operation" is a bit vague, even though it's in content::webcrypto. // Status holds the result of a WebCrypto operation. For failing operations, it carries additional details explaining why the operation failed. design: Why have you designed this akin to std::exception(), when the majority of Chromium code uses enums? This seems more of a Blink-ism than a Chromium-ism.
https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:23: using webcrypto::Status; On 2014/01/24 01:21:50, Ryan Sleevi wrote: > I thought this was discouraged in Chromium side. The practice is quite common in Chrome code, occurring nearly 13,000 times in Chromium code: $ cd src $ git grep '^using ' | wc -l 12824 I am unaware of any decrees forbidding it. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:29: bool completeWithError(const Status& status, blink::WebCryptoResult* result) { On 2014/01/24 01:21:50, Ryan Sleevi wrote: > naming: I find it counter-intuitive that completeWithError takes a negative > result (an error), and will return true if it was an error, because the phrase > "complete with error" suggests "perform this task", not "is this an error" > > if (AbortOnErrorStatus(status, &result)) > return; > > style: completeWithError -> CompleteWithError How about CompletedWithError() ? https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:299: } On 2014/01/24 01:21:50, Ryan Sleevi wrote: > I'm not sure why you added braces here, especially considering lines like > 291/292 or 335/336. > > I agree there's inconsistency, but if the goal is to resolve that, this addition > doesn't. Removed the braces; modifying existing style was not my intention. I verified that the other lines being edited don't change the brace style. I can do a style sweep and switch everything to one style or another as a seprate CL if you want, but both styles are allowed by the styleguide. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:309: if (tag_length_bits > 128 || tag_length_bits % 8 != 0) { On 2014/01/24 01:21:50, Ryan Sleevi wrote: > nit: add parens for clarity (tag_length_bits % 8) != 0 Done. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:847: return Status::ErrorUnexpected(); On 2014/01/24 01:21:50, Ryan Sleevi wrote: > Note: This strikes me as a dangerous pattern (even if pre-existing), because it > leaves me unclear what the status of |*buffer| is (in this case, it refers to a > buffer I'm supposed to free, even though things failed). Is it actually freed? WebArrayBuffer manages the underlying blink buffers, so there are no leaks here. Once the caller's WebArrayBuffer goes out of scope, it will release the final refcount on the WTF::ArrayBuffer. As far as the bigger question on contract, the current contract is that the caller provides a WebArrayBuffer whose contents are replaced with the result of the operation. In the case of failures, the contents may still have been replaced, however an error status is returned indicating that it is not relevant. We could guarantee that the provided WebArrayBuffer is only mutated on success, but that seems like more work for no gain. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:20: // |Status| indicates whether an operation completed successfully, or with an On 2014/01/24 01:21:50, Ryan Sleevi wrote: > nit: No | | - this is not a variable, it's a class > nit: "an operation" is a bit vague, even though it's in content::webcrypto. > > // Status holds the result of a WebCrypto operation. For failing operations, it > carries additional details explaining why the operation failed. > > design: Why have you designed this akin to std::exception(), when the majority > of Chromium code uses enums? This seems more of a Blink-ism than a Chromium-ism. Shrug. There are examples of both models in Chromium. URLRequestStatus is the first one that comes to mind. I chose a class rather than an enum because I prefer the encapsulation of having an object: * Utility functions like "IsError/IsSuccess/ToString" are scoped to the class * Room to add extra parameters for finer granularity error messages. * Provides a central point to histogram errors, attach a debugger When I was playing around debugging unittests I shoved the NSS error code into the Status object, as well as the stacktrace of where the error was created from. Was useful when debugging My current preference is for a class here, but I am not strongly committed to that approach.
Still needing to go through to consider the security implications, although I'm fully on board with this CL as it's stated. Just wanted to provide some early feedback on any (major) code changes. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:23: using webcrypto::Status; On 2014/01/24 03:19:21, eroman wrote: > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > I thought this was discouraged in Chromium side. > > The practice is quite common in Chrome code, occurring nearly 13,000 times in > Chromium code: > > $ cd src > $ git grep '^using ' | wc -l > 12824 > > I am unaware of any decrees forbidding it. Thread: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/mn5OV... Summary: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/SGDZG... https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:29: bool completeWithError(const Status& status, blink::WebCryptoResult* result) { On 2014/01/24 03:19:21, eroman wrote: > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > naming: I find it counter-intuitive that completeWithError takes a negative > > result (an error), and will return true if it was an error, because the phrase > > "complete with error" suggests "perform this task", not "is this an error" > > > > if (AbortOnErrorStatus(status, &result)) > > return; > > > > style: completeWithError -> CompleteWithError > > How about CompletedWithError() ? I still find that a bit confusing. ShouldReturnWithError() ? https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:299: } On 2014/01/24 03:19:21, eroman wrote: > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > I'm not sure why you added braces here, especially considering lines like > > 291/292 or 335/336. > > > > I agree there's inconsistency, but if the goal is to resolve that, this > addition > > doesn't. > > Removed the braces; modifying existing style was not my intention. I verified > that the other lines being edited don't change the brace style. > > I can do a style sweep and switch everything to one style or another as a > seprate CL if you want, but both styles are allowed by the styleguide. I'm aware both styles are permitted, and don't really care other than that it's consistent within it's area (for example, all of net/ discourages it, with the exception of the shared QUIC bits -_-) Per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml 's background, all I care about is consistency - within a file and within a 'module'. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:847: return Status::ErrorUnexpected(); On 2014/01/24 03:19:21, eroman wrote: > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > Note: This strikes me as a dangerous pattern (even if pre-existing), because > it > > leaves me unclear what the status of |*buffer| is (in this case, it refers to > a > > buffer I'm supposed to free, even though things failed). Is it actually freed? > > WebArrayBuffer manages the underlying blink buffers, so there are no leaks here. > Once the caller's WebArrayBuffer goes out of scope, it will release the final > refcount on the WTF::ArrayBuffer. > > As far as the bigger question on contract, the current contract is that the > caller provides a WebArrayBuffer whose contents are replaced with the result of > the operation. In the case of failures, the contents may still have been > replaced, however an error status is returned indicating that it is not > relevant. We could guarantee that the provided WebArrayBuffer is only mutated on > success, but that seems like more work for no gain. Reasonable. My concern here with this pattern is trying to find a good API contract that can ensure that even if an operation fails, the buffer contents won't leak back into the Blink side, since that would be Real Bad (mmkay). From my quick can, it looked like you were always stack-allocating a blink::WebArrayBuffer in the !Internal calls (eg: WebCrypto::Digest), so this should be fine, I just want to request you double check that's the case. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:20: // |Status| indicates whether an operation completed successfully, or with an On 2014/01/24 03:19:21, eroman wrote: > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > nit: No | | - this is not a variable, it's a class > > nit: "an operation" is a bit vague, even though it's in content::webcrypto. > > > > // Status holds the result of a WebCrypto operation. For failing operations, > it > > carries additional details explaining why the operation failed. > > > > design: Why have you designed this akin to std::exception(), when the majority > > of Chromium code uses enums? This seems more of a Blink-ism than a > Chromium-ism. > > Shrug. There are examples of both models in Chromium. URLRequestStatus is the > first one that comes to mind. > > I chose a class rather than an enum because I prefer the encapsulation of having > an object: > > * Utility functions like "IsError/IsSuccess/ToString" are scoped to the class > * Room to add extra parameters for finer granularity error messages. > * Provides a central point to histogram errors, attach a debugger > > When I was playing around debugging unittests I shoved the NSS error code into > the Status object, as well as the stacktrace of where the error was created > from. Was useful when debugging > > My current preference is for a class here, but I am not strongly committed to > that approach. I didn't have strong objections, I just wanted to understand the design more. In particular, I wasn't sure if you were expecting the callers extend this via adding new static creators (much in the same way you'd add new enum values) or perhaps derive from this (as done on the JS exceptions, AIUI, and similar to std::exception()). The "just a number" approach is 'nice' in that you can reserve ranges for implementation-specific details (eg: 1000 - 2000 = some component foo), whereas with the design as written, you'd either need to change to returning a heap-allocated object (so you can avoid type slicing) or you'd have to extend this base object. Again, to be clear, not an objection to the design, just trying to get better clarification. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:62: static Status ErrorKeyLength(); At the risk of being a real PITA, can I ask you to document some of these? For example, if importing a JWK, and the modulus is empty, is it an ErrorJwkDecodeN or an ErrorEmptyModulus?
> Still needing to go through to consider the security > implications, Let me give you a braindump of my security thinking for you to review as well: - The errors exposed should never expose any private information (i.e. key bytes, plain text) - The current set of errors are all validity checks on data passed in by the consumer (i.e. wrong sized IV, unsupported operation), so they do not expose anything the consumer didn't already have access to. ***CONCERN (please read)**** -Key unwrapping. We don't have any key unwrapping committed yet, and hence are not giving any errors from that. However once we do, I believe we will want to swallow any errors returned by the key import. Consider the case of JWK key unwrapping. That would be implemented as a two part operation: first "decrypt" to get the JWK bytes, and then do an "import" on those bytes. If we were to return the errors that occurred during the 'import', we would be telling the consumer something about the wrapped key which they did not already know. In other words, we would be telling them something about the plaintext from that "decrypt" operation. The JWK errors should not be revealing anything about the key bytes itself, but still seems dodgy. Even worse, there are errors during key import for "empty key data". It might be possible then to straight up learn whether the wrapped key was empty bytes. (Not sure if that error is a good idea in fact, I think it was a protection against feeding openssl empty bytes in some case where it was crashing). The solution in both these cases would be to swallow any import errors during unwrap. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:23: using webcrypto::Status; On 2014/01/24 20:27:01, Ryan Sleevi wrote: > On 2014/01/24 03:19:21, eroman wrote: > > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > > I thought this was discouraged in Chromium side. > > > > The practice is quite common in Chrome code, occurring nearly 13,000 times in > > Chromium code: > > > > $ cd src > > $ git grep '^using ' | wc -l > > 12824 > > > > I am unaware of any decrees forbidding it. > > Thread: > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/mn5OV... > > Summary: > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/SGDZG... Thanks for the link to the thread! I'll give it a more detailed look after lunch, but it doesn't seem there was any real consensus on that thread, and the style guide was never amended. At any rate, I have uploaded a patchset with the prefix for comparison to see if you prefer it. My take is the extra verbosity of webcrypto:: inside of a WebCryptoImpl class is pretty weird. The summary on that thread leaves the decision up to the author's judgement. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:299: } On 2014/01/24 20:27:01, Ryan Sleevi wrote: > On 2014/01/24 03:19:21, eroman wrote: > > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > > I'm not sure why you added braces here, especially considering lines like > > > 291/292 or 335/336. > > > > > > I agree there's inconsistency, but if the goal is to resolve that, this > > addition > > > doesn't. > > > > Removed the braces; modifying existing style was not my intention. I verified > > that the other lines being edited don't change the brace style. > > > > I can do a style sweep and switch everything to one style or another as a > > seprate CL if you want, but both styles are allowed by the styleguide. > > I'm aware both styles are permitted, and don't really care other than that it's > consistent within it's area (for example, all of net/ discourages it, with the > exception of the shared QUIC bits -_-) > > Per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml 's > background, all I care about is consistency - within a file and within a > 'module'. Sure, I'll do a separate pass to resolve this inconsistency. https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:847: return Status::ErrorUnexpected(); On 2014/01/24 20:27:01, Ryan Sleevi wrote: > On 2014/01/24 03:19:21, eroman wrote: > > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > > Note: This strikes me as a dangerous pattern (even if pre-existing), because > > it > > > leaves me unclear what the status of |*buffer| is (in this case, it refers > to > > a > > > buffer I'm supposed to free, even though things failed). Is it actually > freed? > > > > WebArrayBuffer manages the underlying blink buffers, so there are no leaks > here. > > Once the caller's WebArrayBuffer goes out of scope, it will release the final > > refcount on the WTF::ArrayBuffer. > > > > As far as the bigger question on contract, the current contract is that the > > caller provides a WebArrayBuffer whose contents are replaced with the result > of > > the operation. In the case of failures, the contents may still have been > > replaced, however an error status is returned indicating that it is not > > relevant. We could guarantee that the provided WebArrayBuffer is only mutated > on > > success, but that seems like more work for no gain. > > Reasonable. > > My concern here with this pattern is trying to find a good API contract that can > ensure that even if an operation fails, the buffer contents won't leak back into > the Blink side, since that would be Real Bad (mmkay). > > From my quick can, it looked like you were always stack-allocating a > blink::WebArrayBuffer in the !Internal calls (eg: WebCrypto::Digest), so this > should be fine, I just want to request you double check that's the case. Correct. It is only possible to stack allocate blink::WebArrayBuffer so I am not concerned about leaks. I have reviewed the code and I don't see any issues.
https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:62: static Status ErrorKeyLength(); On 2014/01/24 20:27:01, Ryan Sleevi wrote: > At the risk of being a real PITA, can I ask you to document some of these? > > For example, if importing a JWK, and the modulus is empty, is it an > ErrorJwkDecodeN or an ErrorEmptyModulus? Done. I have added comments for each of the errors, this is probably the easiest place for you to start the error review. In the process I noticed some inconsistencies so did some renaming, an also: * Got rid of ErrorEmptyData() --> instead encrypt/decrypt will fail with a generic Error. (Seemed arbitrary which cases did this and which didn't). * Got rid of ErrorEmptySignatureData() --> I don't believe it was consistent for this to be an error in the first place, since other truncated inputs would result in a success + verification failure. Changed to that behavior, and added some corresponding tests.
https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/145083006/diff/80001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:29: bool completeWithError(const Status& status, blink::WebCryptoResult* result) { On 2014/01/24 20:27:01, Ryan Sleevi wrote: > On 2014/01/24 03:19:21, eroman wrote: > > On 2014/01/24 01:21:50, Ryan Sleevi wrote: > > > naming: I find it counter-intuitive that completeWithError takes a negative > > > result (an error), and will return true if it was an error, because the > phrase > > > "complete with error" suggests "perform this task", not "is this an error" > > > > > > if (AbortOnErrorStatus(status, &result)) > > > return; > > > > > > style: completeWithError -> CompleteWithError > > > > How about CompletedWithError() ? > > I still find that a bit confusing. ShouldReturnWithError() ? Seeing how this was confusing, I removed the function all together, and replaced it with something a bit more verbose but easier to understand.
https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:187: CompleteWithError(status, &result); Does it make sense for this pattern to follow the Chromium "return early on error"? if (status.IsError()) { CompleteWithError(...) return; } result.completeWithBuffer(buffer) https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.h (right): https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.h:11: #include "content/renderer/webcrypto/webcrypto_util.h" You can forward-declare webcrypto::Status here because it's a return value. See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts ("OrReturnValue()" in particular) https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:211: return Status::ErrorIncorrectSizedAesCbcIv(); nit: IncorrectlySized or IncorrectSize , but IncorrectSized reads weird. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:291: return Status::ErrorUnsupported(); Remind me that we should clarify the error handling here. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:520: return Status::ErrorUnexpectedKeyType(); s/Unexpected/Invalid? https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:725: return Status::ErrorUnexpectedKeyType(); Little weird to have this be Unexpected vs Unsupported https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:781: return Status::ErrorUnexpectedKeyType(); Ditto https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:55: // means there was a parsing error, or the JSON was not of a dictionary. nit: "JSON was not of a dictionary" - "JSON object was not convertable to a dictionary" ? https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:59: static Status ErrorJwkMissingKty(); This seems to be an overloaded error, especially given how much JSON can co-erce things to strings when treating in JS. Is the plan to treat all non-coercable data types (eg: a list, a dict) as missing? https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:63: static Status ErrorJwkExtractableInconsistent(); nit: The JWK "extractable" attribute was present, but was incompatible with the value requested by the Web Crypto caller. Since it's not about "set", it's about the particular value (false), and being incompatible with the API. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:66: // WebCryptoAlgorithm. Either it was malformed, or unrecognized. unnecessary , When in a list of two, it's not necessary to include the comma. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:70: // specified by the Web Crypto import operation. I'd prefer "inconsistent" or "incompatible" over "contradict", if only that "contradict" suggests asserting the opposite or that one value is more true than the other, when really, we don't know which is "right" https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:89: // The "n" parameter was either missing, or could not be parsed as a base-64 drop the first "or" (missing, could not be parsed ..., or the decoed) Same with others. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:127: static Status ErrorDataTooBig(); s/Big/Large, to match both your description and the error message.
Rietveld keeps 500-ing so you may not be able to see my new patchset yet, but here are my responses. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:187: CompleteWithError(status, &result); On 2014/01/28 21:11:58, Ryan Sleevi wrote: > Does it make sense for this pattern to follow the Chromium "return early on > error"? > > if (status.IsError()) { > CompleteWithError(...) > return; > } > > result.completeWithBuffer(buffer) I personally like early return code and would be in favor of that. However I prefer not mixing style adjustments with large refactors to keep them more focused. Unless you say otherwise, I will defer this to a subsequent CL. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.h (right): https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.h:11: #include "content/renderer/webcrypto/webcrypto_util.h" On 2014/01/28 21:11:58, Ryan Sleevi wrote: > You can forward-declare webcrypto::Status here because it's a return value. > > See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts > ("OrReturnValue()" in particular) Neat, didn't realize you could do that for return-by-value too. Done. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:211: return Status::ErrorIncorrectSizedAesCbcIv(); On 2014/01/28 21:11:58, Ryan Sleevi wrote: > nit: IncorrectlySized or IncorrectSize , but IncorrectSized reads weird. Done. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:291: return Status::ErrorUnsupported(); On 2014/01/28 21:11:58, Ryan Sleevi wrote: > Remind me that we should clarify the error handling here. Do you mean in the spec or in the implementation? I have raised the inconsistency with throwing UnsupportedError for unregistered algorithm names and yet possibly having runtime-unsupported algorithms in: https://www.w3.org/Bugs/Public/show_bug.cgi?id=24427 Let me know if there is another aspect you would like me to improve. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:520: return Status::ErrorUnexpectedKeyType(); On 2014/01/28 21:11:58, Ryan Sleevi wrote: > s/Unexpected/Invalid? I left as is based on my own preference, but can change at your request. My thinking was "invalid" sounds more like something was wrong with the key itself, whereas "unexpected" conveys a sense that the key was OK but just being used in the wrong way. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl_nss.cc:725: return Status::ErrorUnexpectedKeyType(); On 2014/01/28 21:11:58, Ryan Sleevi wrote: > Little weird to have this be Unexpected vs Unsupported Not sure if I parsed the comment correctly. Do you mean this should be: (a) ErrorUnsupported (b) ErrorUnsupportedKeyType (new error) I decided against (a) because I wanted the error message to explain that the problem was with the key type. I can introduce a new error name however I think the error text will be much the same as ErrorUnexpectedType. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:55: // means there was a parsing error, or the JSON was not of a dictionary. On 2014/01/28 21:11:58, Ryan Sleevi wrote: > nit: "JSON was not of a dictionary" - "JSON object was not convertable to a > dictionary" ? Done. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:59: static Status ErrorJwkMissingKty(); On 2014/01/28 21:11:58, Ryan Sleevi wrote: > This seems to be an overloaded error, especially given how much JSON can co-erce > things to strings when treating in JS. > > Is the plan to treat all non-coercable data types (eg: a list, a dict) as > missing? I am unaware of any type coercion mandated by JWK parsing, or generic coercion implied by JSON. The current implementation requires strong matching of types, and does not try to stringify types (for instance if you pass an integer where a string was expected, that is a failure, not a stringification to decimal form). There are some aspects to the particular implementation which concern me, and I will follow up on (have added a TODO). For instance, optional fields are considered as "existing" only if the propery is present AND the correct type. So if "extractable: 0" was set, that would be disregarded, which seems dangerous. Instead I believe that should error, since the optional field was present but of the wrong type. I will follow up on this issue separately, and perhaps introduce some new errors to capture these nuances. In the current CL I would prefer to match the current errors. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:63: static Status ErrorJwkExtractableInconsistent(); On 2014/01/28 21:11:58, Ryan Sleevi wrote: > nit: > The JWK "extractable" attribute was present, but was incompatible with the value > requested by the Web Crypto caller. > > Since it's not about "set", it's about the particular value (false), and being > incompatible with the API. Done. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:66: // WebCryptoAlgorithm. Either it was malformed, or unrecognized. On 2014/01/28 21:11:58, Ryan Sleevi wrote: > unnecessary , > > When in a list of two, it's not necessary to include the comma. Done. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:70: // specified by the Web Crypto import operation. On 2014/01/28 21:11:58, Ryan Sleevi wrote: > I'd prefer "inconsistent" or "incompatible" over "contradict", if only that > "contradict" suggests asserting the opposite or that one value is more true than > the other, when really, we don't know which is "right" Done. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:89: // The "n" parameter was either missing, or could not be parsed as a base-64 On 2014/01/28 21:11:58, Ryan Sleevi wrote: > drop the first "or" (missing, could not be parsed ..., or the decoed) > > Same with others. Done. https://codereview.chromium.org/145083006/diff/550001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:127: static Status ErrorDataTooBig(); On 2014/01/28 21:11:58, Ryan Sleevi wrote: > s/Big/Large, to match both your description and the error message. Done.
Can you re-upload? Seems to have failed upload?
Done
lgtm https://codereview.chromium.org/145083006/diff/700001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/145083006/diff/700001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:523: extractable = extractable && jwk_extractable_value; FWIW (and in a cleanup), you don't need this line - in all cases, |extractable| wins (as its default value). You tested the one possible case above on 521/522.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/145083006/740001
Retried try job too often on mac for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/145083006/740001
Message was sent while issue was closed.
Change committed as 248062
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
