Add cryptotokenPrivate and device support (hid and usb) for U2F #268

evq · 2017-07-31T18:03:31Z

This PR contains the following changes required to enable u2f support:

Instantiate the chrome device client to enable the existing chrome.hid and chrome.usb apis
Implement chrome.windows.get which is used by the cryptotoken extension
Build and register the chrome.cryptotokenPrivate extension api
Allow loading of component extensions from pak resources via the brave extension api

I have tested these changes on linux, mac and windows.

bridiver · 2017-08-03T00:54:35Z

atom/common/api/resources/windows_bindings.js

@@ -17,7 +17,21 @@ binding.registerCustomHook(function (bindingsAPI, extensionId) {
  })

  apiFunctions.setHandleRequest('get', function (windowId, getInfo, cb) {
-    console.warn('chrome.windows.get is not supported yet')
+    if (arguments.length === 1) {


you actually don't need all of these checks becausesetHandleRequest normalizes the arguments

bridiver · 2017-08-03T00:56:41Z

atom/common/api/resources/windows_bindings.js

+      getInfo = arguments[1]
+      cb = arguments[2]
+    }
+    var responseId = ++id


we've moved away from using ++id because it can produce duplicate requests across multiple windows. See tabs_bindings.js for usage of ipc.guid()

bridiver · 2017-08-17T17:53:20Z

BUILD.gn

@@ -121,6 +122,7 @@ repack("packed_extra_resources") {
    ":atom_resources",
    ":brave_resources",
    "app:brave_strings",
+    "//chrome/browser/resources:component_extension_resources_grit",


shouldn't think be //chrome/browser/resources:component_extension_resources? I don't see a target for component_extension_resources_grit

bridiver · 2017-08-17T17:55:57Z

atom/browser/extensions/atom_component_extension_resource_manager.cc

+AtomComponentExtensionResourceManager::
+~AtomComponentExtensionResourceManager() {}
+
+bool AtomComponentExtensionResourceManager::IsComponentExtensionResource(


most of this code looks identical to ChromeComponentExtensionResourceManager. We want to avoid copying chrome code so can you subclass instead?

I don't think I can subclass since AddComponentResourceEntries and path_to_resource_id_ are private

bridiver · 2017-08-17T18:17:09Z

chromium_src/BUILD.gn

        "//chrome/browser/extensions/event_router_forwarder.cc",
        "//chrome/browser/extensions/event_router_forwarder.h",
+        "//chrome/browser/extensions/window_controller.cc",


this looks like a potential problem to me because along with windows_util above. None of these methods will work correctly with our code so anything that uses them will get the wrong result. I think we need to look at the usage and possibly override with our own implementation

bridiver · 2017-08-17T18:18:25Z

lib/browser/api/extensions.js

@@ -603,6 +603,28 @@ var windowInfo = function (win, populateTabs) {
  }
 }

+ipcMain.on('chrome-windows-get', function (evt, responseId, windowId, getInfo) {


is this a rebase issue? Doesn't seem related

chrome.windows.get is needed by the cryptotoken extension, thus I implemented it

alexwykoff · 2017-10-31T17:58:55Z

What are the next steps @evq ?

evq · 2017-11-02T03:07:19Z

@alexwykoff
I have some local changes to address the requested changes which I was working on pre-Mercury, I need to rebase and finish those off. Barring major stumbling blocks during rebase, it shouldn't take much time at all once I can get back at it. Would really like to target the end of next week.

evq · 2017-12-22T00:29:34Z

@bridiver apologies for the long hiatus on this one... finally got some time to rebase, finish addressing your comments and test!

the primary change avoids copying a bunch of code from ChromeComponentExtensionResourceManager - I am now using it directly and have removed the small shim AtomComponentExtensionResourceManager. I've created a helper method to enable loading the extension manifest from the resources bundle.

bridiver · 2017-12-26T22:20:59Z

atom/browser/BUILD.gn

@@ -310,6 +310,8 @@ source_set("browser") {
      "extensions/api/atom_extensions_api_client.h",
      "extensions/atom_browser_client_extensions_part.cc",
      "extensions/atom_browser_client_extensions_part.h",
+      "extensions/atom_component_extensions.cc",


since this uses brave_resources.h we need to add a dep for brave_resources here too

evq · 2018-01-02T22:33:07Z

atom/browser/BUILD.gn

@@ -306,6 +308,8 @@ source_set("browser") {
  }

  if (enable_extensions) {
+    data_deps += [ "//electron:brave_resources" ]


@bridiver is this the right way to address your previous comment re: the missing brave_resources.h dependency? wasn't sure since it is generated and I'm not particularly familiar with GN

bridiver · 2018-01-09T00:20:33Z

brave/browser/api/brave_api_extension.cc

-    return NULL;
+
+  int resource_id;
+  if (!IsComponentExtension(path, &resource_id)) {


can we use IsComponentExtensionResource here instead? IsComponentExtension is a bit misleading because we have component extensions that are not loaded from resource paks

bridiver · 2018-01-09T00:31:39Z

muon/browser/muon_browser_process_impl.cc

@@ -19,6 +20,8 @@ MuonBrowserProcessImpl::MuonBrowserProcessImpl(
      const base::CommandLine& command_line) :
    BrowserProcessImpl(local_state_task_runner, command_line) {
  g_browser_process = this;
+
+  device_client_.reset(new ChromeDeviceClient);


this should actually go in chromium_src browser_process_impl.cc
the muon subclass is for muon-specific stuff

bridiver

please do a follow-up with the minor remaining changes

Add cryptotokenPrivate and device support (hid and usb) for U2F

bridiver reviewed Aug 3, 2017

View reviewed changes

evq force-pushed the evq/u2f branch 2 times, most recently from 3046d71 to 3a372c8 Compare August 14, 2017 19:02

evq changed the title ~~WIP Add cryptotokenPrivate and device support (hid and usb) for U2F~~ Add cryptotokenPrivate and device support (hid and usb) for U2F Aug 14, 2017

evq mentioned this pull request Aug 16, 2017

Add U2F support via cryptotoken extension brave/browser-laptop#10530

Merged

8 tasks

bridiver reviewed Aug 17, 2017

View reviewed changes

bridiver assigned evq Sep 29, 2017

bridiver added the PR/work-in-progress ⚒ label Sep 29, 2017

evq added 2 commits December 21, 2017 19:42

add cryptotokenPrivate, windows.get for u2f

b39c033

Allow loading cryptotoken extension from pak via brave extension API

9cd49b4

evq force-pushed the evq/u2f branch from acf4e8a to 74ff965 Compare December 22, 2017 00:11

address review comments

efa647b

evq force-pushed the evq/u2f branch from 74ff965 to efa647b Compare December 22, 2017 00:15

bridiver removed the PR/work-in-progress ⚒ label Dec 22, 2017

bridiver reviewed Dec 26, 2017

View reviewed changes

evq added 2 commits January 2, 2018 22:03

Merge remote-tracking branch 'origin/master' into evq/u2f

6180b4a

add data_dep for brave_resources

1c56f2f

evq commented Jan 2, 2018

View reviewed changes

bbondy mentioned this pull request Jan 4, 2018

Add cryptotokenPrivate and device support (hid and usb) for U2F brave/browser-laptop#12507

Closed

deps not data_deps

50cd559

bridiver reviewed Jan 9, 2018

View reviewed changes

bridiver approved these changes Jan 9, 2018

View reviewed changes

bridiver merged commit 45f057e into master Jan 9, 2018

bridiver added a commit that referenced this pull request Jan 9, 2018

Merge pull request #268 from brave/evq/u2f

97b06db

Add cryptotokenPrivate and device support (hid and usb) for U2F

bridiver added branch-4.6.x merged/branch-4.6.x labels Jan 9, 2018

bridiver added this to the 4.6.7 milestone Jan 9, 2018

diracdeltas mentioned this pull request Jan 18, 2018

Support U2F Keys brave/browser-laptop#518

Closed

bsclifton deleted the evq/u2f branch February 7, 2018 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cryptotokenPrivate and device support (hid and usb) for U2F #268

Add cryptotokenPrivate and device support (hid and usb) for U2F #268

evq commented Jul 31, 2017 •

edited

Loading

bridiver Aug 3, 2017

bridiver Aug 3, 2017

bridiver Aug 17, 2017

bridiver Aug 17, 2017

evq Aug 17, 2017

bridiver Aug 17, 2017 •

edited

Loading

bridiver Aug 17, 2017

evq Aug 17, 2017

alexwykoff commented Oct 31, 2017

evq commented Nov 2, 2017

evq commented Dec 22, 2017

bridiver Dec 26, 2017

evq Jan 2, 2018 •

edited

Loading

bridiver Jan 9, 2018

bridiver Jan 9, 2018

bridiver left a comment

Add cryptotokenPrivate and device support (hid and usb) for U2F #268

Add cryptotokenPrivate and device support (hid and usb) for U2F #268

Conversation

evq commented Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexwykoff commented Oct 31, 2017

evq commented Nov 2, 2017

evq commented Dec 22, 2017

Choose a reason for hiding this comment

evq Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

evq commented Jul 31, 2017 •

edited

Loading

bridiver Aug 17, 2017 •

edited

Loading

evq Jan 2, 2018 •

edited

Loading