Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
616233002:
Add proto definitions for messages used by CryptAuth APIs and the authentication protocol. (Closed)
|
Created:
6 years, 2 months ago by Tim Song Modified:
6 years, 2 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd proto definitions for messages used by CryptAuth APIs and the authentication protocol.
BUG=385719
Committed: https://crrev.com/5921080ce77d1f7674070f8cf96473b4d4b0d684
Cr-Commit-Position: refs/heads/master@{#299747}
Patch Set 1 : #
Total comments: 34
Patch Set 2 : fixes #
Total comments: 14
Patch Set 3 : fixes #Patch Set 4 : remove unused apis #Patch Set 5 : reformat comments #
Messages
Total messages: 18 (3 generated)
Patchset #1 (id:1) has been deleted
tengs@chromium.org changed reviewers: + isherman@chromium.org
Thanks, Tim. Do we have a plan for keeping the Chromium protobuf in sync with the server-side version, e.g. via a script of some sort that either pushes updates or at least alerts us when an update is needed? https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:2: // Generated from server definitions. Do not edit. nit: We should probably add a copyright stanza to the top of the file. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:6: option java_package = "com.google.api.services.cryptauth"; nit: Do we want to keep this line in the Chromium repo? https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:55: optional string callback_bluetooth_address = 2; Is it intentional that the field with tag "1" is omitted? There are a few other fields omitted in the rest of this file as well. Is this because they've been deprecated on the server side? https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:58: // <code>"cryptauth#findEligibleUnlockDevicesRequest"</code>. Hmm, why do we have this field at all, if it has a fixed value? https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:105: optional string device_feature = 2; Is there a list of valid features? (Seems like this ought to be an enum field rather than a field string...) https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:111: // A (possibly empty) list of devices supporting the requested featur nit: "featur" -> "feature" https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:119: // This request is sent by the login page to obtain one challenge session for What is the "login page"? The ChromeOS sign-in screen? https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:217: // A Diffie-Hellman public key used perform a key exchange during enrollment. nit: "used perform" -> "used to perform" https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:224: optional string origin = 2; nit: Can this field be annotated as "[deprecated=true]"? https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:229: // Indicates whether a legacy crypto suite must be used with this device. Is there a specific legacy crypto suite that's being referred to? https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:232: // A URL describing which application facets this enrollment can be used (see nit: I don't understand this sentence :/ https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:233: // http://go/appid). nit: Can we point to a publicly-accessible URL? https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:252: // user's device has received a transaction, as well as to provide additional nit: Odd choice of line-break. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:277: // If true, Easyunlock will be enabled for the device with public key equal nit: "Easyunlock" -> "EasyUnlock" https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:286: // all of a nit: Odd choice of line break.
We don't really have a formal way of getting updates when the server side changes, but this is a general problems with all web APIs. The onus is on the server to maintain backwards compatibility, or to tell the clients when something changes. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:2: // Generated from server definitions. Do not edit. On 2014/10/06 21:15:59, Ilya Sherman wrote: > nit: We should probably add a copyright stanza to the top of the file. Done. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:6: option java_package = "com.google.api.services.cryptauth"; On 2014/10/06 21:15:58, Ilya Sherman wrote: > nit: Do we want to keep this line in the Chromium repo? Done. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:55: optional string callback_bluetooth_address = 2; On 2014/10/06 21:15:59, Ilya Sherman wrote: > Is it intentional that the field with tag "1" is omitted? There are a few other > fields omitted in the rest of this file as well. Is this because they've been > deprecated on the server side? Yes, it's intentional. There are fields used only on the server along with deprecated fields. The client doesn't need to know about them. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:58: // <code>"cryptauth#findEligibleUnlockDevicesRequest"</code>. On 2014/10/06 21:15:58, Ilya Sherman wrote: > Hmm, why do we have this field at all, if it has a fixed value? This value is used if you just have a serialized blob and need to determine the type of the message. I talked to Denis, and he'll remove this on the server-side. For now, we need this here as the all responses from the prod server contain this field. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:105: optional string device_feature = 2; On 2014/10/06 21:15:58, Ilya Sherman wrote: > Is there a list of valid features? (Seems like this ought to be an enum field > rather than a field string...) I'm not sure why the generator turns enums into strings. It might be a bit of work to change this. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:111: // A (possibly empty) list of devices supporting the requested featur On 2014/10/06 21:15:59, Ilya Sherman wrote: > nit: "featur" -> "feature" Done. I'll also change this on the server-side so it won't get wiped the next time we update. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:119: // This request is sent by the login page to obtain one challenge session for On 2014/10/06 21:15:58, Ilya Sherman wrote: > What is the "login page"? The ChromeOS sign-in screen? I think this refers to the GAIA login page. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:217: // A Diffie-Hellman public key used perform a key exchange during enrollment. On 2014/10/06 21:15:59, Ilya Sherman wrote: > nit: "used perform" -> "used to perform" Done. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:224: optional string origin = 2; On 2014/10/06 21:15:59, Ilya Sherman wrote: > nit: Can this field be annotated as "[deprecated=true]"? I think this is a "soft" deprecation rather than a hard one, in that certain clients still use it. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:229: // Indicates whether a legacy crypto suite must be used with this device. On 2014/10/06 21:15:58, Ilya Sherman wrote: > Is there a specific legacy crypto suite that's being referred to? No idea... https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:232: // A URL describing which application facets this enrollment can be used (see On 2014/10/06 21:15:58, Ilya Sherman wrote: > nit: I don't understand this sentence :/ This seems to be something in the early stages of development. We don't use it for enrollment either. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:233: // http://go/appid). On 2014/10/06 21:15:58, Ilya Sherman wrote: > nit: Can we point to a publicly-accessible URL? I don't think this concept is fully implemented yet. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:252: // user's device has received a transaction, as well as to provide additional On 2014/10/06 21:15:58, Ilya Sherman wrote: > nit: Odd choice of line-break. Done. The generated code used a text-width of 100 chars (Java style guide). I'll run a formatter to make it fit 80 chars properly. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:277: // If true, Easyunlock will be enabled for the device with public key equal On 2014/10/06 21:15:59, Ilya Sherman wrote: > nit: "Easyunlock" -> "EasyUnlock" Done. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:286: // all of a On 2014/10/06 21:15:58, Ilya Sherman wrote: > nit: Odd choice of line break. Done.
Sorry forgot to upload the new patch.
LGTM % nits, thanks. https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:119: // This request is sent by the login page to obtain one challenge session for On 2014/10/07 00:06:06, Tim Song wrote: > On 2014/10/06 21:15:58, Ilya Sherman wrote: > > What is the "login page"? The ChromeOS sign-in screen? > > I think this refers to the GAIA login page. Hmm, do we show a GAIA login page as part of EasyUnlock? If not, why do we need this message type? https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:130: // its public key nit: Why did you rewrap this? It seemed fine before. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:174: // a device not eligible to be an unlock key, and we want to be able to add Ditto. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:220: // A Diffie-Hellman public key used perform a key exchange during enrollment. nit: "used perform" -> "used to perform" https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:276: // also causes other devices to sync the new EasyUnlock state. nit: Unnecessary rewrap https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:278: // If true, Easyunlock will be enabled for the device with public key equal nit: "Easyunlock" -> "EasyUnlock"
https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:119: // This request is sent by the login page to obtain one challenge session for On 2014/10/07 01:09:08, Ilya Sherman wrote: > On 2014/10/07 00:06:06, Tim Song wrote: > > On 2014/10/06 21:15:58, Ilya Sherman wrote: > > > What is the "login page"? The ChromeOS sign-in screen? > > > > I think this refers to the GAIA login page. > > Hmm, do we show a GAIA login page as part of EasyUnlock? If not, why do we need > this message type? We don't use this in Easy Unlock, but it might be useful for bootstrap. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:130: // its public key On 2014/10/07 01:09:09, Ilya Sherman wrote: > nit: Why did you rewrap this? It seemed fine before. I just did a vim autoformat (gq) on all comments, which puts as many words on a line as possible. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:174: // a device not eligible to be an unlock key, and we want to be able to add On 2014/10/07 01:09:08, Ilya Sherman wrote: > Ditto. Same. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:220: // A Diffie-Hellman public key used perform a key exchange during enrollment. On 2014/10/07 01:09:09, Ilya Sherman wrote: > nit: "used perform" -> "used to perform" Done. Sorry forgot to edit this file after editing the server-side. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:276: // also causes other devices to sync the new EasyUnlock state. On 2014/10/07 01:09:08, Ilya Sherman wrote: > nit: Unnecessary rewrap Same. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:278: // If true, Easyunlock will be enabled for the device with public key equal On 2014/10/07 01:09:08, Ilya Sherman wrote: > nit: "Easyunlock" -> "EasyUnlock" Done.
https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:119: // This request is sent by the login page to obtain one challenge session for On 2014/10/07 17:05:42, Tim Song wrote: > On 2014/10/07 01:09:08, Ilya Sherman wrote: > > On 2014/10/07 00:06:06, Tim Song wrote: > > > On 2014/10/06 21:15:58, Ilya Sherman wrote: > > > > What is the "login page"? The ChromeOS sign-in screen? > > > > > > I think this refers to the GAIA login page. > > > > Hmm, do we show a GAIA login page as part of EasyUnlock? If not, why do we > need > > this message type? > > We don't use this in Easy Unlock, but it might be useful for bootstrap. Hmm. My preference would be to not include messages that we don't use in Easy Unlock/Sign-in, since we can always add them if they become useful. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:130: // its public key On 2014/10/07 17:05:42, Tim Song wrote: > On 2014/10/07 01:09:09, Ilya Sherman wrote: > > nit: Why did you rewrap this? It seemed fine before. > > I just did a vim autoformat (gq) on all comments, which puts as many words on a > line as possible. It looks like that tool reformatted everything to characters per line rather than 80.
https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/20001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:119: // This request is sent by the login page to obtain one challenge session for On 2014/10/07 20:46:41, Ilya Sherman wrote: > On 2014/10/07 17:05:42, Tim Song wrote: > > On 2014/10/07 01:09:08, Ilya Sherman wrote: > > > On 2014/10/07 00:06:06, Tim Song wrote: > > > > On 2014/10/06 21:15:58, Ilya Sherman wrote: > > > > > What is the "login page"? The ChromeOS sign-in screen? > > > > > > > > I think this refers to the GAIA login page. > > > > > > Hmm, do we show a GAIA login page as part of EasyUnlock? If not, why do we > > need > > > this message type? > > > > We don't use this in Easy Unlock, but it might be useful for bootstrap. > > Hmm. My preference would be to not include messages that we don't use in Easy > Unlock/Sign-in, since we can always add them if they become useful. Okay I removed the unused APIs. https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:130: // its public key On 2014/10/07 20:46:41, Ilya Sherman wrote: > On 2014/10/07 17:05:42, Tim Song wrote: > > On 2014/10/07 01:09:09, Ilya Sherman wrote: > > > nit: Why did you rewrap this? It seemed fine before. > > > > I just did a vim autoformat (gq) on all comments, which puts as many words on > a > > line as possible. > > It looks like that tool reformatted everything to characters per line rather > than 80. I'm not following. The comments looks fine to me =/
https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:130: // its public key On 2014/10/14 00:30:24, Tim Song wrote: > On 2014/10/07 20:46:41, Ilya Sherman wrote: > > On 2014/10/07 17:05:42, Tim Song wrote: > > > On 2014/10/07 01:09:09, Ilya Sherman wrote: > > > > nit: Why did you rewrap this? It seemed fine before. > > > > > > I just did a vim autoformat (gq) on all comments, which puts as many words > on > > a > > > line as possible. > > > > It looks like that tool reformatted everything to characters per line rather > > than 80. > > I'm not following. The comments looks fine to me =/ Sorry, typing fail. It looks like the tool reformatted everything to 79 characters per line rather than 80.
https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... File components/proximity_auth/cryptauth/proto/cryptauth_api.proto (right): https://codereview.chromium.org/616233002/diff/40001/components/proximity_aut... components/proximity_auth/cryptauth/proto/cryptauth_api.proto:130: // its public key On 2014/10/14 00:38:10, Ilya Sherman wrote: > On 2014/10/14 00:30:24, Tim Song wrote: > > On 2014/10/07 20:46:41, Ilya Sherman wrote: > > > On 2014/10/07 17:05:42, Tim Song wrote: > > > > On 2014/10/07 01:09:09, Ilya Sherman wrote: > > > > > nit: Why did you rewrap this? It seemed fine before. > > > > > > > > I just did a vim autoformat (gq) on all comments, which puts as many words > > on > > > a > > > > line as possible. > > > > > > It looks like that tool reformatted everything to characters per line rather > > > than 80. > > > > I'm not following. The comments looks fine to me =/ > > Sorry, typing fail. It looks like the tool reformatted everything to 79 > characters per line rather than 80. Done.
The CQ bit was checked by tengs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616233002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5921080ce77d1f7674070f8cf96473b4d4b0d684 Cr-Commit-Position: refs/heads/master@{#299747} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
