Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for the U2F Web API #30438

Merged
merged 7 commits into from
Aug 30, 2021
Merged

feat: add support for the U2F Web API #30438

merged 7 commits into from
Aug 30, 2021

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Aug 6, 2021

Closes #3226

The large files in this PR namely cryptotoken_private_api.cc and cryptotoken_private.idl are 99% copied from Chromium source. They're copied for build system reasons, the difference is as follows;

  • IDL file --> Identical
  • API impl --> Removed usage of Chromiums tab helper and profile classes

Everything else is just annoyingly undocumented wiring to make this magic U2F component extension work.

Tested this out on https://mdp.github.io/u2fdemo and it appears to work great 馃憤

Notes: Added support for the U2F Web API

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

broadly lgtm

appveyor.yml Outdated Show resolved Hide resolved
// permissions::PermissionRequestManager::FromWebContents(web_contents);
// nullptr;
// if (!permission_request_manager) {
return RespondNow(Error("no PermissionRequestManager"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... it looks like we always respond with an error here. Do we need to expose something through our permission check handlers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API isn't actually used by the U2F extension, I think it might be a private API that some google.com sites use 馃 So I wasn't too concerned with wiring this up to anything in particular.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other extensions APIs that I've copied from Chrome, I've stripped out the bits we explicitly don't want to support. If we don't care about this API at all, maybe replace the entire implementation with just return RespondNow(Error("API not supported in Electron")) or so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that but in order to make diffing easier in the future I left the old implementation here with #if 0 wrappers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm ok, i'm not sure that makes diffing easier but sure.

shell/browser/extensions/electron_extension_system.cc Outdated Show resolved Hide resolved
Comment on lines +198 to +203
// Profile* const profile = Profile::FromBrowserContext(browser_context());
// const PrefService* const prefs = profile->GetPrefs();
// const base::ListValue* const permit_attestation =
// prefs->GetList(prefs::kSecurityKeyPermitAttestation);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probs don't need both // and #if 0

// permissions::PermissionRequestManager::FromWebContents(web_contents);
// nullptr;
// if (!permission_request_manager) {
return RespondNow(Error("no PermissionRequestManager"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm ok, i'm not sure that makes diffing easier but sure.

Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but is there any way to add a test for this feature?

@MarshallOfSound
Copy link
Member Author

LGTM, but is there any way to add a test for this feature?

Not without a U2F key plugged in and a human there to touch it.

@release-clerk
Copy link

release-clerk bot commented Aug 30, 2021

Release Notes Persisted

Added support for the U2F Web API

@ankurk91
Copy link

ankurk91 commented Sep 8, 2021

This feature is coming to version 14 or 15?

@ruiquelhas
Copy link

@MarshallOfSound you mention you tested it on https://mdp.github.io/u2fdemo, but I also did the same, and nothing seems to happen.

I've tried it with Electron v16.0.0 using electron-quick-start app and a U2F authenticator.

$ git clone https://github.com/electron/electron-quick-start
$ cd electron-quick-start
$ npm install && npm start

And then loading the registration page in main.js.

diff --git a/main.js b/main.js
index 815c04b..6b58c35 100644
--- a/main.js
+++ b/main.js
@@ -13,10 +13,11 @@ function createWindow () {
   })
 
   // and load the index.html of the app.
-  mainWindow.loadFile('index.html')
+  // mainWindow.loadFile('index.html')
+  mainWindow.loadURL('https://mdp.github.io/u2fdemo/#reg')
 
   // Open the DevTools.
-  // mainWindow.webContents.openDevTools()
+  mainWindow.webContents.openDevTools()
 }
 
 // This method will be called when Electron has finished

It logs the following in the console until the timeout kicks in:

Screenshot 2021-11-18 at 15 55 10

@moughxyz
Copy link

moughxyz commented Feb 2, 2023

It looks like the U2F API is deprecated in favor of WebAuthn: https://www.yubico.com/blog/google-chrome-u2f-api-decommission/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port Chromium U2F support to Electron
7 participants