Closed Bug 1103196 Opened 10 years ago Closed 8 years ago

Support for untrusted/self-signed TLS certificates

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

(firefox51 wontfix, firefox52 verified, firefox53 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- verified
firefox53 --- verified

People

(Reporter: automatedtester, Assigned: ato)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: pi-marionette-server, pi-marionette-spec, Whiteboard: [marionette=1.0])

Attachments

(9 files, 8 obsolete files)

0001-Bug-1103196-Add-HTTPS-fixture-server-for-Marionette-.patch
41.86 KB, patch
Details | Diff | Splinter Review
0002-Bug-1103196-Mark-specificationLevel-capability-as-pr.patch
1.23 KB, patch
Details | Diff | Splinter Review
0003-Bug-1103196-Logically-reorder-variables-defining-ses.patch
2.26 KB, patch
Details | Diff | Splinter Review
0004-Bug-1103196-Add-acceptInsecureCerts-capability-r-aut.patch
1.73 KB, patch
Details | Diff | Splinter Review
0005-Bug-1103196-Remove-non-conformant-acceptSslCerts-cap.patch
761 bytes, patch
Details | Diff | Splinter Review
0006-Bug-1103196-Add-insecure-certificate-error-r-automat.patch
1.84 KB, patch
Details | Diff | Splinter Review
0007-Bug-1103196-Error-on-encountering-invalid-certificat.patch
3.20 KB, patch
Details | Diff | Splinter Review
0008-Bug-1103196-Add-ability-to-ignore-invalid-TLS-certif.patch
16.25 KB, patch
Details | Diff | Splinter Review
screenshot-2016-12-29-151809.png
117.17 KB, image/png
Details 
We need to be able to handle ssl certs even if self-signed. see spec link
A couple of our current Mozmill tests also rely on that. So marking as a blocker for us.
Blocks: m21s
Whiteboard: [marionette=1.0]
Priority: -- → P1
If someone wanted to jump in and contribute to help resolve this, any recommendations on where to get started?
Summary: Allow Marionette to Handle self-signed certs → Support for untrusted/self-signed SSL certificates
So my alternate "plan" for this in geckodriver is to require the user to provide the CA certificate used to sign their self-signed certificate, and then create a profile with that CA certificate in the certdb. This is similar to the approach we use for web-platform-tests and mochitests [1].

The main practical issue is that we don't ship certutil with firefox and writing rust bindings just to get this to work seems potentially difficult or problematic. I'm not sure how to deal with that, really.

ted: you are often full of good ideas on this kind of problem… do you have any suggestions this time?


[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/harness/wptrunner/browsers/firefox.py#256
Flags: needinfo?(ted)
For compatibility with other implementations and with the WebDriver specification, we need to support untrusted/self-signed certificates by default.  Asking users to provide the self-signed certificate as part of the profile is a reasonable workaround right now, but not a viable long-term option.
OK, it seems like my alternate plan is not worth implementing.
Flags: needinfo?(ted)
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76754/#review74832

Andreas, thank you for the sneak preview of those changes. There are indeed a lot of them. I will just try to import your commit into my history, to get started with bug 1300551. Btw, I made some inline comments about things I noticed.

::: testing/marionette/harness/marionette/runner/base.py:852
(Diff revision 1)
>          if self.marionette.instance and self.emulator:
>              try:
>                  device_info = self.marionette.instance.runner.device.dm.getInfo()
> -            except Exception:
> -                self.logger.warning('Could not get device info.')
> +            except Exception as e:
> +                self.logger.warning("could not get device info")
> +                self.logger.warning(e)

You can add 'exc_info=True' to self.logger.warning() with the same result. With the traceback included it might even be better to find the causing line of code.

::: testing/marionette/harness/marionette/runner/httpd.py:38
(Diff revision 1)
> +    def __init__(self, doc_root, url="http://127.0.0.1:0", use_ssl=False,
> +                 ssl_cert=None, ssl_key=None):
> +        if not os.path.isdir(doc_root):
> +            raise ValueError("Server root is not a valid path: %s" % doc_root)
> +
> +        url = urlparse.urlparse(url)

why not: 'scheme, hostname, port = urlparse()'

::: testing/marionette/harness/marionette/runner/httpd.py:111
(Diff revision 1)
> +
> +    httpd = FixtureServer(args.doc_root, args.url,
> +                          ssl_cert=args.ssl_cert,
> +                          ssl_key=args.ssl_key)
> +    httpd.start()
> +    print >>sys.stderr, "%s: info: started fixture server on %s" % \

why stderr and not stdout?

::: testing/marionette/harness/marionette/runner/serve.py:158
(Diff revision 1)
> +    _, server = item
> +    return server.get_url(uri)
> +
> +
> +def info(msg):
> +    print >>sys.stderr, "%s: info: %s" % (sys.argv[0], msg)

stdout too?
FYI there is no point in me-too type comments to this issue. At present it is known that all versions of Firefox/GeckoDriver are affected by this. The way to find out when that will change is to follow this bug.
Not really useful towards fixing this bug, but my current workaround was to make a small python lib/command for generating cert_override.txt files: https://github.com/Osmose/firefox-cert-override

You can use it to generate cert_override.txt in your profile directory that you're using for testing, and whitelist specific certificates for specific domains: https://gist.github.com/Osmose/e7e4f93008335e2e56ebe95100a78f4c

Looking forward to a fix! :D
I'm interested in picking this up (esp. to play more with Rust), but I'm not sure where to begin. Is it possible to have a mentor on this bug who is familiar with geckodriver?
This requires a change in Marionette, which is written in JS.  It’s only the geckodriver HTTP frontend that is in Rust.

Anyway, I’ve started working on this but forgot to assign the bug to myself.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Summary: Support for untrusted/self-signed SSL certificates → Support for untrusted/self-signed TLS certificates
As a point of curiosity, what is the progress on this particular issue? If you need assistance, I would be happy to lend a hand. Ran in to this issue yesterday as I began working on updating our internal automation project to Selenium 3.0.1 and geckodriver. Since all of our products create a self-signed certificate upon first boot, this is a bit of a blocker. For the moment, we're holding steady at testing against Firefox 46 with the FirefoxDriver bundled with Selenium 2.53.
This work is in progress. Unfortunately I don't think it's Good First Bug material as it requires poking at the deep browser internals to bypass the normal security checks. Please don't add comments to the bug indicating that you need this feature; we are aware that it is very important to many users.
I have a working prototype of self-signed TLS certificates, but it requires a bit of work to make it decent.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review90984
Attachment #8788174 - Flags: review?(dburns) → review+
Comment on attachment 8808296 [details]
Bug 1103196 - Mark specificationLevel capability as proprietary;

https://reviewboard.mozilla.org/r/91106/#review90986
Attachment #8808296 - Flags: review?(dburns) → review+
Comment on attachment 8808297 [details]
Bug 1103196 - Logically reorder variables defining session state;

https://reviewboard.mozilla.org/r/91108/#review90990
Attachment #8808297 - Flags: review?(dburns) → review+
Comment on attachment 8808298 [details]
Bug 1103196 - Add acceptInsecureCerts capability;

https://reviewboard.mozilla.org/r/91110/#review90992
Attachment #8808298 - Flags: review?(dburns) → review+
Comment on attachment 8808299 [details]
Bug 1103196 - Remove non-conformant acceptSslCerts capability;

https://reviewboard.mozilla.org/r/91112/#review91000

::: testing/marionette/driver.js
(Diff revision 2)
>      "acceptInsecureCerts": !this.secureTLS,
>  
>      // supported features
>      "raisesAccessibilityExceptions": false,
>      "rotatable": this.appName == "B2G",
> -    "acceptSslCerts": false,

We need to get the Selenium project up to date for this
Attachment #8808299 - Flags: review?(dburns) → review+
Comment on attachment 8808300 [details]
Bug 1103196 - Add insecure certificate error;

https://reviewboard.mozilla.org/r/91114/#review91002
Attachment #8808300 - Flags: review?(dburns) → review+
Comment on attachment 8808301 [details]
Bug 1103196 - Error on encountering invalid certificate;

https://reviewboard.mozilla.org/r/91116/#review91006
Attachment #8808301 - Flags: review?(dburns) → review+
Comment on attachment 8808302 [details]
Bug 1103196 - Add ability to ignore invalid TLS certificates;

https://reviewboard.mozilla.org/r/91118/#review91206
Attachment #8808302 - Flags: review?(dburns) → review+
Should this have any extra review for security or from someone familiar with nsICertOverrideService (assuming, perhaps incorrectly, AutomatedTester isn't)?
(In reply to James Graham [:jgraham] from comment #45)
> Should this have any extra review for security or from someone familiar with
> nsICertOverrideService (assuming, perhaps incorrectly, AutomatedTester
> isn't)?

Might be a good idea. I assume that we can ask David Keeler (:keeler) who owns the security manager.
(In reply to James Graham [:jgraham] from comment #45)
> Should this have any extra review for security or from someone familiar with
> nsICertOverrideService (assuming, perhaps incorrectly, AutomatedTester
> isn't)?

In principle this isn’t necessary as Marionette is guarded behind a flag, but it’s a good idea.  I will flag keeler for the final patch in the push.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

Those changes are complex! And I find it hard to step through due to all those extra added hierarchy levels. So please bare with me if some of my comments may not be accurate. An explanation would help a lot in those cases.

Otherwise what I really would like to see is a try build for the current code. It would be great if you could trigger one through mozreview across all platforms.

::: testing/marionette/harness/marionette/marionette_test/testcases.py
(Diff revision 3)
>          # duration of the test; this is deleted in tearDown() to prevent
>          # a persistent circular reference which in turn would prevent
>          # proper garbage collection.
>          self.start_time = time.time()
>          self.marionette = self._marionette_weakref()
> -        self.httpd = self._httpd_weakref()

Was this line lost during a rebase? Or don't we need it here because no weakref is necessary? I ask because the implementation was identical to Marionette and that part is still in the code.

::: testing/marionette/harness/marionette/marionette_test/testcases.py:431
(Diff revision 3)
>      match_re = re.compile(r"test_(.*)\.py$")
>  
> -    def __init__(self, marionette_weakref, httpd_weakref, methodName='runTest',
> +    def __init__(self, marionette_weakref, fixtures, methodName='runTest',
>                   filepath='', **kwargs):
>          self._marionette_weakref = marionette_weakref
> -        self._httpd_weakref = httpd_weakref
> +        self.fixtures = fixtures

We make Marionette available in the CommonTestCase. Why do you think we don't have to do the same for fixtures?

::: testing/marionette/harness/marionette/runner/base.py:20
(Diff revision 3)
> -import mozprofile
>  
> -
> -from manifestparser import TestManifest
>  from manifestparser.filters import tags
> +from manifestparser import TestManifest

Why this move? Shouldn't submodules be listed after the general import? Whereby this whole section would need an update. All imports are mixed up in not a PEP8 proposed order. So it's fine and we can fix that later.

::: testing/marionette/harness/marionette/runner/base.py
(Diff revision 3)
> -
> -    @bin.setter
> -    def bin(self, path):
> -        """Set binary and reset parts of runner accordingly.
> -
> -        Intended use: to change binary between calls to run_tests

Why has this been removed? I do not see a relation to the fixtures changes and this will break the update tests.

::: testing/marionette/harness/marionette/runner/base.py
(Diff revision 3)
> -        if not self.marionette:
> -            self.marionette = self.driverclass(**self._build_kwargs())
> -            # if we're working against a desktop version, we usually don't need
> -            # an external ip
> -            if self.appName != 'fennec':
> -                need_external_ip = False

I do not see that those lines specifically for Fennec have been kept. Or are we going to not obey Fennec support at all now?

::: testing/marionette/harness/marionette/runner/base.py
(Diff revision 3)
> -            # if we're working against a desktop version, we usually don't need
> -            # an external ip
> -            if self.appName != 'fennec':
> -                need_external_ip = False
> -        self.logger.info('Initial Profile Destination is '
> -                         '"{}"'.format(self.marionette.profile_path))

Can we please keep this log line? It was a request which hasn't been implemented that long ago. Thanks.

::: testing/marionette/harness/marionette/runner/httpd.py:38
(Diff revision 3)
> +@handlers.handler
> +def slow_loading_document(request, response):
> +    time.sleep(5)
> +    return """<!doctype html>
> +<title>ok</title>
> +<p>ok"""

Now that we expose the local server to the testcase, can you please file a new bug so we can get those handlers moved out of the core code and placed into the appropriate tests?

::: testing/marionette/harness/marionette/runner/httpd.py:69
(Diff revision 3)
> +                                          bind_hostname=True,
> +                                          doc_root=doc_root,
> -            routes=routes,
> +                                          routes=routes,
> -            host=self.host,
> -        )
> -        self._server.start(block=block)
> +                                          use_ssl=True if scheme == "https" else False,
> +                                          certificate=ssl_cert,
> +                                          key_file=ssl_key)

Don't we have to fallback to our default certificate in case of HTTPS? With the above both the certificate and the key file will be None in case when the class is used via the API and not CLI.

::: testing/marionette/harness/marionette/runner/httpd.py:80
(Diff revision 3)
>  
> -    @property
> -    def alive(self):
> -        return self._server is not None
> +    def wait(self):
> +        if not self.is_alive:
> +            return
> +        try:
> +            select.select([], [], [])

Please keep the following in mind. Maybe you haven't tested this yet on Windows?

https://docs.python.org/2/library/select.html#select.select

> Empty sequences are allowed, but acceptance of three empty sequences is platform-dependent. (It is known to work on Unix but not on Windows.)

::: testing/marionette/harness/marionette/runner/httpd.py:96
(Diff revision 3)
> -            raise Exception("Server not started")
> -        return self._server.get_url(path)
> +            raise Exception("Server is not alive")
> +        return self._httpd.get_url(path)
>  
>      @property
>      def router(self):
>          return self._server.router

self._httpd

::: testing/marionette/harness/marionette/runner/httpd.py:114
(Diff revision 3)
> -    doc_root = os.path.join(os.path.dirname(here), "www")
> -    httpd = FixtureServer(doc_root, port=2829)
> -    print "Started fixture server on http://{0}:{1}/".format(httpd.host, httpd.port)
> -    try:
> -        httpd.start(True)
> -    except KeyboardInterrupt:
> +        description="Specialised HTTP server for testing Marionette.")
> +    parser.add_argument("url", help="""
> +service address including scheme, hostname, port, and prefix for document root,
> +e.g. \"https://0.0.0.0:0/base/\"""")
> +    parser.add_argument(
> +        "-r", action="store", dest="doc_root", default=default_doc_root,

`action=store` is the default. So you don't need it here for each argument.

::: testing/marionette/harness/marionette/runner/httpd.py:115
(Diff revision 3)
> -    httpd = FixtureServer(doc_root, port=2829)
> -    print "Started fixture server on http://{0}:{1}/".format(httpd.host, httpd.port)
> -    try:
> -        httpd.start(True)
> -    except KeyboardInterrupt:
> -        pass
> +    parser.add_argument("url", help="""
> +service address including scheme, hostname, port, and prefix for document root,
> +e.g. \"https://0.0.0.0:0/base/\"""")
> +    parser.add_argument(
> +        "-r", action="store", dest="doc_root", default=default_doc_root,
> +        help="path to document root (default %s)" % default_doc_root)

You can use `%(default)s` for that which will automatically replace it with the default value.

Also please do not use `%` anymore for formatting. Instead make use of `.format()` across all the code.

::: testing/marionette/harness/marionette/runner/serve.py:65
(Diff revision 3)
> +
> +class ServerProxy(multiprocessing.Process, BlockingChannel):
> +
> +    def __init__(self, channel, init_func, *init_args, **init_kwargs):
> +        multiprocessing.Process.__init__(self)
> +        BlockingChannel.__init__(self, channel)

Please make use of `super(ServerProxy, self).__init__()` here which will automatically take care of calling the methods in the correct order as listed in the MRO.

::: testing/marionette/harness/marionette/runner/serve.py:92
(Diff revision 3)
> +    def __init__(self, init_func):
> +        self._init_func = init_func
> +        self.proc = None
> +
> +        parent_chan, self.child_chan = multiprocessing.Pipe()
> +        BlockingChannel.__init__(self, parent_chan)

Same here regarding `super()`.

::: testing/marionette/harness/marionette/runner/serve.py:117
(Diff revision 3)
> +        self.proc.terminate()
> +        self.proc.join(0)
> +
> +    @property
> +    def is_alive(self):
> +        return self.proc.is_alive()

`self.proc` could be `None` here.

::: testing/marionette/harness/marionette/runner/serve.py:133
(Diff revision 3)
> +                               ssl_cert=ssl_config["cert_path"])
> +
> +
> +def start_servers(doc_root, ssl_config, **kwargs):
> +    servers = defaultdict()
> +    for domain, builder_fn in registered_servers:

domain vs schema?

::: testing/marionette/harness/marionette/runner/serve.py:141
(Diff revision 3)
> +        servers[domain] = (proc.get_url("/"), proc)
> +    return servers
> +
> +
> +def start(doc_root=None, **kwargs):
> +    """Start all relevant test servers on the given document root, or

FYI the first line should give a brief overview and should not be a multi-line string.

::: testing/marionette/harness/marionette/runner/serve.py:146
(Diff revision 3)
> +    """Start all relevant test servers on the given document root, or
> +    otherwise using the default testing/marionette/harness/marionette/www
> +    folder.
> +
> +    Additional keyword arguments can be given which will be passed on
> +    to the individual ``FixtureServer``'s in httpd.py.

Will tests be able to spawn new fixture servers eg. when other doc_root's are required, or can those be added to the existing one? It seems to me that only a single server per schema is currently allowed.

::: testing/marionette/harness/marionette/runner/serve.py:162
(Diff revision 3)
> +
> +
> +def where_is(uri, on="http"):
> +    """Returns the full URL, including scheme, hostname, and port, for
> +    a fixture resource from the server associated with the ``on`` key.
> +    It will by default look for th eresource in the "http" server.

nit: spelling mistake.

::: testing/marionette/harness/marionette/runner/serve.py:168
(Diff revision 3)
> +
> +    """
> +    item = servers.get(on)
> +    if item is None:
> +        raise ValueError("Unable to find server associated with key: %s" % on)
> +    _, server = item

You don't want that the get() method already raises a ValueError? This should really be better than returning None.

Then those 4 lines would also become a single one.

::: testing/marionette/harness/marionette/runner/serve.py:189
(Diff revision 3)
> +                      ("https", https_server)]
> +servers = defaultdict()
> +
> +
> +def main(args):
> +    global servers

So do we need both cli for httpd.py and serve.py?
Attachment #8788174 - Flags: review?(hskupin) → review-
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91288

Oh, and what I missed. Can we get at least a simple test which makes use of the HTTPS fixture server? It would fit into test_httpd.py.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> Was this line lost during a rebase? Or don't we need it here because no weakref is necessary? I ask because the implementation was identical to Marionette and that part is still in the code.

I don’t see why we need a weakref.  `fixtures` is a reference to the `Fixtures` class which is essentially a wrapper for the library function `serve.where_is`.  I.e. it does not hold any state.

> We make Marionette available in the CommonTestCase. Why do you think we don't have to do the same for fixtures?

What do you mean?

> Why this move? Shouldn't submodules be listed after the general import? Whereby this whole section would need an update. All imports are mixed up in not a PEP8 proposed order. So it's fine and we can fix that later.

Fixed with isort.

> I do not see that those lines specifically for Fennec have been kept. Or are we going to not obey Fennec support at all now?

I don’t think we need this complexity.  It is not used right now and we can change the default to bind to a public-facing interface when the need arises by changing "127.0.0.1" to "0.0.0.0" in httpd.py.

> Can we please keep this log line? It was a request which hasn't been implemented that long ago. Thanks.

I moved it to `BaseMarionetteTestRunner.run_tests`.

> Now that we expose the local server to the testcase, can you please file a new bug so we can get those handlers moved out of the core code and placed into the appropriate tests?

I don’t think we expose the wptserve handler registrar, so this would require further work.

> Don't we have to fallback to our default certificate in case of HTTPS? With the above both the certificate and the key file will be None in case when the class is used via the API and not CLI.

You need to specify the key and certificate explicitly when constructing `httpd.FixtureServer`.  The `certificate` and `key_file` keyword arguments are provided by `serve.https_server`.

> Please keep the following in mind. Maybe you haven't tested this yet on Windows?
> 
> https://docs.python.org/2/library/select.html#select.select
> 
> > Empty sequences are allowed, but acceptance of three empty sequences is platform-dependent. (It is known to work on Unix but not on Windows.)

We will see soon when the try run is complete.  Unfortunately I don’t have any great ideas for how to fix this.  `httpd.wait` is currently only used when spinning it up through the command line so it might be that we can live with this bug.

> Please make use of `super(ServerProxy, self).__init__()` here which will automatically take care of calling the methods in the correct order as listed in the MRO.

I don’t think this allows me to pass an argument to `BlockingChannel`’s ctor?

> Will tests be able to spawn new fixture servers eg. when other doc_root's are required, or can those be added to the existing one? It seems to me that only a single server per schema is currently allowed.

While it is true that you can only have one unique identifier for each registered server (see `registered_servers` in serve.py), it is possible for tests to call `httpd.FixtureServer` directly with a custom root.  Servers spawned by tests will not have their lifetime controlled by the `serve` module.

> So do we need both cli for httpd.py and serve.py?

It is convenient for testing.
Comment on attachment 8808302 [details]
Bug 1103196 - Add ability to ignore invalid TLS certificates;

https://reviewboard.mozilla.org/r/91118/#review91384

This is more of a comment on the specification, but I don't think this is the best approach. If my understanding is correct, the goal here is to programmatically enable the browser to connect to a site over https in a test scenario, meaning that the site may not have the "correct" certificate (so either the connection is going through an intercepting proxy or the site is just pretending to be the real one, etc.). It seems the important properties here are that a) the browser doesn't terminate the connection but also that b) other checks and indicators in the browser work as similarly as possible to the normal scenario of connecting to the real site. This implementation (and the way the specification is framed) will achieve a) but not b) as much (as you've already discovered by having to disable pinning and HSTS preload checks). Additionally, however, this setup will affect the latency on the TLS handshake (there's a bit more shuttling back and forth between threads in the certificate error override case) as well as the front-end UI (if there's a user-added override on the connection, the lock icon and the control center change a bit).

An approach that I believe would achieve both goals would be to introduce an API or property that essentially says "add this certificate as a trust anchor". Then, what you would do is generate a CA certificate in addition to your server certificate (if you use the same key for both, they can still be self-signed) and use that CA as the trust anchor. Firefox already overrides pinning checks for certificates issued by imported trust anchors, so you wouldn't have to disable pinning. HSTS would just work, the TLS handshake wouldn't have the additional latency, and the UI would behave normally as well.

To implement this, you can use `nsIX509CertDB.addCertFromBase64(<base 64>, <trust string (probably "CTu,,")>, <unused argument>);` See https://dxr.mozilla.org/mozilla-central/rev/f13e90d496cf1bc6dfc4fd398da33e4afe785bde/security/manager/ssl/tests/unit/head_psm.js#139 and related code for some examples.

::: testing/marionette/cert.js:132
(Diff revision 5)
> +  return {
> +    register: function() {
> +      // make it possible to register certificate overrides for domains
> +      // that use HSTS or HPKP
> +      let offsetSeconds = 19 * 7 * 24 * 60 * 60;
> +      Preferences.set(TIME_OFFSET_PREF, offsetSeconds);

You can just disable the HSTS preload list by setting "network.stricttransportsecurity.preloadlist" to false. Note that neither pref will have an effect on HSTS sites collected via the header.
Attachment #8808302 - Flags: review?(dkeeler)
Attachment #8808302 - Flags: review?(mconley) → review?(dtownsend)
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> I don’t see why we need a weakref.  `fixtures` is a reference to the `Fixtures` class which is essentially a wrapper for the library function `serve.where_is`.  I.e. it does not hold any state.

Ok, maybe I should have checked the full patch first before doing that comment. Another point to account here is that we do not have a cycle reference as with Marionette. So this is indeed not necessary.

> What do you mean?

Classes which subclass CommonTestCase will have the `marionette` property. I think that it might be better to have the fixtures property there too instead of the MarionetteTestCase class.

> I don’t think we need this complexity.  It is not used right now and we can change the default to bind to a public-facing interface when the need arises by changing "127.0.0.1" to "0.0.0.0" in httpd.py.

Given that David agreed on this I think we can drop this issue then.

> I moved it to `BaseMarionetteTestRunner.run_tests`.

Thanks.

> I don’t think we expose the wptserve handler registrar, so this would require further work.

We do. Please have a look at the test_httpd.py unit test. We do the registration of handlers via `self.httpd.router.register()`. This is a requirement for bringing up the telemetry tests which need custom handlers.

> You need to specify the key and certificate explicitly when constructing `httpd.FixtureServer`.  The `certificate` and `key_file` keyword arguments are provided by `serve.https_server`.

So how is that done when a test needs a specific certificate and key? Does it have to create its own instance of `FixtureServer` which only lives in the scope of the test? If yes, maybe a similar approach should be done for telemetry and we should update test_httpd.py to give an impression how to use it.

> I don’t think this allows me to pass an argument to `BlockingChannel`’s ctor?

You can do, but you have to re-order the list of base classes for ServerProxy so that BlockingChannel comes first in the MRO. Which would also make sense given that you are adding additional features with this mixin. You will also need that order to correctly call both __init__ methods. Here an example of how it will look like: https://gist.github.com/whimboo/d94eb7c42e0b4e2940c64e4e81f7909c

> While it is true that you can only have one unique identifier for each registered server (see `registered_servers` in serve.py), it is possible for tests to call `httpd.FixtureServer` directly with a custom root.  Servers spawned by tests will not have their lifetime controlled by the `serve` module.

I think that this answers my question from above. So passing the original reference to the testcase might not be necessary then.
Attachment #8808302 - Flags: review?(dtownsend)
Comment on attachment 8808302 [details]
Bug 1103196 - Add ability to ignore invalid TLS certificates;

https://reviewboard.mozilla.org/r/91118/#review91762

::: testing/marionette/cert.js:82
(Diff revision 5)
> + *
> + * @param {cert.Override} service
> + *     Service to uninstall.
> + */
> +cert.uninstallOverride = function(service) {
> +  if (!cert.currentOverride) {

You should probably check that cert.currentOverride == service. Or just not pass in the service at all and make this unregister currentOverride
Attachment #8808302 - Flags: review+
Hi,

I am looking forward to use selenium3.0.1 with firefox latest version(presently 49) but since my application involves accepting ssl certificates, I am using below code to achieve the same but it's not working :

System.setProperty("webdriver.gecko.driver", "Some path\\geckodriver.exe");
			FirefoxProfile profile = new FirefoxProfile();
			profile.setAcceptUntrustedCertificates(true); 
			profile.setAssumeUntrustedCertificateIssuer(true);
			return new FirefoxDriver(profile);
Kindly update me when this bug will be resolved so that I can use Selenium 3 with FF 49
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> Classes which subclass CommonTestCase will have the `marionette` property. I think that it might be better to have the fixtures property there too instead of the MarionetteTestCase class.

As far as I can tell [CommonTestCase’s ctor](https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/harness/marionette/marionette_test/testcases.py#L77) doesn’t take a `marionette` argument, and its [`add_tests_to_suite`](https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/harness/marionette/marionette_test/testcases.py#L227) function raises `NotImplementedError`, so it can’t be added there.

> We do. Please have a look at the test_httpd.py unit test. We do the registration of handlers via `self.httpd.router.register()`. This is a requirement for bringing up the telemetry tests which need custom handlers.

What I mean is that the tests themselves don’t have access to `self.httpd`.  They have access to `self.fixtures`, which calls the library function `serve.where_is`.

> So how is that done when a test needs a specific certificate and key? Does it have to create its own instance of `FixtureServer` which only lives in the scope of the test? If yes, maybe a similar approach should be done for telemetry and we should update test_httpd.py to give an impression how to use it.

That would be how you would need to do it unless you set up a HTTPD with a custom cert in serve.py.

> You can do, but you have to re-order the list of base classes for ServerProxy so that BlockingChannel comes first in the MRO. Which would also make sense given that you are adding additional features with this mixin. You will also need that order to correctly call both __init__ methods. Here an example of how it will look like: https://gist.github.com/whimboo/d94eb7c42e0b4e2940c64e4e81f7909c

I don’t think that solution is clearer than what is there now, where the arguments for what constructor is called out explicitly.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91288

Added.
Attachment #8808302 - Flags: review?(mconley)
Comment on attachment 8808302 [details]
Bug 1103196 - Add ability to ignore invalid TLS certificates;

https://reviewboard.mozilla.org/r/91118/#review91762

> You should probably check that cert.currentOverride == service. Or just not pass in the service at all and make this unregister currentOverride

Good plan.  I think I’ll remove the `service` argument altogether to avoid confusion.

Thanks for the review!
Comment on attachment 8808302 [details]
Bug 1103196 - Add ability to ignore invalid TLS certificates;

https://reviewboard.mozilla.org/r/91118/#review91384

David, thanks ever so much for your detailed review of this and suggestions!

Due to the unfortunate nature of the WebDriver spec we don’t know upfront what certificate a site may have.  Instead of sane user API whereby the user is required to install the known-bad cert into the root store before navigation, the specification mandates a carte blanche capability `{acceptInsecureCerts: true}` that is meant cause the browser to accept _all upcoming_ insecure certs.

The idea is to _implicitly_ disregard self-signed or invalid certs when loading the document as well as potential certs in linked resources in the loaded documented which also may have simiarily invalid, but not necessarily the same, certs.  In other words, there is a cascade problem here.

As far as I can tell `nsIX509CertDB.addCertFromBase64` requires you to know the certificate of the site you want to connect to, but the input to the Marionette command that causes navigation is unknown and the user of the API may not have access to the certificate of the site.

Is there another way to use `nsIX509CertDB` so that it adds the cert _upon load_?
Flags: needinfo?(dkeeler)
For DevTools, I handled a similar situation by making a "test" connection, and if it fails, grab the cert from the test connection, store it, and try again.

http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/devtools/shared/security/socket.js#152

Not sure if this works for your case.
(In reply to Andreas Tolfsen ‹:ato› from comment #76)
> Due to the unfortunate nature of the WebDriver spec we don’t know upfront
> what certificate a site may have.  Instead of sane user API whereby the user
> is required to install the known-bad cert into the root store before
> navigation, the specification mandates a carte blanche capability
> `{acceptInsecureCerts: true}` that is meant cause the browser to accept _all
> upcoming_ insecure certs.

Sounds like the specification should maybe be improved. The link ( https://w3c.github.io/webdriver/webdriver-spec.html ) indicates it's a draft and not yet final, so I imagine it would be easier to address now rather than later.

> The idea is to _implicitly_ disregard self-signed or invalid certs when
> loading the document as well as potential certs in linked resources in the
> loaded documented which also may have simiarily invalid, but not necessarily
> the same, certs.  In other words, there is a cascade problem here.
> 
> As far as I can tell `nsIX509CertDB.addCertFromBase64` requires you to know
> the certificate of the site you want to connect to, but the input to the
> Marionette command that causes navigation is unknown and the user of the API
> may not have access to the certificate of the site.

The easiest way to do this is to use a common root certificate to issue each server certificate. If the browser trusts that root, it will accept each server certificate (and again, the easiest way to do this is to use the same key each time).

> Is there another way to use `nsIX509CertDB` so that it adds the cert _upon
> load_?

Like :jryans said, you can make a test connection and grab the certificate. If the server doesn't send an issuing certificate (and only sends the end-entity certificate), that won't work, though.
Flags: needinfo?(dkeeler)
FYI as a user, if I wanted (and could) create each server with some common root and ultimately do some sane ssl setup, then the insecure option wouldn't be needed.
I think these should be separate concerns - insecure browsing to accept all **** (even if some subtle semantics is broken) vs trusting some additional CAs to test proper SSL. I think driver spec has the option exactly because a lot of time the additional effort to setup proper SSL is too high.
Having a solution that doesn't work for all use cases would also be frustrating.
While I'd like to live in a perfect world, most people choose the easiest route and I hope that firefox would be one of the easiest and most robust browsers to test with.
(In reply to Alexander from comment #87)
> FYI as a user, if I wanted (and could) create each server with some common
> root and ultimately do some sane ssl setup, then the insecure option
> wouldn't be needed.

If I understand you correctly, the idea of a wildcard whitelist to configure what servers to ignore invalid/self-signed TLS certs for was rejected by the W3C WebDriver WG.
Andreas, I didn't say this. Maybe it's too late here now. I'll look through older comments on fresh head tomorrow and try to understand how my answer could be understood like this. Maybe I missed something.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #86)
> Sounds like the specification should maybe be improved. The link (
> https://w3c.github.io/webdriver/webdriver-spec.html ) indicates it's a draft
> and not yet final, so I imagine it would be easier to address now rather
> than later.

With the current time schedule the spec is meant to go into CR in late January.  The window to make breaking changes, due to existing implementations, I’m afraid is narrowing.

The full discussion around acceptInsecureCerts can be found in https://www.w3.org/2016/09/19-webdriver-minutes.html.

> > The idea is to _implicitly_ disregard self-signed or invalid certs when
> > loading the document as well as potential certs in linked resources in the
> > loaded documented which also may have simiarily invalid, but not necessarily
> > the same, certs.  In other words, there is a cascade problem here.
> > 
> > As far as I can tell `nsIX509CertDB.addCertFromBase64` requires you to know
> > the certificate of the site you want to connect to, but the input to the
> > Marionette command that causes navigation is unknown and the user of the API
> > may not have access to the certificate of the site.
> 
> The easiest way to do this is to use a common root certificate to issue each
> server certificate. If the browser trusts that root, it will accept each
> server certificate (and again, the easiest way to do this is to use the same
> key each time).

The arguments from the WG participants is that users of WebDriver typically don’t have access to the server environment they’re testing.  Whilst I agree it would be the technically superior alternative to provide the cert or a root cert as a configuration option, we have lost this fight.

> > Is there another way to use `nsIX509CertDB` so that it adds the cert _upon
> > load_?
> 
> Like :jryans said, you can make a test connection and grab the certificate.
> If the server doesn't send an issuing certificate (and only sends the
> end-entity certificate), that won't work, though.

At the moment I can’t see a better way to implement the spec than using `nsICertOverride`.  Do you have an opinion whether making a test connection and grabbing the certificates, then adding it to `nsIX509CertDB` is better or worse than this?
Flags: needinfo?(dkeeler)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #85)
> For DevTools, I handled a similar situation by making a "test" connection,
> and if it fails, grab the cert from the test connection, store it, and try
> again.

Would this work if the site you add the certificate for contains links to other resources that have other invalid or self-signed certificates?
Flags: needinfo?(jryans)
Comment on attachment 8808302 [details]
Bug 1103196 - Add ability to ignore invalid TLS certificates;

https://reviewboard.mozilla.org/r/91118/#review92386

I think the spec issues are unfortunate, but I don't really have the bandwidth to join a working group and bring everyone around to my view, so I guess this is how it's going to be. This implementation should work (with the caveats I mentioned in comment 61). Making a test request to obtain an issuing certificate to trust won't always work because the server won't always send an issuing certificate, and you would have to do this for each separate domain, which isn't great. I didn't review the python code since I don't really have the background for that.

::: testing/marionette/cert.js:12
(Diff revision 7)
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +this.EXPORTED_SYMBOLS = ["cert"];

I wouldn't call this "cert" - maybe "insecureCertOverride" or something?

::: testing/marionette/cert.js:18
(Diff revision 7)
> +
> +const registrar =
> +    Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
> +
> +const CONTRACT_ID = "@mozilla.org/security/certoverride;1";
> +const TIME_OFFSET_PREF = "test.currentTimeOffsetSeconds";

Use "network.stricttransportsecurity.preloadlist" instead (set it to false to disable the HSTS preload list).

To be more thorough, you can clear everything the site security service knows about:

`
let sss = Cc["@mozilla.org/ssservice;1"]
            .getService(Ci.nsISiteSecurityService);
sss.clearAll();
sss.clearPreloads();
`

(this will clear collected HSTS and HPKP state)

::: testing/marionette/cert.js:66
(Diff revision 7)
> +  service.register();
> +  cert.currentOverride = service;
> +
> +  // reinitialise service
> +  Cc["@mozilla.org/security/certoverride;1"]
> +      .getService(Ci.nsICertOverrideService);

I'm surprised this is necessary.

::: testing/marionette/cert.js:102
(Diff revision 7)
> + *     Precise set of certificate errors to override.
> + *
> + * @throws {Components.Exception}
> + *     If there are any problems registering the service.
> + */
> +cert.SweepingOverride = function(matching) {

This mechanism won't work unless you pass all of the bits `Error.Untrusted | Error.Mismatch | Error.Time`, so I wouldn't even make it optional - just hard-code it (this is a consequence of how the back-end matches stored overrides to certificate errors it encounters).
Attachment #8808302 - Flags: review?(dkeeler) → review+
(In reply to Andreas Tolfsen ‹:ato› from comment #91)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #85)
> > For DevTools, I handled a similar situation by making a "test" connection,
> > and if it fails, grab the cert from the test connection, store it, and try
> > again.
> 
> Would this work if the site you add the certificate for contains links to
> other resources that have other invalid or self-signed certificates?

I've only needed to use this process for a single top-level connection before, so I am not sure what would happen with a bad cert for a subresource of a page.  You're only going to be able to store the top-level cert up front, so it would seem that with this approach, you could get cert errors for subresources.  I am not sure how we currently handle that case (do the subresources not load at all, etc.), so let's check with :keeler.
Flags: needinfo?(jryans) → needinfo?(dkeeler)
Right - for that approach, you would have to intercept every first connection to a new domain. In general, if a subresource has a certificate error, we silently block it.
Flags: needinfo?(dkeeler)
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> As far as I can tell [CommonTestCase’s ctor](https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/harness/marionette/marionette_test/testcases.py#L77) doesn’t take a `marionette` argument, and its [`add_tests_to_suite`](https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/harness/marionette/marionette_test/testcases.py#L227) function raises `NotImplementedError`, so it can’t be added there.

Sorry for being not that precise. I actually meant `setUp()` here, which handles the weakref of Marionette. I haven't checked the implementation in detail yet, but maybe it's old and no-longer used code.

> What I mean is that the tests themselves don’t have access to `self.httpd`.  They have access to `self.fixtures`, which calls the library function `serve.where_is`.

As long as we come to a conclusion that one of the two things will work I'm fine:

1) Tests can use the global fixture server and attach a handler
2) Tests can instantiate their own fixture server for handlers (which will not be via the default port)

> We will see soon when the try run is complete.  Unfortunately I don’t have any great ideas for how to fix this.  `httpd.wait` is currently only used when spinning it up through the command line so it might be that we can live with this bug.

Sounds good. I assume we even won't have it tested and cannot see the results on try.

> I don’t think that solution is clearer than what is there now, where the arguments for what constructor is called out explicitly.

It's not about that it is clearer in terms of reading the code. It's more about using mixins correctly by obeying the MRO. As such the current code will add obstacles to other classes who want to do it correctly and want to base on the affected proxy class. We could live with this for now, and might have to change it later. Up to you to fix it or not.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review92774

::: testing/marionette/harness/marionette/marionette_test/testcases.py:481
(Diff revisions 4 - 6)
>          # In the case no session is active (eg. the application was quit), start
>          # a new session for clean-up steps.
>          if not self.marionette.session:
>              self.marionette.start_session()
>  
> -        if not self.marionette.check_for_crash():
> +        if not self.marionette.crashed:

This is unrelated code and doesn't belong to the changes you do for a fixture server. Please try to stick to those changes only which are related to it. This is similar to other changes in this file.

::: testing/marionette/harness/marionette/runner/base.py:294
(Diff revisions 4 - 6)
>                            dest='prefs_files',
>                            help="read preferences from a JSON or INI file. For INI, use "
>                                 "'file.ini:section' to specify a particular section.")
>          self.add_argument('--addon',
>                            action='append',
> +                          dest='addons',

This was already done by another bug, and is unrelated to this feature.

::: testing/marionette/harness/marionette/runner/httpd.py:121
(Diff revisions 4 - 6)
>      parser.add_argument(
> -        "-c", action="store", dest="ssl_cert", default=default_ssl_cert,
> -        help="path to SSL certificate (default %s)" % default_ssl_cert)
> +        "-c", dest="ssl_cert", default=default_ssl_cert,
> +        help="path to SSL certificate (default %(default)s)")
>      parser.add_argument(
> -        "-k", action="store", dest="ssl_key", default=default_ssl_key,
> +        "-k", dest="ssl_key", default=default_ssl_key,
>          help="path to SSL certificate key(default %s)" % default_ssl_key)

you missed to update that part.

::: testing/marionette/harness/marionette/runner/serve.py:144
(Diff revisions 4 - 6)
>      return servers
>  
>  
>  def start(doc_root=None, **kwargs):
> -    """Start all relevant test servers on the given document root, or
> -    otherwise using the default testing/marionette/harness/marionette/www
> +    """Start all relevant test servers.
> +    

Linter failure due to leading spaces

::: testing/marionette/harness/marionette/tests/harness_unit/test_httpd.py:1
(Diff revision 6)
> +# This Source Code Form is subject to the terms of the Mozilla Public

Please get Maja to review this harness test. She is the expert here.

::: testing/marionette/harness/marionette/tests/harness_unit/test_httpd.py:43
(Diff revision 6)
> +    assert isinstance(url, types.StringTypes)
> +    assert "http://" in url
> +
> +
> +def test_router(server):
> +    assert server.router is not None

Maybe we can add a check that handlers can be registered?
Attachment #8788174 - Flags: review?(hskupin) → review-
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review92780

::: testing/marionette/harness/marionette/runner/base.py
(Diff revision 6)
> -from argparse import ArgumentParser
> -
> -from copy import deepcopy
>  import json
> -import mozinfo
> -import moznetwork

This is the only place where moznetwork was used. We can also remove the dependency for this package from marionette_client.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> Sorry for being not that precise. I actually meant `setUp()` here, which handles the weakref of Marionette. I haven't checked the implementation in detail yet, but maybe it's old and no-longer used code.

OK.

To make sense of this I’ve moved the assignment of `marionette_weakref` and `fixtures` to `CommonTestCase`, since it is `CommonTestCase.setUp` that unwraps the Marionette weak reference and assigns `self.marionette`.  Since `fixtures` does not require unwrapping, it is assigned directly in the ctor.

This is the summary of the changes: https://gist.github.com/andreastt/ab4da121f8c4754240d52395be6f89df
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> As long as we come to a conclusion that one of the two things will work I'm fine:
> 
> 1) Tests can use the global fixture server and attach a handler
> 2) Tests can instantiate their own fixture server for handlers (which will not be via the default port)

1 is not possible with the approach implemented in this patch.  2 is, but I’m not sure it is better than 1.

I’m going to consider this issue solved now.  We can always change later.

> Sounds good. I assume we even won't have it tested and cannot see the results on try.

Resolving this issue for you then.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review92774

> Maybe we can add a check that handlers can be registered?

That is a test for wptserve, not for testing/marionette/harness/marionette/runner/httpd.py.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review93578

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:28
(Diff revision 7)
>      MarionetteTestRunner instance with mocked-out
>      self.marionette and other properties,
>      to enable testing runner.run_tests().
>      """
>      runner.driverclass = mock_marionette
>      for attr in ['_set_baseurl', 'run_test_set', '_capabilities']:

Remove `_set_baseurl`

::: testing/marionette/harness/marionette/runner/base.py
(Diff revision 8)
> -        need_external_ip = True
> -        if not self.marionette:
> -            self.marionette = self.driverclass(**self._build_kwargs())
> -            # if we're working against a desktop version, we usually don't need
> -            # an external ip
> -            if self.appName != 'fennec':

I know you already responded to a comment from Henrik about this, but I'd like to raise the issue again: don't actively break the Marionette runner on Fennec. It's easier and faster to get it right now, while you're in the middle of this patch, then coming back to it later. For your convenience: `try: -b d -p android-api-15 -u marionette -t none`
Attachment #8788174 - Flags: review?(mjzffr) → review-
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review93578

> I know you already responded to a comment from Henrik about this, but I'd like to raise the issue again: don't actively break the Marionette runner on Fennec. It's easier and faster to get it right now, while you're in the middle of this patch, then coming back to it later. For your convenience: `try: -b d -p android-api-15 -u marionette -t none`

Thanks for the try syntax.  Just to be clear, Fennec runs in an Android emulator that requires the host to listen on a public host?  I thought adb port forwarding would be able to expose _any_ port to the guest OS in the emulator?

The reason we forced binding to external IPs was originally that we needed to run the harness against real devices connected over USB.  But we can instruct httpd.py to listen on 0.0.0.0 by default.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review93578

> Thanks for the try syntax.  Just to be clear, Fennec runs in an Android emulator that requires the host to listen on a public host?  I thought adb port forwarding would be able to expose _any_ port to the guest OS in the emulator?
> 
> The reason we forced binding to external IPs was originally that we needed to run the harness against real devices connected over USB.  But we can instruct httpd.py to listen on 0.0.0.0 by default.

Correction: _Not_ connected over USB, but over the WLAN.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review93578

> Correction: _Not_ connected over USB, but over the WLAN.

I just remember that removing the `need_external_ip` code paths broke the runner on Fennec before: https://bugzilla.mozilla.org/show_bug.cgi?id=1294427
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> 1 is not possible with the approach implemented in this patch.  2 is, but I’m not sure it is better than 1.
> 
> I’m going to consider this issue solved now.  We can always change later.

It is a feature we currently need and as said earlier Telemetry tests (which are currently in the works) rely on. We really should not regress this feature. Lets close this issue only if the test in test_httpd.py is passing.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review93578

> I just remember that removing the `need_external_ip` code paths broke the runner on Fennec before: https://bugzilla.mozilla.org/show_bug.cgi?id=1294427

Made the servers listen on a public interface by default.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> It is a feature we currently need and as said earlier Telemetry tests (which are currently in the works) rely on. We really should not regress this feature. Lets close this issue only if the test in test_httpd.py is passing.

Henceforth we no longer have a single HTTPD server and tests need to be specific about which of them to use, unless they rely on the `marionette.absolute_url(…)` stopgap which defaults to the HTTP server.  For future tests we should move towards using `self.fixtures.where_is(URL, on=SERVER_ID)` which by default selects the HTTP server.

This API hides the complexity of server and process management for long-running HTTPDs, and disassociates any server state from the test case.  I consider both these things to be good.  Currently these long-running processes last for the lifetime of the Marionette test runner.

If we want to expose the ability for tests to register routes on the long-running HTTPDs at runtime, we need to expose references to _all_ of these, which can scale linearly in the future as we add more server types.  As I’ve tried to explain before, we have three options:

1. Expose each long-running HTTPD instance to the test
2. Start and stop HTTPDs per test case
3. Let tests start their own HTTPDs as needed, using the long-running HTTPDs for “common fixtures”

I believe (3) is the path of least resistance currently, and it’s not clear to me why this doesn’t address the Telemetry tests’ requirements.  Am I missing something?
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review92774

> That is a test for wptserve, not for testing/marionette/harness/marionette/runner/httpd.py.

Decided to add a test for it after all.
Comment on attachment 8808302 [details]
Bug 1103196 - Add ability to ignore invalid TLS certificates;

https://reviewboard.mozilla.org/r/91118/#review92386

I share your concerns with the choice of API.  I think it is worth circling back to the WG that it is impossible for us to conform 100% to the specification as it is written with the carte blanche `acceptInsecureCerts` boolean, for the reasons stated above (comment 61).

As I understand it, our preferred solution would be for the user of the Marionette API provide a set of certificates (possibly root certificates covering multiple sites) as a capability that we would install using `nsIX509CertDB` on creating a new session.  When destroying the session we could nuke the cert store and clear the site security service.

The alternative I argued at the last WG meeting was a whitelist array of domains to accept invalid and self-signed certs for, but this approach really isn’t any better than a boolean, apart from (I believe) at the consumer API level.  It doesn’t approach any of the technical challenges you have highlighted.

Meanwhile I would like to move forward with integrating this patch as it is.

Thank you and jryans for the thorough review and feedback on this!

> I wouldn't call this "cert" - maybe "insecureCertOverride" or something?

Marionette exports a single symbol the same name as the file for each module.  I will rename `SweepingOverride` so that it is clear it is insecure, however.

> I'm surprised this is necessary.

It apparently isn’t.  Removed.

> This mechanism won't work unless you pass all of the bits `Error.Untrusted | Error.Mismatch | Error.Time`, so I wouldn't even make it optional - just hard-code it (this is a consequence of how the back-end matches stored overrides to certificate errors it encounters).

Noted and changed.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review93988
Attachment #8788174 - Flags: review?(mjzffr) → review+
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review91270

> Henceforth we no longer have a single HTTPD server and tests need to be specific about which of them to use, unless they rely on the `marionette.absolute_url(…)` stopgap which defaults to the HTTP server.  For future tests we should move towards using `self.fixtures.where_is(URL, on=SERVER_ID)` which by default selects the HTTP server.
> 
> This API hides the complexity of server and process management for long-running HTTPDs, and disassociates any server state from the test case.  I consider both these things to be good.  Currently these long-running processes last for the lifetime of the Marionette test runner.
> 
> If we want to expose the ability for tests to register routes on the long-running HTTPDs at runtime, we need to expose references to _all_ of these, which can scale linearly in the future as we add more server types.  As I’ve tried to explain before, we have three options:
> 
> 1. Expose each long-running HTTPD instance to the test
> 2. Start and stop HTTPDs per test case
> 3. Let tests start their own HTTPDs as needed, using the long-running HTTPDs for “common fixtures”
> 
> I believe (3) is the path of least resistance currently, and it’s not clear to me why this doesn’t address the Telemetry tests’ requirements.  Am I missing something?

The updated testcase for httpd makes it clear now how it should get implemented via a test-wide server. I think that this is totally clear now and also solves all my concerns. Thanks.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review94514
Attachment #8788174 - Flags: review?(hskupin) → review+
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review95256

Please see my comments inline. Also please re-ask for review from Maja for the harness unit test. Thanks.

::: testing/marionette/harness/marionette/runner/serve.py:78
(Diff revisions 11 - 13)
>  
>          try:
>              while True:
>                  # ["func", ("arg1", "arg2", ...)]
>                  func, args = self.recv()
> -                self.send(getattr(server, func)(*args))
> +                attr = getattr(server, func)

Maybe you should rename `func` to something else given that it is not always a function. 

Also how do you handle `AttributeError` exceptions for unknown attributes? Right now you won't send a response.

::: testing/marionette/harness/marionette/tests/harness_unit/test_httpd.py:45
(Diff revisions 11 - 13)
>      url = server.get_url("/")
>      assert isinstance(url, types.StringTypes)
>      assert "http://" in url
>  
> +    server.stop()
> +    with pytest.raises(Exception):

We don't throw a specific exception type in those cases?

::: testing/marionette/harness/marionette/tests/harness_unit/test_serve.py:41
(Diff revision 13)
> +def test_start_with_custom_root(tmpdir_factory):
> +    tdir = tmpdir_factory.mktemp("foo")
> +    serve.start(str(tdir))
> +    for server in iter_proc(serve.servers):
> +        assert server.doc_root == tdir
> +        server.stop()

If the assertion above fails it will not stop the remaining servers. How does it affect the next tests?
Attachment #8788174 - Flags: review+ → review?(hskupin)
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review95256

> We don't throw a specific exception type in those cases?

I don’t think there are any better built-in Python exceptions: https://docs.python.org/2/library/exceptions.html

Since we don’t have more than one exception scenario, where the server hasn’t been started yet, I’m not too concerned about using this.  But it would certainly be possible to define a custom one.

> If the assertion above fails it will not stop the remaining servers. How does it affect the next tests?

The test itself will hault and you will have to kill the python process.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review95256

> Maybe you should rename `func` to something else given that it is not always a function. 
> 
> Also how do you handle `AttributeError` exceptions for unknown attributes? Right now you won't send a response.

Renamed some variables to clarify this.

I think throwing `AttributeError` when the attribute doesn’t exist is perfectly fine.

> The test itself will hault and you will have to kill the python process.

I added the following to stop remaining servers after each test function:

```py
def teardown_function(func):
    for server in [server for server in iter_proc(serve.servers) if server.is_alive]:
        server.stop()
        server.kill()
```
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review95508

Looks fine to me now. Just some minor things remaining. Beside that I would suggest that you also ask Maja for review again given that she is more familiar with the harness unit tests.

::: testing/marionette/harness/marionette/runner/serve.py:76
(Diff revisions 14 - 15)
>          server = self.init_func(*self.init_args, **self.init_kwargs)
>          server.start(block=False)
>  
>          try:
>              while True:
>                  # ["func", ("arg1", "arg2", ...)]

nit: the comment needs an update given that we not only allow function calls now.

::: testing/marionette/harness/marionette/tests/harness_unit/test_serve.py:38
(Diff revisions 14 - 15)
>      assert len(serve.servers) == 2
>      assert "http" in serve.servers
>      assert "https" in serve.servers
>      for url in iter_url(serve.servers):
>          assert isinstance(url, types.StringTypes)
>      for server in iter_proc(serve.servers):

Maybe getting rid of this extra loop given that we have the `stop()` call also in teardown now? It would also apply to some of the test methods below.
Attachment #8788174 - Flags: review?(hskupin) → review+
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review95256

> I don’t think there are any better built-in Python exceptions: https://docs.python.org/2/library/exceptions.html
> 
> Since we don’t have more than one exception scenario, where the server hasn’t been started yet, I’m not too concerned about using this.  But it would certainly be possible to define a custom one.

Do whatever you think is best in this situation.
Comment on attachment 8788174 [details]
Bug 1103196 - Add HTTPS fixture server for Marionette;

https://reviewboard.mozilla.org/r/76756/#review95256

> Do whatever you think is best in this situation.

Introduced a new exception.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c42ee29c468
Add HTTPS fixture server for Marionette; r=automatedtester,maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/baa46ca196fa
Mark specificationLevel capability as proprietary; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/b63c853a1b56
Logically reorder variables defining session state; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/3d2b43bd09e6
Add acceptInsecureCerts capability; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/b2910c43905c
Remove non-conformant acceptSslCerts capability; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/0c28b77a3279
Add insecure certificate error; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f27d827ba404
Error on encountering invalid certificate; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/9fae5015803b
Add ability to ignore invalid TLS certificates; r=automatedtester,keeler,mossop
Backed out for broken Marionette tests on Windows (error loading pages):

https://hg.mozilla.org/integration/autoland/rev/602dfbdc77eac57cbb512c103467c37c1907699c
https://hg.mozilla.org/integration/autoland/rev/bd0f63832f4e9de0daab3cadba42330da7a6657e
https://hg.mozilla.org/integration/autoland/rev/9c1a017771468294719d900658314f79b6e26f79
https://hg.mozilla.org/integration/autoland/rev/e74e31391dbd1807aa7be7160e34ecc1a7be3893
https://hg.mozilla.org/integration/autoland/rev/f43e5a1f88a04b2fede6d17f41756b5598b39d6b
https://hg.mozilla.org/integration/autoland/rev/92e3bd08056fc7c68a86ad540a39f22a33e7f617
https://hg.mozilla.org/integration/autoland/rev/0dc0daaaa649064a1663d5bf5f0a4b169e9379c8
https://hg.mozilla.org/integration/autoland/rev/bb320c72a6bba8d688cb66736839c8d348ba0502

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9fae5015803b847ab5fdf6c6c93b0aee348e17be
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7216089&repo=autoland
14:55:55     INFO -  TEST-START | test_accessibility.py TestAccessibility.test_click_raises_element_not_accessible
14:55:55    ERROR -  TEST-UNEXPECTED-ERROR | test_accessibility.py TestAccessibility.test_click_raises_element_not_accessible | UnknownException: Reached error page: about:neterror?e=connectionFailure&u=http%3A//0.0.0.0%3A49320/test_accessibility.html&c=UTF-8&f=regular&d=Firefox%20can%E2%80%99t%20establish%20a%20connection%20to%20the%20server%20at%200.0.0.0%3A49320.
14:55:55     INFO -  Traceback (most recent call last):
14:55:55     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette\marionette_test\testcases.py", line 166, in run
14:55:55     INFO -      testMethod()
14:55:55     INFO -    File "c:\slave\test\build\tests\marionette\tests\testing\marionette\harness\marionette\tests\unit\test_accessibility.py", line 128, in test_click_raises_element_not_accessible
14:55:55     INFO -      self.setup_accessibility()
14:55:55     INFO -    File "c:\slave\test\build\tests\marionette\tests\testing\marionette\harness\marionette\tests\unit\test_accessibility.py", line 97, in setup_accessibility
14:55:55     INFO -      self.marionette.navigate(test_accessibility)
14:55:55     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\marionette.py", line 1649, in navigate
14:55:55     INFO -      self._send_message("get", {"url": url})
14:55:55     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\decorators.py", line 23, in _
14:55:55     INFO -      return func(*args, **kwargs)
14:55:55     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\marionette.py", line 740, in _send_message
14:55:55     INFO -      self._handle_error(err)
14:55:55     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\marionette.py", line 773, in _handle_error
14:55:55     INFO -      raise errors.lookup(error)(message, stacktrace=stacktrace)
Flags: needinfo?(ato)
We spawn the HTTP(S) servers on 0.0.0.0 but not 127.0.0.1 (localhost). I have to say that at least the latest try build did not run any tests on Windows, so that this problem was not recognized. Maybe a problem on Taskcluster?
This is probably because 0.0.0.0 isn’t recognised on Windows systems.  We have to pull moznetwork back in to get a public interface to bind to, but it might just be fine to bind to 127.0.0.1 for the time being.
Flags: needinfo?(ato)
Could you add back the check for Fennec and set 0.0.0.0 accordingly?
Flags: needinfo?(ato)
Flags: needinfo?(ato)
Summary: Support for untrusted/self-signed TLS certificates → setTimeout set in injected script is cancelled
Summary: setTimeout set in injected script is cancelled → Support for untrusted/self-signed TLS certificates → setTimeout set in injected script is cancelled
Oops.
Summary: Support for untrusted/self-signed TLS certificates → setTimeout set in injected script is cancelled → Support for untrusted/self-signed TLS certificates
(In reply to Maja Frydrychowicz (:maja_zf) from comment #236)
> Could you add back the check for Fennec and set 0.0.0.0 accordingly?

Since 0.0.0.0 doesn’t resolve to a public networking interface on Windows, I’ve updated the HTTPD fixtures patch to bind to the IP given by `moznetwork.get_ip()` by default.

This should resolve the Fennec failures observed in https://treeherder.mozilla.org/#/jobs?repo=try&revision=2138b54cec5d.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64b29e13e60f
Add HTTPS fixture server for Marionette; r=automatedtester,maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/ae960fe256d5
Mark specificationLevel capability as proprietary; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/e29928717802
Logically reorder variables defining session state; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/129b46743db0
Add acceptInsecureCerts capability; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/322b7d11be2d
Remove non-conformant acceptSslCerts capability; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/7c9f81f27145
Add insecure certificate error; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/ca031e434fb9
Error on encountering invalid certificate; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/d207581536c0
Add ability to ignore invalid TLS certificates; r=automatedtester,keeler,mossop

        Attachment #8788174 -
        Attachment is obsolete: true

        Attachment #8808296 -
        Attachment is obsolete: true

        Attachment #8808297 -
        Attachment is obsolete: true

        Attachment #8808298 -
        Attachment is obsolete: true

        Attachment #8808299 -
        Attachment is obsolete: true

        Attachment #8808300 -
        Attachment is obsolete: true

        Attachment #8808301 -
        Attachment is obsolete: true

        Attachment #8808302 -
        Attachment is obsolete: true

    
    

    
  
Sheriffs: Test-only uplift as Marionette is hidden behind flag, please.  Uplift bug 1317462 first as it is a dependency.
Whiteboard: [marionette=1.0] → [marionette=1.0][checkin-needed-aurora]

    
    

    
  
As mentioned by end of last week it would have been very appreciated if all known regression would have been fixed before we backported these changes to aurora. Our Fx-ui tests for nightly builds are in a very busted state on Aurora too now. Please be aware of that for the future. Thanks.

    
    

    
  
It is considered a higher priority for end-users to have TLS certificate management available in the next Firefox ESR, but I’m addressing bug 1320643 at the moment.  I’m not sure if bug 1321517 is an actual regression.  I haven’t examined bug 1320642 yet.

    
    

    
  
There is no need to push through this patch that quickly given that the aurora train is twice that long as usual. There is enough time remaining to get changes uplifted. We even have the beta train for that. So doing a proper uplift without causing known regressions on other branches is from a QA standpoint more than welcome.

Bug 1320642 I have fully examined yesterday, so have a look over there and bug 1322277 about what's going on.

Thanks

    
    

    
  
Hello Henrik,
This issue is a showstopper for many. We can't use latest FF, Webdriver 3.0 binding anymore.

Kindly push this update asap rather than keeping this on hold for other issues to be uplifted.

We(end users) are in much agony since we don't know the ETA of this issue and can't use it either locally or remotely.

Many people raining questions about the reliability of Webdriver Vs commercial products like Smartbears's TestComplete. Our product is not able to support latest version of FF with utmost conviction.

Not sure, how much this is taken seriously and looked at. But just wanted to inform you guys the current situation we are going through.

Thanks

    
    

    
  
Andreas, what do you think about the 51 release? Should we try to get this uplifted? I would expect a couple of merge conflicts to solve. On mozilla-aurora (52) everything looks fine now.
Flags: needinfo?(ato)

    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #271)
> Andreas, what do you think about the 51 release? Should we try to get this
> uplifted? I would expect a couple of merge conflicts to solve. On
> mozilla-aurora (52) everything looks fine now.

Although the changesets associated with bug 1317462, which is a dependency to this, applies cleanly, there is a plethora of conflicts trying to uplift this changeset to beta.  Since 51 is not an ESR, I would say our time is best spent focussing on the features needed for 52.

I also want to add that we recommend WebDriver users to track Firefox Nightly for the time being.  Generally the more recent the Firefox release, the less bugs and the more features.
Flags: needinfo?(ato)

    
    

    
  
(In reply to svaze74 from comment #270)
> This issue is a showstopper for many. We can't use latest FF, Webdriver 3.0
> binding anymore.
> 
> Kindly push this update asap rather than keeping this on hold for other
> issues to be uplifted.

These patches are available in Firefox Nightly and Firefox DevEdition/Aurora.

    
    

    
  
Makes sense. Marking as wontfix then. Thanks.

          status-firefox51:
          --- → wontfix

    
    

    
  
(In reply to Andreas Tolfsen ‹:ato› from comment #273)
> (In reply to svaze74 from comment #270)
> > This issue is a showstopper for many. We can't use latest FF, Webdriver 3.0
> > binding anymore.
> > 
> > Kindly push this update asap rather than keeping this on hold for other
> > issues to be uplifted.
> 
> These patches are available in Firefox Nightly and Firefox DevEdition/Aurora.

Thanks Adreas and Henrik. Appreciate your response. Though tried using the nightly build, its not launching the nightly binary from C:\Program Files\Nightly\Firefox.exe location.
Is there a different way to setup Nightly binary to be launched instead regular exe? 

Thank

    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #274)
> Makes sense. Marking as wontfix then. Thanks.

Henrik, so when can we expect the fix for this issue in full public release ESR?

    
    

    
  
(In reply to svaze74 from comment #275)
> Thanks Adreas and Henrik. Appreciate your response. Though tried using the
> nightly build, its not launching the nightly binary from C:\Program
> Files\Nightly\Firefox.exe location.
> Is there a different way to setup Nightly binary to be launched instead
> regular exe? 

See the {"moz:firefoxProfile": {"binary": "/path/to/nightly"}} capability as documented in the geckodriver README: https://github.com/mozilla/geckodriver#firefox-capabilities

(In reply to svaze74 from comment #276)
> (In reply to Henrik Skupin (:whimboo) from comment #274)
> > Makes sense. Marking as wontfix then. Thanks.
> 
> Henrik, so when can we expect the fix for this issue in full public release
> ESR?

Yes, Aurora (52) will be the next ESR.  Although please let me reiterate that you should track Firefox Nightly (currently 53) for the best WebDriver support.

You can get the latest nightlies from https://nightly.mozilla.org/.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
I think the problem you both noticed should be the same as what I discovered with inappropriately not running but failing unit tests. I filed this as bug 1325079. It indeed shows that we are failing.
Depends on: 1325079

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  
Just to prove to commenters that this functionality is working in Marionette, I’m attaching a screenshot of Marionette successfully navigating to a a site with an expired TLS certificate.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
This is not the place for Marionette support.  Please do not use this bug to debug Selenium issues.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Flags: needinfo?(ato)

    
    

    
  
So I had a look at this from the Selenium side and can tell that everything is working as expected when using a Firefox version starting with 52.0 (soon in Beta) and having Marionette enabled. In case anyone has problems please check my blog post:

https://www.hskupin.info/2017/01/23/using-selenium-and-webdriver-to-interact-with-insecure-ssl-pages-in-firefox/

And please don't comment anymore on this bug. Thanks.

    
    

    
      
  

    
    

    
  
@Joshua
This should work, its working for me!
    --Scala code--
    var caps = DesiredCapabilities.firefox();
    caps.setCapability("marionette", true);
    caps.setCapability("acceptInsecureCerts",true)
    var capabilities = new FirefoxOptions()
        .addTo(caps)

    
  
Product: Testing → Remote Protocol

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: