Add another grpc service for authenticated requests.
This differs from the existing service by having an interceptor
which will fail if authentication did not succeed.
Details
Details
- Reviewers
varun bartek - Commits
- rCOMMe9f1b63891fa: [Identity] Add authenticated gRPC service
N/A. Implementation in later diff.
This is meant to be more of a design review.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jonringer/keyserver-identity (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
so basically just passing the token, user id, and device id as metadata instead of message fields?
Comment Actions
rather than creating a new .proto file, can we just add the interceptor to the existing .proto?
| services/identity/src/grpc_services/authenticated.rs | ||
|---|---|---|
| 4 ↗ | (On Diff #28554) | i'm not sure i understand this comment. why do we need the client module here at all? |
| 50 ↗ | (On Diff #28554) | also not sure i understand this comment. do you mean it must be run synchronously? |
| 73 ↗ | (On Diff #28554) | seems weird to return Empty from the client module instead of something from the auth_proto module |
| services/identity/src/grpc_services/authenticated.rs | ||
|---|---|---|
| 4 ↗ | (On Diff #28554) | Because when importing from other proto files, tonic will reflect the structure of imported structs based on their protobuf package paths. In this case we need Empty and PreKey from the identity.client protobuf package, and tonic will refer to this as client::Empty and client::PreKey in rust. Would like like to reword the comment? // This must be named client, because generated code from the authenticated protobuf file // references message structs from the client protobuf file with the client:: namespace I decided to just change the comment, as the current comment doesn't seem to be satisfactory. |
| 50 ↗ | (On Diff #28554) | auth_intercept can't be async (otherwise the types don't match). So new_db_client.verify_access_token which is async can't be awaited normally. Instead I have to do this work around which involves tokio pinning the action on the current thread. Handle::block_on is the only way I'm aware to get around an explicit .await. |
| 50 ↗ | (On Diff #28554) | I reworded the comment to be more clear. |
| 73 ↗ | (On Diff #28554) | I don't think so, there' will be others around key management that should be shared as well in the future. Eventually I would like to see:
Also reduce the size of the individual proto files. identity_client is already ~360 lines long. |
Comment Actions
i don't love that we block on verify_access_token in the interceptor function. would rather use a tower layer instead of a tonic interceptor to call the code asynchronously, or just call an authenticate helper function at the start of each service method
Comment Actions
i don't love that we block on verify_access_token in the interceptor function. would rather use a tower layer instead of a tonic interceptor to call the code asynchronously,
We need to verify the access token, I don't see how we can get around blocking the request until we made the determination for passing the request through.
Also, looking at the tower example code, I'm not sure if it's a strict improvement anymore, looks like there still a few oddities to get the middleware to play nicely: https://github.com/hyperium/tonic/blob/master/examples/src/tower/server.rs
or just call an authenticate helper function at the start of each service method
From the perspective of the calling client, this would still be the same amount of time. Real difference is whether you want the handler to be aware of it.
I think the issue of awaiting for DDB can be mitigated by using some type of LRU cache. I'm assuming in the future that authenticating devices are likely going to burst with requests (e.g. fan-out of X3DH messages).