Implemented a dynamic generation of SSL request certificates #198

Jack12816 · 2017-10-15T18:25:49Z

- What is it good for?

This update ships a fully secure proxy handling of HTTPS requests with
the runtime generation of a certificate authority and the generation of
the request certificates based on the requested domains. A user can then
take care of the importing of the certificate authority. From the
perspective of puffing billy we do not have to handle outdated
certificates or static files anymore in the future with the help of this
feature. This is the same case for users of older puffing billy
versions as well.

The full feature mimics the mighty mitmproxy functionality which results
in secure proxying on modern browsers.

- What I did

I added three new classes, (Authority, Certificate and CertificateChain) with
unit tests for each of them. All static certificate files were removed
(mitm.{crt,key}, test-server.{crt,key}). The ProxyConnection class now makes
use of the new certificate generation. The spec test server uses now
dynamically generated certificates (same like ProxyConnection does it) and
proxy_spec/@https is configured to verify the SSL connection. (By adding our
custom certificate authority file to faraday) And last but not least I
documented the usage of the certificate authority with a Google Chrome Headless
example.

- A picture of a cute animal (not mandatory but encouraged)

This update ships a fully secure proxy handling of HTTPS requests with the runtime generation of a certificate authority and the generation of the request certificates based on the requested domains. A user can then take care of the importing of the certificate authority. From the perspective of puffing billy we do not have to handle outdated certificates or static files anymore in the future with the help of this feature. This is the same case for users of older puffing billy versions as well. The full feature mimics the mighty mitmproxy functionality which results in secure proxying on modern browsers. Signed-off-by: Hermann Mayer <[email protected]>

Jack12816 · 2017-10-22T12:01:28Z

It's now ready for a review. :)

@ronwsmith maybe?

ronwsmith

Pretty great contribution! Mostly minor comments to fix up. Will this break any existing test suites?

ronwsmith · 2017-10-27T03:09:39Z

README.md

+SCRIPT
+```
+
+Mind the reset of the `HOME` environment variable. Furtunately Chrome takes


Typo in Fortunately

ronwsmith · 2017-10-27T03:11:13Z

README.md

+ENV['HOME'] = "#{Dir.tmpdir}/chrome-home-#{Time.now.to_i}"
+
+# Clear and recreate the Chrome home directory.
+FileUtils.rm_rf(ENV['HOME'])


Using Time.now above should pretty much guarantee this directory will never exist right?

We can do the Time.now trick, by then we have a mess while cleaning up. I think of a new configured directory like cache_dir and certs_dir. I also do not like the manipulation of the HOME environment variable. But it's just for child processes. It would be better to have a handling like written below.

ronwsmith · 2017-10-27T03:11:55Z

README.md

+FileUtils.mkdir_p(ENV['HOME'])
+
+# Setup a new pki certificate database for Chrome
+system <<~SCRIPT


Can we create a rake task to do all this?

~~Would be good, yeah.~~ This is not possible right now due to the fact that the rake task does not have access to a puffing billy module. And when it starts it, it would be meaningless, because the user test suite will get a new instance and the authority changes per run. So this would not work out.

But we could package this as a helper on the Billy module. Something which can be used like this:

RSpec.configure do |config| config.before :suite do Billy:.Browsers::Chrome.setup_user_directory end end

We could also include this at the Watir and Capybara driver level. But I like to keep the
Billy:.Browsers::Chrome.setup_user_directory option open for users. (It's also for me, because I configure my drivers by myself, Chrome Headless, you know..)

ronwsmith · 2017-10-27T03:17:26Z

lib/billy/proxy_connection.rb

-        private_key_file: File.expand_path('../mitm.key', __FILE__),
-        cert_chain_file: File.expand_path('../mitm.crt', __FILE__)
-      )
+      start_tls(private_key_file: @key_file, cert_chain_file: @chain_file)


I think we should avoid the use of instance variables. Perhaps the new helper method can return a hash and be used directly as a param in this method call?

Good point.

ronwsmith · 2017-10-27T03:20:32Z

lib/billy/ssl/authority.rb

+    end
+
+    # Write out the private key to file (PEM format) and give back the
+    # file path. This will produce a temporary file which will be remove


Updated the comment.

ronwsmith · 2017-10-27T03:35:52Z

lib/billy/ssl/certificate.rb

+
+    # Give back an appropriate date for the beginning of this
+    # certificate life. We give back now 2 days ago.
+    def valid_from


Maybe these three helper methods move to a proper helper so they aren't copy/pasted in both classes?

Sounds good to me.

ronwsmith · 2017-10-27T03:37:41Z

lib/billy/ssl/certificate.rb

+    # Give back an appropriate date for the beginning of this
+    # certificate life. We give back now 2 days ago.
+    def valid_from
+      Time.now - (2 * 24 * 60 * 60)


Can we just use 2.days.ago?

No ActiveSupport at the gemspec. ;) NoMethodError: undefined method +days+ for 2:Fixnum

ronwsmith · 2017-10-27T03:40:23Z

lib/billy/ssl/certificate.rb

+    # Give back an appropriate date for the end of this certificate life.
+    # We give back now in 2 days.
+    def valid_to
+      Time.now + (2 * 24 * 60 * 60)


Can we just use 2.days.from_now?

No ActiveSupport at the gemspec. ;) NoMethodError: undefined method +days+ for 2:Fixnum

ronwsmith · 2017-10-27T03:42:14Z

lib/billy/ssl/certificate_chain.rb

+
+module Billy
+  # This class is dedicated to the generation of a certificate chain in the
+  # PEM format. Fortunately we just have to concatinate the given certificates


concatenate

ronwsmith · 2017-10-27T03:46:15Z

spec/support/test_server.rb

+      ca = Billy.certificate_authority.cert
+      cert = Billy::Certificate.new(domain)
+      chain = Billy::CertificateChain.new(domain, cert.cert, ca)
+      [chain.file, cert.key_file]


Let's just return a hash and use directly here: http_server.ssl_options = certificate_chain(domain)

Signed-off-by: Hermann Mayer <[email protected]>

Jack12816 · 2017-10-27T07:34:04Z

Will this break any existing test suites?

No, I don't think so for two reasons.

The original mitm.crt is expired since Oct 2 06:42:11 2013 GMT.
So it was not valid from there on and if a browser on a suite was checking this
it was hardly complaining all the time.
The original mitm.crt was never valid on the subject of the domain. In the past
you could allow this certificate (at least until 2013) and all requests which were
signed by it. But modern browsers like Chrome 59+ and Firefox 54+ will not allow
any requests which are signed this certificate.

With this in mind, any user of puffing billy disabled the SSL check (like it was done on our
test suite) or was simply not using SSL with modern browsers. (Phantomjs doesn't count)
So no. No test suite will break from my point of view.

Evidence:

$  openssl x509 -text -noout -in mitm.crt

    Signature Algorithm: sha1WithRSAEncryption
        Issuer: C = US, ST = CA, L = Mountain View, O = Internet Widgits Pty Ltd, CN = Puffing Billy, emailAddress = [email protected]
        Validity
            Not Before: Oct  2 06:42:11 2012 GMT
            Not After : Oct  2 06:42:11 2013 GMT

Jack12816 · 2017-11-07T08:40:28Z

@ronwsmith Ping :)

ronwsmith · 2017-11-10T04:01:16Z

@Jack12816 Looks great now. Awesome contribution, thanks!

ronwsmith · 2017-11-10T04:23:51Z

Released in v0.11.0

Jack12816 force-pushed the handle-ssl-better branch 2 times, most recently from fbea17c to 3390d58 Compare October 22, 2017 10:23

Jack12816 force-pushed the handle-ssl-better branch from 3390d58 to d36cc01 Compare October 22, 2017 11:55

ronwsmith requested changes Oct 27, 2017

View reviewed changes

Reworked the SSL handling while fixing typos and DRYing the code.

951d681

Signed-off-by: Hermann Mayer <[email protected]>

ronwsmith approved these changes Nov 10, 2017

View reviewed changes

ronwsmith merged commit 83e010a into oesmith:master Nov 10, 2017

Jack12816 mentioned this pull request Nov 10, 2017

HTTPS proxying does not work in headless Chrome #193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented a dynamic generation of SSL request certificates #198

Implemented a dynamic generation of SSL request certificates #198

Jack12816 commented Oct 15, 2017 •

edited

Loading

Jack12816 commented Oct 22, 2017

ronwsmith left a comment

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017 •

edited

Loading

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017 •

edited

Loading

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017 •

edited

Loading

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017

ronwsmith Oct 27, 2017

Jack12816 Oct 27, 2017

Jack12816 commented Oct 27, 2017

Jack12816 commented Nov 7, 2017

ronwsmith commented Nov 10, 2017

ronwsmith commented Nov 10, 2017

Implemented a dynamic generation of SSL request certificates #198

Implemented a dynamic generation of SSL request certificates #198

Conversation

Jack12816 commented Oct 15, 2017 • edited Loading

Jack12816 commented Oct 22, 2017

ronwsmith 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

Jack12816 Oct 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jack12816 Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jack12816 Oct 27, 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

Jack12816 commented Oct 27, 2017

Jack12816 commented Nov 7, 2017

ronwsmith commented Nov 10, 2017

ronwsmith commented Nov 10, 2017

Jack12816 commented Oct 15, 2017 •

edited

Loading

Jack12816 Oct 27, 2017 •

edited

Loading

Jack12816 Oct 27, 2017 •

edited

Loading

Jack12816 Oct 27, 2017 •

edited

Loading