[dbsc] Handle transient cryptographic signing errors [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: antonio...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): antonio...@chromium.org
Reviewer source(s):
antonio...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Alex Ilin
- Alex Rudenko
- Antonio Sartori
- Jan Wilken Dörrie
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
Related details
- Alex Ilin
- Antonio Sartori
- Jan Wilken Dörrie
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori voted and added 1 comment![]( "Open in Gerrit")
Votes added by Antonio Sartori
| Code-Review | +1 |
1 comment
- Alex Ilin
- Jan Wilken Dörrie
Alex Ilin (Gerrit)
Alex Ilin voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alex Ilin
| Code-Review | +1 |
3 comments
base::unexpected(unexportable_keys::ServiceError::kCryptoApiFailed)));Do we still need to update these tests? It looks like we could keep them as is and make them test persistent errors.
if (refresh_result == RefreshResult::kFatalError) {Hmm, is it guaranteed that a session should be deleted only if `error.GetRefreshResult()` returns `kFatalError`?
It'd be nice to add a session_error_unittest.cc verifying this invariant.
`error.GetRefreshResult() != kFatalError` -> `!error.GetDeletionReason().has_value()`
Alternatively, I'd be fine with keeping the existing code structure:
```
SessionError::ErrorType error_type = error.type;
if (std::optional<DeletionReason> deletion_reason =
error.GetDeletionReason();
deletion_reason.has_value()) {
DeleteSessionAndNotify(*deletion_reason, session_key,
on_access_callback);
UnblockDeferredRequests(session_key,
error.GetRefreshResult().value_or(RefreshResult::kFatalError),
std::move(error));
} else {
// can be moved outside of the "else" block
UnblockDeferredRequests(session_key,
error.GetRefreshResult().value_or(RefreshResult::kUnreachable),
std::move(error));
}
```
Related details
- Jan Wilken Dörrie
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Wilken Dörrie (Gerrit)
Jan Wilken Dörrie added 3 comments![]( "Open in Gerrit")
base::unexpected(unexportable_keys::ServiceError::kCryptoApiFailed)));Do we still need to update these tests? It looks like we could keep them as is and make them test persistent errors.
Done
Hmm, is it guaranteed that a session should be deleted only if `error.GetRefreshResult()` returns `kFatalError`?
It'd be nice to add a session_error_unittest.cc verifying this invariant.
`error.GetRefreshResult() != kFatalError` -> `!error.GetDeletionReason().has_value()`
Alternatively, I'd be fine with keeping the existing code structure:
```
SessionError::ErrorType error_type = error.type;
if (std::optional<DeletionReason> deletion_reason =
error.GetDeletionReason();
deletion_reason.has_value()) {
DeleteSessionAndNotify(*deletion_reason, session_key,
on_access_callback);
UnblockDeferredRequests(session_key,
error.GetRefreshResult().value_or(RefreshResult::kFatalError),
std::move(error));
} else {
// can be moved outside of the "else" block
UnblockDeferredRequests(session_key,
error.GetRefreshResult().value_or(RefreshResult::kUnreachable),
std::move(error));
}
```
Good point, I kept the existing structure now.
base::unexpected(unexportable_keys::ServiceError::kKeyNotReady));FYI, I changed this test, because kKeyNotReady is now a transient error and the session does not get deleted.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Wilken Dörrie (Gerrit)
Jan Wilken Dörrie voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |