Query DB for token corresponding to user and device. If DB client returns a token, compare it to the one in the VerifyUserTokenRequest, checking if the strings match.
Details
Details
Called RPC using bloom and tested all paths
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/identity/proto/identity.proto | ||
|---|---|---|
| 45 ↗ | (On Diff #13314) | For C++ both string and bytes are represented by std::string. Are these represented by different types in rust? |
| services/identity/src/service.rs | ||
| 71–89 ↗ | (On Diff #13314) | The amount of duplication here is huge. Could you find a way to make it less duplicated? I'm only learning rust, but I think using match with a guard might be a lot simpler (see the suggestion). Another option might be to determine token_valid value separately and then create a return value - Response::new(VerifyUserTokenResponse would be used in only one place. |
| services/identity/src/database.rs | ||
|---|---|---|
| 291 ↗ | (On Diff #13456) | |
| services/identity/src/service.rs | ||
|---|---|---|
| 62 ↗ | (On Diff #13456) | Constant time compare the tokens |
| 67 ↗ | (On Diff #13456) | nit: I would factor some of this out rust
let token_valid = match self.client.get_token(...).await {
...
};
Ok(Response::new(VerifyUserTokenResponse { token_valid })) |
| 67 ↗ | (On Diff #13456) | You should actually inspect and handle this error. For example if there's a DB misconfiguration and you can't connect, the app shouldn't tell the user that their token is invalid. |
Comment Actions
Addressing feedback. Need some clarification on one piece of feedback -- replied inline
| services/identity/src/service.rs | ||
|---|---|---|
| 62 ↗ | (On Diff #13456) | can you elaborate please? |
| services/identity/src/service.rs | ||
|---|---|---|
| 81 ↗ | (On Diff #13485) | "Service implementors can use the following guidelines to decide between FAILED_PRECONDITION, ABORTED, and UNAVAILABLE: (a) Use UNAVAILABLE if the client can retry just the failing call. (b) Use ABORTED if the client should retry at a higher level (e.g., when a client-specified test-and-set fails, indicating the client should restart a read-modify-write sequence). (c) Use FAILED_PRECONDITION if the client should not retry until the system state has been explicitly fixed. E.g., if an "rmdir" fails because the directory is non-empty, FAILED_PRECONDITION should be returned since the client should not retry unless the files are deleted from the directory." |
| services/identity/src/service.rs | ||
|---|---|---|
| 76 ↗ | (On Diff #13485) | Don't expose this out through the API! The error message returned from the API should either be something actionable by the user or a vague/opaque "Internal error" or "Unexpected error" messages without any details. Details can be logged for debugging. |
| 62 ↗ | (On Diff #13456) | This introduces a timing side channel attack where the attacker can extract information about how much of the request token is right by the amount of time the comparison takes. Search for more about timing side channel attacks and constant time string comparison. eg https://javascript.plainenglish.io/what-are-timing-attacks-and-how-to-prevent-them-using-nodejs-158cc7e2d70c The Rust constant_time_eq crate looks good for this. |