Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
243433006:
[webcrypto] Set the error type for failures. (Closed)
|
Created:
6 years, 8 months ago by eroman Modified:
6 years, 7 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[webcrypto] Set the error type for failures.
This corresponds with the WebCrypto spec update:
https://dvcs.w3.org/hg/webcrypto-api/rev/64993be45f9f
And is a dependency for the Blink change:
https://codereview.chromium.org/243853004/
BUG=245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266620
Patch Set 1 : #
Total comments: 23
Patch Set 2 : Address comments #
Total comments: 8
Patch Set 3 : address sleevi comments #Patch Set 4 : Rebase #Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : Address comments #Patch Set 7 : Rebase and try to fix android build... #
Messages
Total messages: 25 (0 generated)
https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:797: Status status = Status::OperationError(); What part of the spec led you to believe this was the right error? I can't find any support for it. Then again, this value is never returned - so seems worth documenting that it's a dummy value? Or just use Status::Error? https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... File content/child/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:857: return Status::OperationError(); DataError - Note that this handling is not defined in a generic sense, but in each of the representative sections 18.4.5 (RSAES-PKCS1-v1.5 importKey) 18.5.6 (RSASSA-PKCS1-v1_5 importKey) 18.6.4 (RSA-PSS importKey) 18.7.4 (RSA-OAEP importKey) 18.8.7 (ECDSA importKey) 18.9.4 (ECDH importKey) Note that ECDH is bugged - filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=25419 https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:862: return Status::OperationError(); DataError https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:866: return Status::OperationError(); DataError https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:884: SECKEY_EncodeDERSubjectPublicKeyInfo(key->key())); FWIW, this doesn't match what the spec says - or at least, this function is not guaranteed to. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:886: return Status::OperationError(); Not a valid return from this function. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:961: return Status::OperationError(); None of these are spec-compliant return values. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:994: return Status::OperationError(); DataError https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:1001: return Status::OperationError(); DataError https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:1396: return Status::OperationError(); Not spec compliant return values
https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:797: Status status = Status::OperationError(); On 2014/04/22 23:45:50, Ryan Sleevi wrote: > What part of the spec led you to believe this was the right error? I can't find > any support for it. Then again, this value is never returned - so seems worth > documenting that it's a dummy value? Or just use Status::Error? I renamed Status::Error() to Status::OperationError(), since there isn't an equivalent generic error for the exceptions. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... File content/child/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:857: return Status::OperationError(); On 2014/04/22 23:45:50, Ryan Sleevi wrote: > DataError - Note that this handling is not defined in a generic sense, but in > each of the representative sections > > 18.4.5 (RSAES-PKCS1-v1.5 importKey) > 18.5.6 (RSASSA-PKCS1-v1_5 importKey) > 18.6.4 (RSA-PSS importKey) > 18.7.4 (RSA-OAEP importKey) > 18.8.7 (ECDSA importKey) > 18.9.4 (ECDH importKey) > > Note that ECDH is bugged - filed > https://www.w3.org/Bugs/Public/show_bug.cgi?id=25419 Done. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:884: SECKEY_EncodeDERSubjectPublicKeyInfo(key->key())); On 2014/04/22 23:45:50, Ryan Sleevi wrote: > FWIW, this doesn't match what the spec says - or at least, this function is not > guaranteed to. Can you explain? As in the OID is not guaranteed to be the same as what spec says? https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:886: return Status::OperationError(); On 2014/04/22 23:45:50, Ryan Sleevi wrote: > Not a valid return from this function. Can you explain? https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:961: return Status::OperationError(); On 2014/04/22 23:45:50, Ryan Sleevi wrote: > None of these are spec-compliant return values. Not sure what you mean by this. Is your concern that the spec doesn't allow these operations to fail, and yet failures are possible here? Or that the error type is not correct. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:994: return Status::OperationError(); On 2014/04/22 23:45:50, Ryan Sleevi wrote: > DataError Done. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:1001: return Status::OperationError(); On 2014/04/22 23:45:50, Ryan Sleevi wrote: > DataError Done.
https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... File content/child/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:884: SECKEY_EncodeDERSubjectPublicKeyInfo(key->key())); On 2014/04/23 00:11:30, eroman wrote: > On 2014/04/22 23:45:50, Ryan Sleevi wrote: > > FWIW, this doesn't match what the spec says - or at least, this function is > not > > guaranteed to. > > Can you explain? As in the OID is not guaranteed to be the same as what spec > says? Correct. You have no guarantee that the OID - or that the associated parameters - are being encoded consistently. I would actually be surprised if it's "doing the right thing" for RSA keys of all types, since the export format calls for a particular OID that is uncommon in the X.509 case. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:886: return Status::OperationError(); On 2014/04/23 00:11:30, eroman wrote: > On 2014/04/22 23:45:50, Ryan Sleevi wrote: > > Not a valid return from this function. > > Can you explain? This operation is not allowed to fail. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:961: return Status::OperationError(); On 2014/04/23 00:11:30, eroman wrote: > On 2014/04/22 23:45:50, Ryan Sleevi wrote: > > None of these are spec-compliant return values. > > Not sure what you mean by this. Is your concern that the spec doesn't allow > these operations to fail, and yet failures are possible here? Or that the error > type is not correct. That they're not allowed to fail. I agree, it's a spec/implementation skew, but it's just worth calling out here. Either we make the implementation unfailable (eww), or we document how it can fail. Suggestions welcome from implementors ;)
https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... File content/child/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:884: SECKEY_EncodeDERSubjectPublicKeyInfo(key->key())); On 2014/04/23 00:17:56, Ryan Sleevi wrote: > On 2014/04/23 00:11:30, eroman wrote: > > On 2014/04/22 23:45:50, Ryan Sleevi wrote: > > > FWIW, this doesn't match what the spec says - or at least, this function is > > not > > > guaranteed to. > > > > Can you explain? As in the OID is not guaranteed to be the same as what spec > > says? > > Correct. You have no guarantee that the OID - or that the associated parameters > - are being encoded consistently. > > I would actually be surprised if it's "doing the right thing" for RSA keys of > all types, since the export format calls for a particular OID that is uncommon > in the X.509 case. I will document it for now. How do you suggest proceeding here? Do I need to set up a template and do the encoding manually? https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:886: return Status::OperationError(); On 2014/04/23 00:17:56, Ryan Sleevi wrote: > On 2014/04/23 00:11:30, eroman wrote: > > On 2014/04/22 23:45:50, Ryan Sleevi wrote: > > > Not a valid return from this function. > > > > Can you explain? > > This operation is not allowed to fail. I will add a note. https://codereview.chromium.org/243433006/diff/20001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:961: return Status::OperationError(); On 2014/04/23 00:17:56, Ryan Sleevi wrote: > On 2014/04/23 00:11:30, eroman wrote: > > On 2014/04/22 23:45:50, Ryan Sleevi wrote: > > > None of these are spec-compliant return values. > > > > Not sure what you mean by this. Is your concern that the spec doesn't allow > > these operations to fail, and yet failures are possible here? Or that the > error > > type is not correct. > > That they're not allowed to fail. > > I agree, it's a spec/implementation skew, but it's just worth calling out here. > Either we make the implementation unfailable (eww), or we document how it can > fail. Suggestions welcome from implementors ;) I think the spec should allow for such failures. Most of the other operations have an escape hatch that reads something like: """If the key generation step fails, then return an error named OperationError.""" Which leads me to believe that OperationError is the catch all for anything else that goes wrong while executing an operation. I think the wording should be updated so it more clearly applies to both exportKey() and digest(). I will file a bug.
Uploaded patchset with fixes
https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... File content/child/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:933: // technically none of the subsequent errors are spec compliant. Filed http://crbug.com/366427 to track this - I dislike tracking issues like this as mere notes :) Update the comments to be // http://crbug.com/366427 - The spec does not define ... https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:62: } This isn't really idiomatic GTest to do this. The GTest friendly way - bool StatusAreEqual(const Status& a, const Status& b) { if (a.IsSuccess() != b.IsSuccess()) return false; if (a.IsSuccess()) return true; return a.error_type() == b.error_type() && a.error_details == b.error_details(); } void PrintTo(const Status& status, ::std::ostream* os) { if (status.IsSuccess()) *os << "Success"; else *os << "Error type: " << status.error_type() << " Error details: " << status.error_details(); } And then EXPECT_PRED_FORMAT2(StatusAreEqual, Status::DataError(), status); If you decide not to use the GTest/GMock friendly comparators, then you've at least got your format string wrong. Presumably you meant to write "error_type=%d, error_details=%s" - otherwise, the error_details= seems superfluous https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:694: TEST_F(SharedCryptoTest, Status) { It's not clear what you're trying to test by this test - what the success or failure conditions are, or why it's necessary. Please add a comment to this test to indicate what you're testing, and then comments to each of these sections where you're doing status = "" to indicate what you're testing. The way you have it structured now, it seems you're just testing data identical to what you have hardcoded, and that makes no sense. https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:711: // the same by the tests. This doesn't make any sense. Is "error message" the output of StatusToString? If so, you're testing exactly the opposite of what you say. If not, it's ambiguous.
https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... File content/child/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:933: // technically none of the subsequent errors are spec compliant. On 2014/04/24 01:17:01, Ryan Sleevi wrote: > Filed http://crbug.com/366427 to track this - I dislike tracking issues like > this as mere notes :) > > Update the comments to be > > // http://crbug.com/366427 - The spec does not define ... Done. https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:62: } On 2014/04/24 01:17:01, Ryan Sleevi wrote: > This isn't really idiomatic GTest to do this. > > The GTest friendly way - > > bool StatusAreEqual(const Status& a, const Status& b) { > if (a.IsSuccess() != b.IsSuccess()) > return false; > if (a.IsSuccess()) > return true; > return a.error_type() == b.error_type() && a.error_details == > b.error_details(); > } > > void PrintTo(const Status& status, ::std::ostream* os) { > if (status.IsSuccess()) > *os << "Success"; > else > *os << "Error type: " << status.error_type() << " Error details: " << > status.error_details(); > } > > > And then > EXPECT_PRED_FORMAT2(StatusAreEqual, Status::DataError(), status); > > > If you decide not to use the GTest/GMock friendly comparators, then you've at > least got your format string wrong. Presumably you meant to write > "error_type=%d, error_details=%s" - otherwise, the error_details= seems > superfluous I addressed this by changing the code to using EXPECT_EQ(), ASSERT_EQ() directly on Status objects. And defining an operator==, operator!=, and PrintTo() for it in the test file. https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:694: TEST_F(SharedCryptoTest, Status) { On 2014/04/24 01:17:01, Ryan Sleevi wrote: > It's not clear what you're trying to test by this test - what the success or > failure conditions are, or why it's necessary. > > Please add a comment to this test to indicate what you're testing, and then > comments to each of these sections where you're doing status = "" to indicate > what you're testing. > > The way you have it structured now, it seems you're just testing data identical > to what you have hardcoded, and that makes no sense. The relevant distinction is to make sure that errors with no details, but a different error type, do not compare as equal. In other words, Status::OperationError() != Status::DataError() != Status::Success(). The intent of these tests is to verify that the comparison of errors works. If it doesn't, then all of the assertions on the error earlier in this test file mean very little. Added a comment. Also now that it is testing the equality operator things are clearer. https://codereview.chromium.org/243433006/diff/40001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:711: // the same by the tests. On 2014/04/24 01:17:01, Ryan Sleevi wrote: > This doesn't make any sense. Is "error message" the output of StatusToString? If > so, you're testing exactly the opposite of what you say. If not, it's ambiguous. error message, versus error type.
LGTM mod nit. https://codereview.chromium.org/243433006/diff/120001/content/child/webcrypto... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/243433006/diff/120001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:303: }; I for one welcome my new clang-formatting overlords? https://codereview.chromium.org/243433006/diff/120001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:704: // Tests a couple error objects against their expected hard coded values, as nit: grammar // Tests several error objects (a couple -> a couple of, but an idiomatic expression easier replaced by 'several') https://codereview.chromium.org/243433006/diff/120001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:707: // error type). s/error message/error details/, since that's what you call it on the object.
https://codereview.chromium.org/243433006/diff/120001/content/child/webcrypto... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/243433006/diff/120001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:303: }; On 2014/04/25 23:08:52, Ryan Sleevi wrote: > I for one welcome my new clang-formatting overlords? This was the result of having run clang-format on the file. I have removed this edit as it is unrelated to this changelist. https://codereview.chromium.org/243433006/diff/120001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:704: // Tests a couple error objects against their expected hard coded values, as On 2014/04/25 23:08:52, Ryan Sleevi wrote: > nit: grammar > > // Tests several error objects > > (a couple -> a couple of, but an idiomatic expression easier replaced by > 'several') Done. https://codereview.chromium.org/243433006/diff/120001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:707: // error type). On 2014/04/25 23:08:52, Ryan Sleevi wrote: > s/error message/error details/, since that's what you call it on the object. Done.
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/243433006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/243433006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/243433006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/243433006/160001
Message was sent while issue was closed.
Change committed as 266620 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
