certificate trust API for Windows #9242

Merged
merged 20 commits into from May 3, 2017

Conversation

Projects
None yet
5 participants
@shiftkey
Contributor

shiftkey commented Apr 21, 2017

This is the second half of #9099 - adding the certificate trust API for Windows.

The Setup

I've configured a local IIS instance with a self-signed certificate to trigger this failure over HTTPS. Edge and IE don't like it, and spit this error out at the user:

I can write up the way to setup this environment locally if others want to test this flow out themselves. The sample app I've been using can be found here: shiftkey/electron-test-network-windows

User Experience

The magic in certificate_trust_win.cc is some code I've tested in isolation to resolve the above error. It installs the received certificate to the Trusted Root Certificate for the current user (not the local computer) and because we're only doing it for the current user it doesn't require elevation.

However, interacting with the trusted root certificate store will trigger this dialog when you invoke CertAddCertificateContextToStore:

It's certainly not as pretty (or as functional) as the macOS equivalent, but that might be okay for now. As a result I'm also not using the message argument that ShowCertificateTrust receives. I'm open to putting more context into this flow based on feedback and security concerns - this is just a first pass.

Testing

I'm currently at the point where the test app has the right IPC setup to trigger the certificate error, but I'm missing something with the "swap in a debug version" process and the dialog.showCertificateTrustDialog method isn't visible on Windows (there's nothing in the original PR to suggest this is being hidden deliberately).

Here's my flow for testing this PR (I've probably missed something):

  • I added an #if defined here
  • python script\bootstrap.py --msvs to get a VS solution file
  • npm run build to get a debug build of my branch
  • copy out\D\* to node_modules\electron\dist\

I can then run the test harness, attach to the electron.exe processes and VS loads the PDB symbols for electron.exe and node.exe.

UPDATE: there's something up with how electron.asar is being packaged for debug builds - I've repackaged that archive with the showCertificateTrustDialog method and I can launch the dialog in the app.

TODO

  • test flow end-to-end in sample app
  • refresh certificate database if added
  • handle failure cases in ShowCertificateTrust gracefully, ensuring we cleanup any resources
  • validate certificate chain and only work with self-signed or untrusted root certificate
  • docs
  • throw some more testing at it

@shiftkey shiftkey referenced this pull request in desktop/desktop Apr 21, 2017

Closed

Enterprise self-signed/untrusted certificate support #671

shiftkey added some commits Apr 21, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 21, 2017

Contributor

Looks like a great start here, let me know if you have any questions or when it is ready to review,

Contributor

kevinsawicki commented Apr 21, 2017

Looks like a great start here, let me know if you have any questions or when it is ready to review,

@kevinsawicki kevinsawicki self-assigned this Apr 21, 2017

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Apr 22, 2017

Contributor

@kevinsawicki I think the only issue I have at this stage is with the debugging experience:

I think I'm missing something with the "swap in a debug version" process and the dialog.showCertificateTrustDialog method isn't visible on Windows (there's nothing in the original PR to suggest this is being hidden deliberately).

Is there anything I need to be aware of when generating electron.asar?

Contributor

shiftkey commented Apr 22, 2017

@kevinsawicki I think the only issue I have at this stage is with the debugging experience:

I think I'm missing something with the "swap in a debug version" process and the dialog.showCertificateTrustDialog method isn't visible on Windows (there's nothing in the original PR to suggest this is being hidden deliberately).

Is there anything I need to be aware of when generating electron.asar?

docs/api/dialog.md
-The `browserWindow` argument allows the dialog to attach itself to a parent
-window, making it modal.
+On macOS the `browserWindow` argument allows the dialog to attach itself to a parent
+window, making it modal. On Windows a modal dialog is not supported.

This comment has been minimized.

@joshaber

joshaber Apr 24, 2017

Contributor

Or it looks like it's always a modal dialog on Windows?

@joshaber

joshaber Apr 24, 2017

Contributor

Or it looks like it's always a modal dialog on Windows?

This comment has been minimized.

@shiftkey

shiftkey Apr 26, 2017

Contributor

This is probably getting way into obscure Win32 territory, but there's no way to pass a window handle to CertAddCertificateContextToStore to associate the popup message with a parent window - so modality isn't enforced here (that is, you can interact with the window without acknowledging the popup).

I need to tidy this these docs, but I also had a idea to show a dialog before this step with the message argument that the caller provides.

@shiftkey

shiftkey Apr 26, 2017

Contributor

This is probably getting way into obscure Win32 territory, but there's no way to pass a window handle to CertAddCertificateContextToStore to associate the popup message with a parent window - so modality isn't enforced here (that is, you can interact with the window without acknowledging the popup).

I need to tidy this these docs, but I also had a idea to show a dialog before this step with the message argument that the caller provides.

This comment has been minimized.

@joshaber

joshaber Apr 26, 2017

Contributor

Ahhhh I see 👍

@joshaber

joshaber Apr 26, 2017

Contributor

Ahhhh I see 👍

@shiftkey shiftkey changed the title from [WIP] certificate trust API for Windows to certificate trust API for Windows Apr 27, 2017

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Apr 27, 2017

Contributor

I found myself off spending way too much time thinking about certificate chains and trust which we didn't really need to do here (only handle self-signed certs).

I'm going to throw some more testing at it in a live app to ensure I haven't missed anything but I think this is ready for a first round of reviewing.

Contributor

shiftkey commented Apr 27, 2017

I found myself off spending way too much time thinking about certificate chains and trust which we didn't really need to do here (only handle self-signed certs).

I'm going to throw some more testing at it in a live app to ensure I haven't missed anything but I think this is ready for a first round of reviewing.

atom/browser/ui/certificate_trust_win.cc
+// store for the current user.
+//
+// This requires prompting the user to confirm they trust the certificate.
+BOOL AddToTrustedRootStore(const PCCERT_CONTEXT certContext,

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

Underscore case names (cert_context) are preferred, looks like there are several camel cases in this file currently.

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

Underscore case names (cert_context) are preferred, looks like there are several camel cases in this file currently.

atom/browser/ui/certificate_trust_win.cc
+ NULL,
+ NULL,
+ &chainContext)) {
+ switch (chainContext->TrustStatus.dwErrorStatus) {

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

Maybe an if block would make this a little more direct and concise, unless you are planning to add more cases to this block in this PR:

if (chainContext->TrustStatus.dwErrorStatus == CERT_TRUST_IS_SELF_SIGNED) {
  AddToTrustedRootStore(pCertContext, cert);
}
@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

Maybe an if block would make this a little more direct and concise, unless you are planning to add more cases to this block in this PR:

if (chainContext->TrustStatus.dwErrorStatus == CERT_TRUST_IS_SELF_SIGNED) {
  AddToTrustedRootStore(pCertContext, cert);
}
docs/api/dialog.md
+On macOS, this displays a modal dialog that shows a message and certificate
+information, and gives the user the option of trusting/importing the
+certificate. If you provide a `browserWindow` argument the dialog will be
+attached to the parent window, makign it modal.

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

makign -> making

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

makign -> making

docs/api/dialog.md
-window, making it modal.
+On Windows the options are more limited, due to the Win32 APIs used:
+
+ - the `message` argument is not used, as the OS provides it's own confirmation

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

it's -> its

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

it's -> its

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

the -> The

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

the -> The

docs/api/dialog.md
+
+ - the `message` argument is not used, as the OS provides it's own confirmation
+ dialog.
+ - it is not possible to make this confirmation dialog modal.

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

Maybe expand a bit to mention browserWindow is ignored:

The `browserWindow` argument is ignored since it is not possible to make this confirmation dialog modal.
@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

Maybe expand a bit to mention browserWindow is ignored:

The `browserWindow` argument is ignored since it is not possible to make this confirmation dialog modal.
@kevinsawicki

Left a few very minor style comments, code looks great otherwise 👍

atom/browser/ui/certificate_trust_win.cc
@@ -0,0 +1,101 @@
+// Copyright (c) 2013 GitHub, Inc.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@paulcbetts

paulcbetts May 4, 2017

Contributor

Oh I just mean code that would be good for copy-pasting - any time you can copy-paste security code instead of writing it yourself it's a Good Thing

@paulcbetts

paulcbetts May 4, 2017

Contributor

Oh I just mean code that would be good for copy-pasting - any time you can copy-paste security code instead of writing it yourself it's a Good Thing

atom/browser/ui/certificate_trust_win.cc
@@ -0,0 +1,101 @@
+// Copyright (c) 2013 GitHub, Inc.

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

2013 -> 2017

@kevinsawicki

kevinsawicki Apr 28, 2017

Contributor

2013 -> 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki May 1, 2017

Contributor

I've configured a local IIS instance with a self-signed certificate to trigger this failure over HTTPS. Edge and IE don't like it, and spit this error out at the user

So I tried testing this using https://badssl.com via fetch('https://self-signed.badssl.com/') and the error status I got was CERT_TRUST_IS_UNTRUSTED_ROOT, so I'm wondering do you think that status should be handled the same as CERT_TRUST_IS_SELF_SIGNED?

Contributor

kevinsawicki commented May 1, 2017

I've configured a local IIS instance with a self-signed certificate to trigger this failure over HTTPS. Edge and IE don't like it, and spit this error out at the user

So I tried testing this using https://badssl.com via fetch('https://self-signed.badssl.com/') and the error status I got was CERT_TRUST_IS_UNTRUSTED_ROOT, so I'm wondering do you think that status should be handled the same as CERT_TRUST_IS_SELF_SIGNED?

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey May 2, 2017

Contributor

@kevinsawicki yep, we can handle those two in the same way - looks like we get CERT_TRUST_IS_SELF_SIGNED when the certificate is tied to a specific domain, and CERT_TRUST_IS_UNTRUSTED_ROOT for a wildcard certificate that's untrusted.

Contributor

shiftkey commented May 2, 2017

@kevinsawicki yep, we can handle those two in the same way - looks like we get CERT_TRUST_IS_SELF_SIGNED when the certificate is tied to a specific domain, and CERT_TRUST_IS_UNTRUSTED_ROOT for a wildcard certificate that's untrusted.

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey May 2, 2017

Contributor

Yep, I'm happy with the testing of this for those two scenarios above. Let me know if there's anything else left to do to get this merged 🤘

Contributor

shiftkey commented May 2, 2017

Yep, I'm happy with the testing of this for those two scenarios above. Let me know if there's anything else left to do to get this merged 🤘

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey May 3, 2017

Contributor

@kevinsawicki friendly bump 🤜

Contributor

shiftkey commented May 3, 2017

@kevinsawicki friendly bump 🤜

@kevinsawicki kevinsawicki merged commit c59fab0 into electron:master May 3, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki May 3, 2017

Contributor

Thanks @shiftkey 👍

Contributor

kevinsawicki commented May 3, 2017

Thanks @shiftkey 👍

@coreybutler

This comment has been minimized.

Show comment
Hide comment
@coreybutler

coreybutler May 19, 2017

Correct me if I'm wrong, but it looks like this only supports importing certificates to the operating systems' trust chain? If this is the case, self signed certs will still throw up a nastygram in Firefox, which relies exclusively on its own certdb.

Correct me if I'm wrong, but it looks like this only supports importing certificates to the operating systems' trust chain? If this is the case, self signed certs will still throw up a nastygram in Firefox, which relies exclusively on its own certdb.

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey May 19, 2017

Contributor

Correct me if I'm wrong, but it looks like this only supports importing certificates to the operating systems' trust chain?

Yes, because this is what an Electron app relies on for it's certificate validation.

Contributor

shiftkey commented May 19, 2017

Correct me if I'm wrong, but it looks like this only supports importing certificates to the operating systems' trust chain?

Yes, because this is what an Electron app relies on for it's certificate validation.

@shiftkey shiftkey deleted the shiftkey:certificate-addition-windows branch May 19, 2017

@coreybutler

This comment has been minimized.

Show comment
Hide comment
@coreybutler

coreybutler May 21, 2017

Got it, thanks.

I have an Electron app that spins up local Express servers and generates self-signed certs on the fly. Handling trust chain support for the OS wasn't too taxing, but Firefox support was pretty gnarly... I was hoping for a cleaner approach :-) I may still use this for working with OS trust chains though.

Great work on all of this.

Got it, thanks.

I have an Electron app that spins up local Express servers and generates self-signed certs on the fly. Handling trust chain support for the OS wasn't too taxing, but Firefox support was pretty gnarly... I was hoping for a cleaner approach :-) I may still use this for working with OS trust chains though.

Great work on all of this.

@shiftkey shiftkey referenced this pull request in desktop/desktop Aug 30, 2017

Merged

enable adding self-signed certificates on Windows #2581

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