Use HTTPS with BrowserSync #57

ataylorme · 2018-07-16T18:07:55Z

Description

Addresses issue #6. Bundles a default key/cert generated from the instructions in this great article.

The certs need to be trusted. I was able to do this on macOS High Sierra by dragging the certificate file into the keychain.

List of changes

Add the dev/config/BrowserSync directory with an SSL cert, key and config.
Default to HTTPS true for BrowserSync
Add options to dev/config/themeConfig.js to allow for custom certificates

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

This pull request relates to a ticket.
My code is tested.
I want my code added to WP Rig.

Bundle a default key/cert generated from the instructions in [this great article](https://deliciousbrains.com/https-locally-without-browser-privacy-errors/).

ataylorme · 2018-07-16T18:15:26Z

@mor10 or @hellofromtonya I would love someone else to test this and see if they can get the certs trusted and working for them. I think it makes sense to bundle a cert rather than walk folks through creating one.

We will need to document the step to add the cert to the keychain. I also only have a Mac and can't test on Windows or Linux.

ataylorme · 2018-07-16T18:51:29Z

This cert was generated with subjectAlternativeName to appease the all mighty Chrome - see this Chrome feature for details.

ataylorme · 2018-07-16T18:52:12Z

Once #55 is merged I will rebase it here, which should allow Travis to pass.

bamadesigner

Hi @ataylorme! I left a few notes to get started.

bamadesigner · 2018-07-19T22:02:45Z

dev/config/themeConfig.js

@@ -10,7 +10,12 @@ module.exports = {
 		browserSync: {
 			live: true,
 			proxyURL: 'wprig.test:8888',
-			bypassPort: '8181'
+            bypassPort: '8181',


Can we fix the spacing issues here?

bamadesigner · 2018-07-19T22:04:02Z

gulp/browserSync.js

@@ -4,6 +4,8 @@
 // External dependencies
 import requireUncached from 'require-uncached';
 import browserSync from 'browser-sync';
+import log from 'fancy-log';


Hey @ataylorme! I don't mind introducing logging and colors, but let's do so in its own PR. Then we can add it for other functionality.

The logging packages are already in (see #47 (comment)), I am just making use of them.

Would you prefer using log messages to be a separate PR later?

I'm fine with keeping the logging in. Thanks!

bamadesigner · 2018-07-19T22:04:57Z

gulp/browserSync.js

+        log(colors.yellow(`Using the default SSL key ${colors.bold(keyPath)}`));
+    }
+
+    if (config.dev.browserSync.https) {


We should check this logic first, because otherwise there's no point in running the rest.

And it should all be moved inside if (config.dev.browserSync.live).

bamadesigner · 2018-07-19T22:07:36Z

gulp/browserSync.js

@@ -21,12 +23,42 @@ export function serve(done) {
    // get a fresh copy of the config
    const config = requireUncached(paths.config.themeConfig);

+    let serverConfig = {
+        proxy: config.dev.browserSync.proxyURL,


Should we be checking to make sure these are defined?

bamadesigner · 2018-07-19T22:09:21Z

Also, FYI, we already had a PR open for this. So we'll need to keep it in mind.

ataylorme · 2018-07-19T22:17:33Z

Yes, I saw #7 and used that PR for inspiration ;-). However, it does not supply a cert and was made before the recent gulp changes. With my research into what makes Chrome happy generating a cert is not fun so bundling one is preferred if possible.

@bamadesigner were you able to test this and see if the cert works for you? It worked for me but I also generated it so would like a second (and maybe third) opinion. You'll need to add the cert file to the keychain and mark as trusted but it should be okay after that since the BrowserSync URL is hard coded and the cert matches.

mor10 · 2018-07-19T23:40:46Z

Looping in @miklb as he was the one who put in the original issue (#6) and PR (#7). See @ataylorme's request for testing.

It makes more sense to have this check at the beginning rather than a conditional later.

…orme/wprig into issue-6-browser-sync-https-support

ataylorme · 2018-07-19T23:51:59Z

gulp/browserSync.js

-			liveReload: true
-		});
+    // bail early if not serving via BrowserSync
+    if (! config.dev.browserSync.live) {


And it should all be moved inside if (config.dev.browserSync.live).

@bamadesigner I took the opposite approach and just exit early if config.dev.browserSync.live is not true

Good call. Let's wrap L39-63 in the check for if (config.dev.browserSync.https), since there's no point if no https.

miklb · 2018-07-20T00:41:35Z

Let me get my local fork updated and will test both with my local certs as well as the generated.

Much appreciated taking a look at it.

ataylorme · 2018-07-26T20:02:53Z

@miklb have you had a chance to test this yet?

miklb · 2018-07-26T22:32:57Z

apologies, I have not, was out of town for a few days. I can definitely test this evening.

bamadesigner · 2018-07-27T00:13:59Z

I'm testing as well. Thanks for being patient with me. I had to knock out a project after WPCampus.

bamadesigner · 2018-07-27T00:12:56Z

gulp/browserSync.js

-			liveReload: true
-		});
+    // bail early if not serving via BrowserSync
+    if (! config.dev.browserSync.live) {


Good call. Let's wrap L39-63 in the check for if (config.dev.browserSync.https), since there's no point if no https.

bamadesigner · 2018-07-27T00:30:19Z

@ataylorme if there are required steps for the user to implement, e.g. "add the cert file to the keychain and mark as trusted", lets make sure we add instructions in the README.

Current status: I added to Keychain Access, marked to always trust, re built the theme but its struggling to connect.

Update: it's working now. I forgot to update my config file with my local proxyURL.

miklb · 2018-07-27T03:10:14Z

dev/config/themeConfig.js

+
+            // Use a custom cert/key if desired
+			// certPath: '',
+			// keyPath: ''


this needs a , // keyPath: '',

miklb · 2018-07-27T04:07:42Z

I misunderstood and thought the script was generating a unique local cert. I'm not inclined to accept a public cert like that in my browser.

I attempted to use my own cert and key and other than the two missing commas and I'm able to

However it seems like for some reason it's not cache busting the version number of the css file when I update a file. Specifically I edited dev/style.css and change background color of <body>. Gulp styles runs, reload completes and Browsersync reloads message is displayed. However background color doesn't change. I tested outside of Browsersync, and multiple browsers.

OK, looking closer,wp_enqueue_style is hardcoding the ver 20180514 if I'm reading that correctly. Which I'm guessing the browser is caching and not updating with the changes. But I'm rusty on how that works, and definitely not sure if it's related to https.

bamadesigner · 2018-07-27T14:29:03Z

@miklb Yea, I believe the cache stuff is on purpose, plus some of that is being worked on in #39.

@ataylorme and I are gonna have a working session early next week, confirm everything and close this up. Have you had a chance to test @mor10? You feel good about this?

ataylorme · 2018-07-27T15:37:38Z

this needs a , // keyPath: '',

Good catch @miklb, this has been updated.

I misunderstood and thought the script was generating a unique local cert. I'm not inclined to accept a public cert like that in my browser.

@miklb can you expand on this concern? I would object as well if we were recommending the certs be used for production but for local development I don't think it is a concern.

Using your own cert is great for users with the knowledge to create one but generating a self-signed cert with the proper Subject Alternative Name for Chrome to not show warnings took some education, creating an OpenSSL configuration file, and more than one attempt.

By including the certs here I am hoping others do not have to go through the same thing and can get up and running quickly. Since the BrowserSync domain is constant at localhost even when the port is changed the cert works.

The alternative would be to not include a cert in WP Rig and ask users to generate their own. We would need to provide ample documentation on how to do so. I'm happy to discuss further but would need some convincing before putting that burden on WP Rig users.

ataylorme · 2018-07-27T15:59:37Z

if there are required steps for the user to implement, e.g. "add the cert file to the keychain and mark as trusted", lets make sure we add instructions in the README.

@bamadesigner I added instructions for Mac but I don't have access to a Windows on Linux machine to test. The README has

WP Rig has been tested on Linux, Mac, and Windows.

How have you been testing Windows/Linux? I'd love help making sure those are documented as well

miklb · 2018-07-27T16:33:32Z

I would object as well if we were recommending the certs be used for production but for local development I don't think it is a concern.

It is true that the key is tied to the domain (which might need to be documented? if I change the projectURL the key shouldn't work with anything but wprig.test:8888 )

Once you're browser and machine accept that cert, you are trusting ANY site served with that cert/domain. So an attacker would need to poison your dns in order to have the malicious site use the same domain name as is on the certificate. Highly unlikely, but as a rule I would never suggest accepting a cert/key that is just floating around in a GitHub repo.

I wonder if a one off Gulp task could be written to generate the local key/cert. Something like gulp https that would generate a key using the projectURL from themeConfig.js

ataylorme · 2018-07-27T16:42:52Z

It is true that the key is tied to the domain (which might need to be documented? if I change the projectURL the key shouldn't work with anything but wprig.test:8888 )

It is tied to localhost, which BrowserSync uses. The URL you are proxying can change without issue. The BrowserSync port can change as well. I don't believe BrowserSync can be configured to use a different domain.

I wonder if a one off Gulp task could be written to generate the local key/cert. Something like gulp https that would generate a key using the projectURL from themeConfig.js

I think this would be very hard to do. Different operating systems aside, even for Mac which version of OpenSSL and other unpredictable things would make this unreliable.

Once you're browser and machine accept that cert, you are trusting ANY site served with that cert/domain. So an attacker would need to poison your dns in order to have the malicious site use the same domain name as is on the certificate.

I'm +1 on adding this info to the README. I'm curious though how this is different even if each user generates their own cert as you would still need to trust a cert with localhost as the SAN.

To be really risk-averse you could trust the cert when you start developing locally after disabling WiFi and then distrust it again when you are done working and re-enable WiFi.

miklb · 2018-07-27T16:56:33Z

I did not realize the cert was tied to localhost. I’ll need to read up on security issues there before commenting further. My only experience is with domain level certs.

miklb · 2018-07-27T17:14:20Z

OK, this seems to be the best argument against using localhost

https://letsencrypt.org/docs/certificates-for-localhost/

Besides the policy issues, it would be a bad idea to encourage developers to connect to an HTTPS origin for which the private key is publicly available.

letsencrypt/boulder#137 (comment)

ataylorme · 2018-07-27T17:29:11Z

@miklb thanks for the research/comments. This is an area I am not super familiar with as well. I'm coming around to the alternative of making users generate their own certs.

We should not toss security out for the sake of convenience. Security vs convenience is a constant battle but in this case, asking users to generate their own certs may be necessary.

It would be a one-time effort as once they have a cert if would work for multiple WP Rig projects.

I'd like to see what @bamadesigner and @mor10 have to say as well.

ataylorme · 2018-07-27T17:40:15Z

@ericmann you are the most knowledgeable security person I know. Any chance you can weigh in?

ericmann · 2018-07-27T17:53:26Z

@ataylorme I would highly recommend against distributing a cert and private key with the project. There are a lot of applications that leverage localhost for internal communication. That anyone on the Internet could pick up a key/cert pair that would be accepted by my machine is troubling. Yes, it would take a certain level of skill to exploit the issue, but there's still a risk.

In short: never distribute a private key in a repo. Ever. And in order for a local environment to leverage a pre-generated cert, it'd need the private key. In other words, this is a bad idea.

However, your heart's in the right place!

I wonder if a one off Gulp task could be written to generate the local key/cert.

Something like https://github.com/jfromaniello/selfsigned could likely be used in a Gulp task to generate a key and cert locally. Then use that generated key (and trust it in your keystore) using existing instructions. As the key is generated offline, no one will be able to abuse it.

ataylorme · 2018-07-27T18:25:25Z

Thanks for the quick feedback @ericmann. I've removed the bundled cert/key. I will explore generating a cert/key with gulp but for now, I have just added a link to the LetsEncrypt instructions in the README.

I also set https to false by default in the config as it won't "just work" and updated the BrowserSync config.

ataylorme · 2018-07-27T19:22:51Z

I tested both with and without a custom cert. Screenshots of the results below. One thing to note is that my local WordPress installation that BrowserSync is proxying has HTTPS forced. Putting an https URL in the proxyURL will cause BrowserSync to run at https even if the https option is explicitly set to false. The is still served fine, it just shows as insecure without the cert.

Without a custom cert

With a custom cert

ataylorme · 2018-07-27T19:23:24Z

@miklb caching issues aside can you test with your custom cert/key as well?

bamadesigner · 2018-07-27T19:50:02Z

Thanks for all the hard work and research!

miklb · 2018-07-27T20:22:44Z

caching issues aside can you test with your custom cert/key as well?

@ataylorme custom cert/key works, caching issues aside.

Give me a shout if you want a hand on the task to create the key/cert. I think that would be pretty rad.

ataylorme · 2018-07-27T21:08:51Z

Give me a shout if you want a hand on the task to create the key/cert. I think that would be pretty rad.

I won't be able to play around with that until next week. Help would be great if you want to give it a go. https://www.npmjs.com/package/@magento/devcert looks promising.

ataylorme · 2018-08-09T22:04:40Z

Something like jfromaniello/selfsigned could likely be used in a Gulp task to generate a key and cert locally.

I think generating a cert with gulp can be a separate feature/PR later and shouldn't hold this back.

mor10 · 2018-08-09T22:46:38Z

I have tested and this works, though the setup is rather clunky. Not that we can do much about it as the clunkiness is centered around how to get certificates up and running. Nice work!

mor10 · 2018-08-09T23:38:58Z

Feel free to merge once you feel the issues you've flagged above are sufficiently handled @bamadesigner

mor10 · 2018-08-09T23:53:41Z

Agreed @ataylorme: Let's get this thing wired up first, and then talk about automatic generation of certs. That would probably involve some serious documentation work and testing which warrants a separate PR.

bamadesigner · 2018-08-10T05:53:24Z

Thanks for all the hard work @ataylorme!

Use HTTPS with BrowserSync

a1831fd

Bundle a default key/cert generated from the instructions in [this great article](https://deliciousbrains.com/https-locally-without-browser-privacy-errors/).

bamadesigner requested changes Jul 19, 2018

View reviewed changes

Merge branch 'v1.1' into issue-6-browser-sync-https-support

1fc3117

mor10 added enhancement New feature or request help wanted Extra attention is needed gulp Gulp related issues labels Jul 19, 2018

ataylorme and others added 4 commits July 19, 2018 16:42

Correct spacing

f539b07

Bail early if not serving via BrowserSync

08a76cc

It makes more sense to have this check at the beginning rather than a conditional later.

Merge branch 'issue-6-browser-sync-https-support' of github.com:atayl…

c16be99

…orme/wprig into issue-6-browser-sync-https-support

Not config.dev.browserSync.live

2b7e3ca

ataylorme commented Jul 19, 2018

View reviewed changes

bamadesigner requested changes Jul 27, 2018

View reviewed changes

miklb reviewed Jul 27, 2018

View reviewed changes

dev/config/themeConfig.js Outdated

// Use a custom cert/key if desired

// certPath: '',

// keyPath: ''

Copy link

miklb Jul 27, 2018

Choose a reason for hiding this comment

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

this needs a , // keyPath: '',

Add missing trailing comma after keyPath

afe4148

ataylorme added 2 commits July 27, 2018 08:41

Add link to article on generating an SSL cert for local development

8beee6f

Add instructions for trusting SSL certificate

66c0786

Only setup HTTPS certificates if HTTPS is enabled

3a69c8f

ataylorme added 3 commits July 27, 2018 11:05

Remove bundled certs

9abfd01

Default HTTPS to off and require a custom key/cert

a08cbe9

Update HTTPS instructions

07ed2c5

Remove unused browserSync constant

fe7e03b

ataylorme mentioned this pull request Aug 9, 2018

Add the ability to generate an SSL certificate with gulp #81

Closed

Simply fixing syntax

33c14c4

bamadesigner approved these changes Aug 10, 2018

View reviewed changes

bamadesigner merged commit 36353a4 into wprig:v1.1 Aug 10, 2018

bamadesigner mentioned this pull request Aug 10, 2018

add: support for https in Browser Sync #7

Closed

Use HTTPS with BrowserSync #57

Use HTTPS with BrowserSync #57

Conversation

ataylorme commented Jul 16, 2018 • edited

Description

List of changes

Checklist:

ataylorme commented Jul 16, 2018

ataylorme commented Jul 16, 2018

ataylorme commented Jul 16, 2018

bamadesigner left a 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

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

bamadesigner commented Jul 19, 2018

ataylorme commented Jul 19, 2018

mor10 commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miklb commented Jul 20, 2018

ataylorme commented Jul 26, 2018

miklb commented Jul 26, 2018

bamadesigner commented Jul 27, 2018

Choose a reason for hiding this comment

bamadesigner commented Jul 27, 2018 • edited

Choose a reason for hiding this comment

miklb commented Jul 27, 2018 • edited

bamadesigner commented Jul 27, 2018

ataylorme commented Jul 27, 2018

ataylorme commented Jul 27, 2018

miklb commented Jul 27, 2018

ataylorme commented Jul 27, 2018

miklb commented Jul 27, 2018

miklb commented Jul 27, 2018

ataylorme commented Jul 27, 2018

ataylorme commented Jul 27, 2018

ericmann commented Jul 27, 2018

ataylorme commented Jul 27, 2018

ataylorme commented Jul 27, 2018

ataylorme commented Jul 27, 2018

bamadesigner commented Jul 27, 2018

miklb commented Jul 27, 2018

ataylorme commented Jul 27, 2018

ataylorme commented Aug 9, 2018

mor10 commented Aug 9, 2018

mor10 commented Aug 9, 2018

mor10 commented Aug 9, 2018

bamadesigner commented Aug 10, 2018

ataylorme commented Jul 16, 2018 •

edited

bamadesigner commented Jul 27, 2018 •

edited

miklb commented Jul 27, 2018 •

edited