Receivers will need the outbound user's username/wallet address. They should get it from the identity service, rather than rely on the outbound user to provide the correct value.
Details
Details
tested this on staging by calling the get_inbound client method from my local keyserver and successfully retrieving a username for an existing user.
tried again with a wallet user and got back a wallet address.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
ESLint failures show that this diff was created in a branch with some commits that haven't been put up on Phabricator. Can you explain what's going on?
Seeing as this diff doesn't have a listed stack on Phabricator, you should be able to apply it cleanly to master and then try arc diff again to get rid of the failures
Comment Actions
Before landing, can you:
- Create a follow-up task to add the same info to KeyserverKeysResponse, and set it to block ENG-6079?
- Create a follow-up task to add the same info to OutboundKeysForUserResponse. I had trouble finding the right task to block here, but I guess ENG-5453. (cc @kamil, @inka)
- Respond to my questions in the third inline comment, and consider amending this diff to include the social_proof if possible
Would also appreciate @bartek's review here.
| shared/protos/identity_authenticated.proto | ||
|---|---|---|
| 90 ↗ | (On Diff #35329) | I think we need to update this too. Consider a client trying to connect to a keyserver. Currently, that client would learn of a keyserver's userID by calling that keyserver's getVersion endpoint. Some discussion of that here. getVersion also returns a username, which is displayed in the keyserver management UI. But while we can trust the userID (since we get the keys from identity service based on it), we can't trust the keyserver's claimed username. Instead, we'll want the client to get the keyserver's username from the identity service. cc @tomek, as this affects your work on the keyserver connection handlers. PS – if the keyserver host is an Ethereum user, we'd also ideally want the client to verify the social proof before displaying their wallet address (or ENS name). See discussion below for more details on that. |
| 97 ↗ | (On Diff #35329) | I think we'll want to update this too. @kamil is slated to work on initiating Olm sessions between different users next month. That workflow should be based on userIDs as well, since a user may learn of another user if they are in the same community. (In another case, a user may learn of another user by searching for their username; in that case, we would have them use @will's upcoming user search API, which should return a userID as well.) |
| 118–121 ↗ | (On Diff #35329) | In general, when we hand out a wallet_address, we should include the accompanying social_proof. This enables the receiver to verify the root of trust. This is documented somewhat in the whitepaper, but not well enough. I created ENG-6370 to track updating the whitepaper's description of peer-to-peer authentication. Created ENG-6371 to track implementing the social proof stuff. Two questions for you here:
|
Comment Actions
If it's in the same branch, that should be reflected on the diff stack. Can you add a relationship between this diff and its parent in your branch?
Comment Actions
Rust looks good, I left some improvement ideas (more to code quality than actual logic so they're optional)
Remember to address @ashoat's feedback before landing ;)
| services/identity/src/database.rs | ||
|---|---|---|
| 1110–1124 ↗ | (On Diff #35329) | Optionally this could be done as impl TryFrom<AttributeMap> for Identifier but up to you |
| 1122 ↗ | (On Diff #35329) | Might be worth adding |
| services/identity/src/grpc_services/authenticated.rs | ||
| 200–209 ↗ | (On Diff #35329) | I'd prefer implementing impl From<DBIdentifier> for proto::Identifier and use it here. Makes the code cleaner and re-usable |
| shared/protos/identity_authenticated.proto | ||
| 118–121 ↗ | (On Diff #35329) |
There's a check in Identity Service code. If we went with option 2 in ENG-6371, we'd get this enforced by schema. |
| keyserver/addons/rust-node-addon/rust-binding-types.js | ||
|---|---|---|
| 14–15 ↗ | (On Diff #35329) | yeah just noticed while rebasing, thanks for the heads up |
| shared/protos/identity_authenticated.proto | ||
|---|---|---|
| 118–121 ↗ | (On Diff #35329) |
Pretty easy, I think. But do we need to handle verifying the root of trust here, too?
It's required by the wallet login RPCs. It does get stored in DDB |
Comment Actions
@ashoat and @will created the follow up tasks here: https://linear.app/comm/issue/ENG-6408/provide-username-when-returning-keys-from-identity-service
| shared/protos/identity_authenticated.proto | ||
|---|---|---|
| 118–121 ↗ | (On Diff #35329) | Yeah but it still wouldn’t hurt to include the socialProof here now if it’s not too hard |
Comment Actions
Trying to understand if the social proof work was completed here
| keyserver/addons/rust-node-addon/src/identity_client/get_inbound_keys_for_user.rs | ||
|---|---|---|
| 47 | Can you clarify what "We ignore the social proof for now" means here? | |
| keyserver/addons/rust-node-addon/src/identity_client/get_inbound_keys_for_user.rs | ||
|---|---|---|
| 39–51 | ||
| 47 | I just meant that we can avoid creating an unused variable with this shorthand. The EthereumIdentity struct does contain a social_proof field, though. In the future, we'll replace this line with the suggested code below | |
| keyserver/addons/rust-node-addon/src/identity_client/get_inbound_keys_for_user.rs | ||
|---|---|---|
| 39–51 | last arm in the suggested edit's match statement should actually be ... _ => (None, None, None), | |