macOS: Add certificate trust API #9099

Merged
merged 36 commits into from Apr 4, 2017

Conversation

Projects
None yet
3 participants
@joshaber
Contributor

joshaber commented Apr 3, 2017

Continuing from #9068

Add the dialog.showCertificateTrustDialog API to show the OS-provided UI to let the user decide to accept and trust a self-signed or untrusted certificate:

cert_trust_panel_5b4faf16-9b52-4d4f-8aa0-63b89361dd60

This is only the macOS-side of things. I think we can get @shiftkey to do the Windows side.

I'm opening this early because I have no idea what I'm doing and I'd welcome any feedback or advice anyone would like to provide.

atom/browser/ui/certificate_trust.h
+
+void ShowCertificateTrust(atom::NativeWindow* parent_window,
+ const scoped_refptr<net::X509Certificate>& cert,
+ std::string message,

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Could this be const std::string& to match the other params?

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Could this be const std::string& to match the other params?

@@ -68,11 +69,72 @@ v8::Local<v8::Value> Converter<scoped_refptr<net::X509Certificate>>::ToV8(
val->GetIntermediateCertificates().front(),
issuer_intermediates);
dict.Set("issuerCert", issuer_cert);
+
+ std::vector<std::string> intermediates_encoded;
+ for (size_t i = 0; i < intermediates.size(); i++) {

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

I think this could be for (auto& native_cert : intermediates)

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

I think this could be for (auto& native_cert : intermediates)

atom/browser/api/atom_api_app.h
@@ -19,6 +19,7 @@
#include "content/public/browser/gpu_data_manager_observer.h"
#include "native_mate/handle.h"
#include "net/base/completion_callback.h"
+#include "net/cert/x509_certificate.h"

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

🔥

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

🔥

+ for (size_t i = 0; i < intermediates.size(); i++) {
+ auto native_cert = intermediates[i];
+ std::string encoded;
+ net::X509Certificate::GetPEMEncoded(native_cert, &encoded);

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

This method does return a bool, wonder if we should only push encoded to the vector when it returns true?

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

This method does return a bool, wonder if we should only push encoded to the vector when it returns true?

This comment has been minimized.

@joshaber

joshaber Apr 3, 2017

Contributor

I'm a little torn on that. On the one hand, I like being careful. On the other hand, we don't have any way of signaling that "yooooo we couldn't encode this one." And if we encode some nonsense, that has the funny upside of failing when we FromV8 the certificate. On the other hand, it's hard to know exactly what shape of nonsense we'll get if this fails.

What do you think?

@joshaber

joshaber Apr 3, 2017

Contributor

I'm a little torn on that. On the one hand, I like being careful. On the other hand, we don't have any way of signaling that "yooooo we couldn't encode this one." And if we encode some nonsense, that has the funny upside of failing when we FromV8 the certificate. On the other hand, it's hard to know exactly what shape of nonsense we'll get if this fails.

What do you think?

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

What do you think?

Hmm, yeah, looks like we don't use the return value on the root certificate so makes sense not to use it here either, can revisit separately.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

What do you think?

Hmm, yeah, looks like we don't use the return value on the root certificate so makes sense not to use it here either, can revisit separately.

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Would it make sense for intermediates to be an array of objects though that is the full cert information instead of just the encoded part?

I think it could just reuse what dict.Set("issuerCert", issuer_cert); does.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Would it make sense for intermediates to be an array of objects though that is the full cert information instead of just the encoded part?

I think it could just reuse what dict.Set("issuerCert", issuer_cert); does.

This comment has been minimized.

@joshaber

joshaber Apr 3, 2017

Contributor

Yeah I'm into it 👍

@joshaber

joshaber Apr 3, 2017

Contributor

Yeah I'm into it 👍

@joshaber joshaber changed the title from [WIP] macOS: Add certificate trust API to macOS: Add certificate trust API Apr 3, 2017

@joshaber

This comment has been minimized.

Show comment
Hide comment
@joshaber

joshaber Apr 3, 2017

Contributor

screen shot 2017-04-03 at 5 04 39 pm

I believe this is ready for review 🙏

Test method:

  1. Use Charles to put an SSL proxy in front of https://api.github.com.
  2. Launch Electron.app.
  3. Paste the following code into the inspector:
remote.app.on('certificate-error', (event, webContents, url, error, cert, cb) => {
  remote.dialog.showCertificateTrustDialog(remote.getCurrentWindow(), {certificate: cert, message: "yo this is super dodgey"}, () => {}) 
})
  1. Paste the following code into the inspector:
fetch('https://api.github.com/zen')
  1. The fetch fails and the certificate sheet is shown.
  2. Accept both certificates.
  3. Re-run fetch.
  4. Fetch succeeds.
Contributor

joshaber commented Apr 3, 2017

screen shot 2017-04-03 at 5 04 39 pm

I believe this is ready for review 🙏

Test method:

  1. Use Charles to put an SSL proxy in front of https://api.github.com.
  2. Launch Electron.app.
  3. Paste the following code into the inspector:
remote.app.on('certificate-error', (event, webContents, url, error, cert, cb) => {
  remote.dialog.showCertificateTrustDialog(remote.getCurrentWindow(), {certificate: cert, message: "yo this is super dodgey"}, () => {}) 
})
  1. Paste the following code into the inspector:
fetch('https://api.github.com/zen')
  1. The fetch fails and the certificate sheet is shown.
  2. Accept both certificates.
  3. Re-run fetch.
  4. Fetch succeeds.

@joshaber joshaber referenced this pull request in desktop/desktop Apr 3, 2017

Closed

Enterprise self-signed/untrusted certificate support #671

atom/browser/ui/certificate_trust_mac.mm
+ cert_db->NotifyObserversCertDBChanged(cert_.get());
+ }
+
+ callback_.Run(returnCode);

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Since the callback expects a bool, what do you think about comparing the value passed to the return code constants for clarity like:

 callback_.Run(returnCode == NSFileHandlingPanelOKButton ? true : false);

Or maybe add an else block that calls it with false and call it with true in the if block?

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Since the callback expects a bool, what do you think about comparing the value passed to the return code constants for clarity like:

 callback_.Run(returnCode == NSFileHandlingPanelOKButton ? true : false);

Or maybe add an else block that calls it with false and call it with true in the if block?

atom/browser/ui/certificate_trust_mac.mm
+ nil;
+ auto msg = base::SysUTF8ToNSString(message);
+
+ SFCertificateTrustPanel *panel = [[SFCertificateTrustPanel alloc] init];

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Super minor: SFCertificateTrustPanel* panel

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Super minor: SFCertificateTrustPanel* panel

atom/browser/api/atom_api_dialog.cc
@@ -119,13 +121,27 @@ void ShowSaveDialog(const file_dialog::DialogSettings& settings,
}
}
+#if defined(OS_MACOSX)
+void ShowCertificateTrust(atom::NativeWindow* parent_window,

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

I think you can delete this method and just use the following below:

dict.SetMethod("showCertificateTrustDialog",
               &certificate_trust::ShowCertificateTrust);
@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

I think you can delete this method and just use the following below:

dict.SetMethod("showCertificateTrustDialog",
               &certificate_trust::ShowCertificateTrust);
docs/api/dialog.md
@@ -175,6 +175,17 @@ it is usually used to report errors in early stage of startup. If called
before the app `ready`event on Linux, the message will be emitted to stderr,
and no GUI dialog will appear.
+### `dialog.showCertificateTrustDialog(browserWindow, certificate, message, callback)` _macOS_

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

If/when this gets support on other platforms there might be some additional options needed so it might be better to start this off as:

dialog.showCertificateTrustDialog(browserWindow, options, callback)

You can leave the C++ signature as-is, and just pluck the options in JS like showMessageBox does.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

If/when this gets support on other platforms there might be some additional options needed so it might be better to start this off as:

dialog.showCertificateTrustDialog(browserWindow, options, callback)

You can leave the C++ signature as-is, and just pluck the options in JS like showMessageBox does.

filenames.gypi
@@ -279,6 +279,8 @@
'atom/browser/ui/accelerator_util_views.cc',
'atom/browser/ui/atom_menu_model.cc',
'atom/browser/ui/atom_menu_model.h',
+ 'atom/browser/ui/certificate_trust_mac.mm',
+ 'atom/browser/ui/certificate_trust.h',

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Super minor: I think the order here should be swapped, looks like . sorts before _ in other places in this list like file_dialog.h.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Super minor: I think the order here should be swapped, looks like . sorts before _ in other places in this list like file_dialog.h.

atom/browser/ui/certificate_trust_mac.mm
+ return self;
+}
+
+- (void)panelDidEnd:(NSWindow *)sheet

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

🔥 the space between NSWindow and *

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

🔥 the space between NSWindow and *

atom/browser/ui/certificate_trust_mac.mm
+ certChain:(CFArrayRef)certChain
+ secPolicy:(SecPolicyRef)secPolicy;
+
+- (void)panelDidEnd:(NSWindow *)sheet

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

🔥 the space between NSWindow and *

@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

🔥 the space between NSWindow and *

@kevinsawicki kevinsawicki self-assigned this Apr 3, 2017

@joshaber

This comment has been minimized.

Show comment
Hide comment
@joshaber

joshaber Apr 4, 2017

Contributor

🏓

Contributor

joshaber commented Apr 4, 2017

🏓

@@ -73,6 +73,54 @@ v8::Local<v8::Value> Converter<scoped_refptr<net::X509Certificate>>::ToV8(
return dict.GetHandle();
}
+bool CertFromData(const std::string& data,

This comment has been minimized.

@deepak1556

deepak1556 Apr 4, 2017

Member

Can be moved to an anonymous namespace.

@deepak1556

deepak1556 Apr 4, 2017

Member

Can be moved to an anonymous namespace.

joshaber added some commits Apr 4, 2017

Don't pass the result through
It's meaningless on macOS, at least.
@joshaber

This comment has been minimized.

Show comment
Hide comment
@joshaber

joshaber Apr 4, 2017

Contributor

🍍

Contributor

joshaber commented Apr 4, 2017

🍍

lib/browser/api/dialog.js
+ }
+
+ let {certificate, message} = options
+ if (certificate == null || typeof options !== 'object') {

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 4, 2017

Contributor

Should this be typeof certificate !== 'object' instead?

@kevinsawicki

kevinsawicki Apr 4, 2017

Contributor

Should this be typeof certificate !== 'object' instead?

This comment has been minimized.

@joshaber

joshaber Apr 4, 2017

Contributor

Ha, good catch 👍

@joshaber

joshaber Apr 4, 2017

Contributor

Ha, good catch 👍

@joshaber

This comment has been minimized.

Show comment
Hide comment
@joshaber

joshaber Apr 4, 2017

Contributor

Contributor

joshaber commented Apr 4, 2017

@kevinsawicki kevinsawicki merged commit 3e9014c into master Apr 4, 2017

7 of 9 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #6107085 succeeded in 71s
Details
electron-linux-ia32 Build #6107086 succeeded in 66s
Details
electron-linux-x64 Build #6107087 succeeded in 148s
Details
electron-mas-x64 Build #3815 succeeded in 8 min 8 sec
Details
electron-osx-x64 Build #3812 succeeded in 8 min 39 sec
Details
electron-win-ia32 Build #2804 succeeded in 8 min 42 sec
Details
electron-win-x64 Build #2776 succeeded in 8 min 32 sec
Details
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 4, 2017

Contributor

Thanks @joshaber, great work on this 👍

Contributor

kevinsawicki commented Apr 4, 2017

Thanks @joshaber, great work on this 👍

@kevinsawicki kevinsawicki deleted the certificate-trust branch Apr 4, 2017

@shiftkey shiftkey referenced this pull request Apr 21, 2017

Merged

certificate trust API for Windows #9242

6 of 6 tasks complete

@dpgraham dpgraham referenced this pull request in appium/appium-desktop Jan 18, 2018

Closed

Add ability to trust SSL Certificates #390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment