Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655853006:
Define FIDO U2f Device abstraction (Closed)
|
Created:
3 years, 11 months ago by Casey Piper Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDefine FIDO U2f Device abstraction
U2fDevice is an abstraction of an individual U2f hardware
device. Each device supports Register, Sign, and GetVersion
commands as defined in the FIDO U2f specification.
BUG=686306
Review-Url: https://codereview.chromium.org/2655853006
Cr-Commit-Position: refs/heads/master@{#451837}
Committed: https://chromium.googlesource.com/chromium/src/+/fddb0db78957570df7f18f5af13b7a4fda898f88
Patch Set 1 : Define FIDO U2f Device abstraction #
Total comments: 10
Patch Set 2 : U2f Device abstraction #Patch Set 3 : Update includes #
Total comments: 8
Patch Set 4 : Fix nits #
Messages
Total messages: 27 (19 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add U2f response classes Define parameter object classes for U2f sign and register messages. BUG=NONE ========== to ========== U2fDevice is an abstraction of an individual U2f hardware device. Each device supports Register, Sign, and GetVersion commands as defined in the FIDO U2f specification. BUG=NONE ==========
Patchset #1 (id:20001) has been deleted
piperc@chromium.org changed reviewers: + reillyg@chromium.org
piperc@chromium.org changed required reviewers: + reillyg@chromium.org
piperc@chromium.org changed reviewers: + juanlang@chromium.org, kpaulhamus@chromium.org
Description was changed from ========== U2fDevice is an abstraction of an individual U2f hardware device. Each device supports Register, Sign, and GetVersion commands as defined in the FIDO U2f specification. BUG=NONE ========== to ========== Define FIDO U2f Device abstraction U2fDevice is an abstraction of an individual U2f hardware device. Each device supports Register, Sign, and GetVersion commands as defined in the FIDO U2f specification. BUG=None ==========
Please create a bug (if one doesn't exist) to track this development work and put it in the BUG= line. https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.cc File device/u2f/u2f_device.cc (right): https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.c... device/u2f/u2f_device.cc:81: return nullptr; nit: braces around if body when the condition is multiline. https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.h File device/u2f/u2f_device.h (right): https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.h... device/u2f/u2f_device.h:23: class U2fDevice : public base::RefCountedThreadSafe<U2fDevice> { I haven't commented on this in previous reviews because I haven't had a good vision of how you will be using these classes but I should mention that Chromium style certainly allows reference counted objects where necessary but encourages single ownership through std::unique_ptr (with base::WeakPtr for weak references) if at all possible as reference counting can lead to subtle bugs. https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.h... device/u2f/u2f_device.h:40: DeviceCallback; Please use "using foo = bar" instead of "typedef bar foo". https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.h... device/u2f/u2f_device.h:64: DeviceCallback callback) = 0; const DeviceCallback& https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.h... device/u2f/u2f_device.h:91: const std::vector<uint8_t>& key_handle); Since these methods don't depend on any properties of the device should they not be part of U2fApduCommand? https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.h... device/u2f/u2f_device.h:93: void OnRegisterComplete(RegistrationCallback callback, const RegistrationCallback& (ditto for other callbacks below) https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_responses.h File device/u2f/u2f_responses.h (right): https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_response... device/u2f/u2f_responses.h:18: : public base::RefCountedThreadSafe<U2fRegisterResponse> { I should have mentioned this in my response (har har) to you earlier. These should be plain structs with public members and be passed by const& or std::unique_ptr. Reference counting is unnecessary for something that will only have one owner.
Hi Casey, apologies for the slow review. https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.h File device/u2f/u2f_device.h (right): https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_device.h... device/u2f/u2f_device.h:74: static constexpr uint8_t kInsGetResponse = 0xC0; Pretty sure this isn't used? https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_responses.h File device/u2f/u2f_responses.h (right): https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_response... device/u2f/u2f_responses.h:22: std::vector<uint8_t> user_public_key, TL;DR: I think returning an opaque string of bytes with the entire response body might be preferable. This an interesting design approach, one which I admit I hadn't considered. Does the webauthn API return each individual element from the response separately? If it returns an opaque string of bytes like the U2F API does, it would be better to just return the data as a string of bytes, since it avoids having to parse the response only in order to convert back to raw bytes. Even if returns each item individually, I wonder whether it might be better to return U2F responses as opaque strings of bytes from the "bottom half" all the same. That'll allow you to avoid having to repackage the bytes yourself if you wire up a plain-old U2F top half to this bottom half. I think doing so will be a useful vehicle for testing your bottom half, e.g. if you wire up the CryptoToken extension to this bottom half. https://codereview.chromium.org/2655853006/diff/40001/device/u2f/u2f_response... device/u2f/u2f_responses.h:60: uint8_t presence, Same as above. If you accept my suggestion, you could use the same response class for all APDU methods.
Description was changed from ========== Define FIDO U2f Device abstraction U2fDevice is an abstraction of an individual U2f hardware device. Each device supports Register, Sign, and GetVersion commands as defined in the FIDO U2f specification. BUG=None ========== to ========== Define FIDO U2f Device abstraction U2fDevice is an abstraction of an individual U2f hardware device. Each device supports Register, Sign, and GetVersion commands as defined in the FIDO U2f specification. BUG=686306 ==========
Patchset #2 (id:60001) has been deleted
lgtm, just a bunch of nits and hints. https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.cc File device/u2f/u2f_device.cc (right): https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.... device/u2f/u2f_device.cc:21: if (register_cmd == nullptr) { if (!register_cmd) https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.... device/u2f/u2f_device.cc:25: DeviceTransact(register_cmd, You can std::move a scoped_refptr when you don't need it anymore in this scope and it saves an AddRef/Release. Plus then the code doesn't need to change if you decide to use a unique_ptr instead. https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.... device/u2f/u2f_device.cc:36: if (sign_cmd == nullptr) { if (!sign_cmd) https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.... device/u2f/u2f_device.cc:46: if (version_cmd == nullptr) { if (!version_cmd) https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.... device/u2f/u2f_device.cc:54: // TODO Put a NOTIMPLEMENTED(); in the body instead of the TODO. https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.... device/u2f/u2f_device.cc:60: // TODO Put a NOTIMPLEMENTED(); in the body instead of the TODO. https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.... device/u2f/u2f_device.cc:69: if (success && version_response != nullptr && && version_response && https://codereview.chromium.org/2655853006/diff/100001/device/u2f/u2f_device.... device/u2f/u2f_device.cc:79: // TODO Put a NOTIMPLEMENTED(); in the body instead of the TODO.
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by piperc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2655853006/#ps120001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by piperc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487702263368590,
"parent_rev": "803397e04777bc734a9c028813351c2e5079faf1", "commit_rev":
"fddb0db78957570df7f18f5af13b7a4fda898f88"}
Message was sent while issue was closed.
Description was changed from ========== Define FIDO U2f Device abstraction U2fDevice is an abstraction of an individual U2f hardware device. Each device supports Register, Sign, and GetVersion commands as defined in the FIDO U2f specification. BUG=686306 ========== to ========== Define FIDO U2f Device abstraction U2fDevice is an abstraction of an individual U2f hardware device. Each device supports Register, Sign, and GetVersion commands as defined in the FIDO U2f specification. BUG=686306 Review-Url: https://codereview.chromium.org/2655853006 Cr-Commit-Position: refs/heads/master@{#451837} Committed: https://chromium.googlesource.com/chromium/src/+/fddb0db78957570df7f18f5af13b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fddb0db78957570df7f18f5af13b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
