Page MenuHomePhabricator

[services][identity] implement verify_user_token rpc
ClosedPublic

Authored by varun on Jun 2 2022, 3:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 24, 1:53 PM
Unknown Object (File)
Sun, Oct 13, 7:54 PM
Unknown Object (File)
Sun, Oct 13, 7:54 PM
Unknown Object (File)
Sun, Oct 13, 7:54 PM
Unknown Object (File)
Sun, Oct 13, 7:54 PM
Unknown Object (File)
Sun, Oct 13, 7:54 PM
Unknown Object (File)
Sun, Oct 13, 7:54 PM
Unknown Object (File)
Sun, Oct 13, 7:54 PM

Details

Summary

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.

Test Plan

Called RPC using bloom and tested all paths

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun requested review of this revision.Jun 2 2022, 4:02 PM
tomek requested changes to this revision.Jun 3 2022, 3:27 AM
tomek added inline comments.
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.

This revision now requires changes to proceed.Jun 3 2022, 3:27 AM

address feedback

services/identity/proto/identity.proto
45 ↗(On Diff #13314)

yes, in Rust, bytes are represented by Vec<u8> and string is represented by String.

services/identity/src/service.rs
71–89 ↗(On Diff #13314)

yeah this is a great use case for a match guard

varun edited the summary of this revision. (Show Details)

Adding @ashoat since I'm making a small change to the .proto file

karol added inline comments.
services/identity/src/database.rs
291 ↗(On Diff #13456)
jim requested changes to this revision.Jun 13 2022, 9:35 AM
jim added inline comments.
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.

This revision now requires changes to proceed.Jun 13 2022, 9:35 AM
varun marked 3 inline comments as done.

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

"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."

https://grpc.github.io/grpc/core/md_doc_statuscodes.html

services/identity/src/service.rs
76

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.

Address remaining feedback, add some logging

Update Rust version in Docker container to >1.59 to support inline assembly

Thanks for adding me, proto change seems good

This revision is now accepted and ready to land.Jun 14 2022, 2:43 PM