Based on this revision: D2950
Adding proto file as discussed in the meeting and also as an introduction to implementing a new version of backup service.
This proto should be compatible with the API described in the linked revision.
Details
Details
grpc files generated successfully.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- arcpatch-D3076
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 51–52 ↗ | (On Diff #9642) | Yes. This proto code is correct. I think you miss one important thing here. Just because this object(CreateNewBackupResponse) comes after the keyword return does not mean that it is returned once. If you do something like this: rpc CreateNewBackup(stream CreateNewBackupRequest) returns (stream CreateNewBackupResponse) {}you can do a bidi stream in which you can do something like this: [client] => [CreateNewBackupRequest] => [server] [server] => [CreateNewBackupResponse] => [client] [client] => [CreateNewBackupRequest] => [server] [server] => [CreateNewBackupResponse] => [client] // N times... [client] => [CreateNewBackupRequest] => [server] [server] => [CreateNewBackupResponse] => [client] [end connection] |
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 17 ↗ | (On Diff #9642) | yeah requestBytes and responseBytes is fine, I forgot that we wanted these gRPC messages to be generic. also these names match what the opaque-ke library uses so no need to reveal the cryptographic internals IMO. |
Comment Actions
Turned request bytes and response bytes into oneof definitions.
pakeRegistrationRequest: blinded password info (step 1 of PAKE registration)
pakeRegistrationResponse: server's public key and output of a cryptographic function (step 2 of PAKE registration)
pakeRegistrationUpload: envelope containing sealed cryptographic identifiers and the client’s public key (step 3 of PAKE registration)
pakeRegistrationSuccess: lets client know if registration was successful or not
pakeCredentialRequest: blinded password info (step 1 of PAKE login)
pakeCredentialResponse: sealed envelope only client can open (step 2 of PAKE login)
pakeCredentialFinalization: client answer to the server upon reception of stored envelope (step 3 of PAKE login)
pakeServerMAC: server sends MAC of a static string to client using the shared secret key
pakeClientMAC: client sends MAC of a static string to server using the shared secret key
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 23 ↗ | (On Diff #9770) | This is just a wallet address? |
| 54–55 ↗ | (On Diff #9770) | What's the point of keeping these two separate? They are always served together, and if you combined them you might be able to reduce the number of individual messages that need to be sent. I suspect that nonce can be sent up at the same time as something else |
Comment Actions
I still don't understand why we cannot proceed here, implement the service leaving the places where the auth is supposed to be as TODOs and follow up with this in following diffs. What is the reason for this?
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 54–55 ↗ | (On Diff #9770) |
I don't think so:
Ok, we can merge AuthenticationInformationData into FullAuthenticationRequestData and FullAuthenticationResponseData, but:
On the other hand it feels like raw message/nonce is a part of the auth process, so you can change it if you want. |
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 23 ↗ | (On Diff #9770) | So I assume we'll need yet a couple more cycles since here there are going to be changes as well with the explicit types for the wallet, is this correct(IIRC wallet auth has more steps than just one for the full auth, right?)? |
Comment Actions
After reading up on Sign-in with Ethereum (SIWE), I now understand that the only communication between client and server for wallet auth is:
- Client sends rawMessage to server, containing wallet address, timestamp, etc.
- Server validates the message per the EIP spec, and sends back a bool indicating the success/failure of said validation (if we were generating the message on the server we could omit this step, but we want to generate it on the client)
The rest of the auth process is with the user's wallet. Once the user is authenticated the server can send the desired data back to the client via gRPC.
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 23 ↗ | (On Diff #9770) | We can actually remove walletAuthenticationRequest entirely. The client only needs to send the rawMessage for full auth with wallet. |
| 34 ↗ | (On Diff #9770) | this should be bool walletRawMessageValid = 5; |
| 54–55 ↗ | (On Diff #9770) | I'm leaning towards keeping this as is, but can update if you feel strongly about it @ashoat |
Comment Actions
remove walletAuthenticationRequest, change walletAuthenticationResponse to walletRawMessageValid
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 53 ↗ | (On Diff #9780) | So we can go ahead and name narrow this name to be pake specific as wallet does not use this field at all, right? |
Comment Actions
I change these types a bit, please let me know if it's ok!
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 18–23 | not sure if nonce is supposed to be sent at the beginning or at the end? | |
Comment Actions
Assuming we are still trying to get the auth right here, there are some changes we still need. Hopefully @varun can help with some of this stuff once he's feeling recovered. I think the pattern of commandeering the diff is a good one, and should make this a bit easier for @karol-bisztyga
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 18–23 | There are 6 request fields here but only 4 response fields. That doesn't add up... given that this is an interactive protocol, the the difference should be no more than 1. I don't understand Protobufs well, but what we want to do is to represent each step of the interactive protocol. Eg. "first clients send A and B, then server returns C, then client sends D, then server returns E and F, etc."
| |
| 25 | Why are we sending this up every time we send up any one of the six fields above? We should only need to send up the userID once | |
| 29–30 | You're not even including the wallet address? | |
| 41 | We obviously need more here | |
| 97–99 | These three get sent back together, so they should be combined | |
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 18–23 | These types have been added by @varun, Varun could you answer this, please? | |
| 25 | I think so. | |
| 29–30 | It is basen on the Varun's comment:
@varun can you clarify? | |
| 41 | as above | |
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 97–99 | Do they? I thought we are supposed to send either nonce or rawMessage depending on whether we use pake or wallet. So it could look more like this: oneof {
{
nonce
backupID
}
{
rawMessage
backupID
}
}But this is a more complicated structure. | |
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 29–30 |
| |
Comment Actions
addressing feedback. fixed wallet auth structures, removed redundant messages, moved entropy values to separate message
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 18–23 | nonce should probably be sent after password auth is complete, and since it's only part of CreateNewBackup and not part of RecoverBackupKey (which also uses the FullAuthenticationRequestData message), it should be moved out to CreateNewBackupRequest. | |
| 25 | fixed in latest revision | |
| 29–30 | after discussing this with ashoat, we're moving rawMessage to CreateNewBackupRequest too, since it's specific to that RPC | |
| 41 | discussed with ashoat, don't think we need anything else here | |
Comment Actions
This looks pretty much ready to go to me. @karol-bisztyga should definitely take a look before we land, and would be great to get @geekbrother and maybe @jimpo's eyes on it as well.
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 18 ↗ | (On Diff #9952) | For consistency with the rest of the codebase (and the identifier name here), please capitalize ID at the end of PakeRegistrationRequestAndUserId |
| 89 ↗ | (On Diff #9952) | What's this entropyValid message communicate, and in what cases would the entry be invalid? |
| 110–111 ↗ | (On Diff #9952) | Instead of listing these two individually here, can/should we just have them combined by NewBackupKeyEntropy? |
| native/cpp/CommonCpp/grpc/protos/backup.proto | ||
|---|---|---|
| 89 ↗ | (On Diff #9952) | I was thinking there might be some constraints that the server should enforce, e.g. rawMessage can’t be a duplicate |