861521 - Test failure "Timeout exceeded for waitForElement ID: errorTitleText" in testSecurity/testSSLDisabledErrorPage.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andreea Matei [:AndreeaMatei]]

Also started today, on Nightly, all platforms and locales. I will investigate in a bit.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Attachment: skip test

This is due to bug 733642, where the support for prefs ssl and tls was dropped and they were replaced with security.tls.version.min and security.tls.version.max prefs.

The test needs an update, adding a skip patch to stop failures until that is done.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/9d7b50f8a82d (default)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can we make some progress here?

Comment 4

[mario garbi]

I'm taking this and will come with an update as soon as possible.

Comment 5

[mario garbi]

Updating flags since we skip this both on Aurora 23 and Nightly 24.

Comment 6

[mario garbi]

The preferences "network.http.keep-alive", "security.enable_ssl3" and "security.enable_tls" have been removed in 22.03.2013 as can be seen in Bug 733642. They were replaced with two preferences that handle the interval of security protocols we want to use (ssl3 = 0 , tls 1.1 = 2). These two new preferences are called "security.tls.version.min" and "security.tls.version.max" and have int value types as specified above.

Do we want to rewrite the test to handle this new development or do we remove the test since enable/disable of SSL3 or TLS isn't possible anymore (we can specify the highest value for .Min or lowest for .Max but that will not disable the protocols, just limit the browser acceptance).

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, as you know we should figure out if the already existent Mochitests or XPCShell tests already cover that scenario. It's not a clear functional test anymore, so it might be possible.

Comment 8

[mario garbi]

Brian I've looked over Mochitests on MXR and I wasn't able to find a test relevant to this issue, I'd like to confirm with you whether  we have a test or not for the new functionality of security protocols.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to mario garbi from comment #6)
> Do we want to rewrite the test to handle this new development or do we
> remove the test since enable/disable of SSL3 or TLS isn't possible anymore
> (we can specify the highest value for .Min or lowest for .Max but that will
> not disable the protocols, just limit the browser acceptance).

Setting the .min and .max versions WILL enable/disable the versions in a similar way to how the previous prefs controlled them. The only limitation is that it isn't possible to disable a version in the middle. For example, you can't enable SSL 3.0 and TLS 1.1 but disable TLS 1.0.

Also, at this time, SSL 3.0 and TLS 1.0 are the only versions that we support. Though, we're hoping to add TLS 1.1 and TLS 1.2 very soon (I will be reviewing those patches in the next 24 hours).

(In reply to mario garbi from comment #8)
> Brian I've looked over Mochitests on MXR and I wasn't able to find a test
> relevant to this issue, I'd like to confirm with you whether  we have a test
> or not for the new functionality of security protocols.

Sadly, I don't think we have any xpcshell or mochitests to test which versions of TLS are enabled.

Comment 10

[mario garbi]

Attachment: Patch v1.0

The patch updates the test to use the new preferences by forcing TLS1.2 (value 3). 
I have gathered these values from: http://kb.mozillazine.org/Security.tls.version.*#Possible_values_and_their_effects

This will work with Firefox versions down to 23 for which we need to use TLS1.1 (value 2) as TLS 1.2 isn't supported.

Reports
WinXP:
http://mozmill-crowd.blargon7.com/#/functional/report/593227b0561e6d84f2e52c84222d09ba
http://mozmill-crowd.blargon7.com/#/functional/report/593227b0561e6d84f2e52c84222dddb1

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/593227b0561e6d84f2e52c84222d982c
http://mozmill-crowd.blargon7.com/#/functional/report/593227b0561e6d84f2e52c84222e70c4

Mac:
http://mozmill-crowd.blargon7.com/#/functional/report/593227b0561e6d84f2e52c8422307aad
http://mozmill-crowd.blargon7.com/#/functional/report/593227b0561e6d84f2e52c842230ef17

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 772052 [details] [diff] [review]
Patch v1.0

Review of attachment 772052 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/testSecurity/testSSLDisabledErrorPage.js
@@ -26,5 @@
>    tabs.closeAllTabs(aModule.controller);
>  
> -  // Bug 513129
> -  // Disable Keep-alive connections
> -  prefs.preferences.setPref(PREF_KEEP_ALIVE, false);

This bug has not been fixed yet. So why do you remove disabling keep-alive connections?

@@ +26,5 @@
> +  // Disable SSL 3.0, TLS 1.0 and TLS 1.1 for secure connections
> +  // by forcing the use of TLS 1.2
> +  // see: http://kb.mozillazine.org/Security.tls.version.*#Possible_values_and_their_effects
> +  prefs.preferences.setPref(PREF_TLS_MIN, 3);
> +  prefs.preferences.setPref(PREF_TLS_MAX, 3);

This enables TLS 1.2 as given by the documentation. How would we test that secure sites are failing? I don't see how this works.

@@ +60,5 @@
>    // Verify the error message is correct
>    var text = new elementslib.ID(controller.tabs.activeTab, "errorShortDescText");
>    controller.waitForElement(text);
>  
> +  expect.contain(text.getNode().textContent, 'ssl_error_no_cypher_overlap',

Is this change necessary? Has this error id been also changed by the underlying core change?

Comment 12

[mario garbi]

(In reply to Henrik Skupin (:whimboo) [away 06/28 - 07/07] from comment #11)

> > +  prefs.preferences.setPref(PREF_TLS_MAX, 3);
> 
> This enables TLS 1.2 as given by the documentation. How would we test that
> secure sites are failing? I don't see how this works.

The site we use (https://mail.mozilla.org) has an TLS 1.0 security protocol and by setting the minimum accepted protocol to TLS 1.1 or higher we will get an error page. Due to the new way Firefox handles security protocols (we can't simply disable SSL3 or TLS anymore) this is the only way to force an error page as Brian suggested. It have tried setting the accepted values to 0 (ssl3) but we don't get an error then as it would seem that the page can downgrade it's protocols to our max accepted value.

> >  
> > +  expect.contain(text.getNode().textContent, 'ssl_error_no_cypher_overlap',
> 
> Is this change necessary? Has this error id been also changed by the
> underlying core change?

The Error page is different if we set the min/max values to a high value (TLS 1.2) than what it was when we simply disable SSL3 or TLS (initial approach) and therefore we had a new error message.
This is the same reason why I removed the last expect.contain, the "PSMERR_SSL_Disabled" value isn't present on the page since we would have a different error text.

If there is a better way to handle this I would be happy to remake the patch from scratch, this is just how I understood the solution from what Brian said and what I could find to read about the new preferences and how to disable security protocols.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to mario garbi from comment #12)
> > > +  prefs.preferences.setPref(PREF_TLS_MAX, 3);
> > 
> > This enables TLS 1.2 as given by the documentation. How would we test that
> > secure sites are failing? I don't see how this works.
> 
> The site we use (https://mail.mozilla.org) has an TLS 1.0 security protocol
> and by setting the minimum accepted protocol to TLS 1.1 or higher we will
> get an error page. Due to the new way Firefox handles security protocols (we
> can't simply disable SSL3 or TLS anymore) this is the only way to force an
> error page as Brian suggested. It have tried setting the accepted values to
> 0 (ssl3) but we don't get an error then as it would seem that the page can
> downgrade it's protocols to our max accepted value.

Dumb question but where can I retrieve the information which TLS version is used? I feel bad in using this external site for the test. So we should file a new bug to get subdomains setup for all cases on mozqa.com.

Comment 14

[mario garbi]

I gathered the TLS version by using "openssl s_client -connect mail.mozilla.org:443
" under Linux. This returned:
SSL-Session:
    Protocol  : TLSv1
    Cipher    : DHE-RSA-AES256-SHA

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ah ok. In that case we can go ahead. It sounds good for now until we have our own instances. Please re-request for review then. For the follow-up bug regarding mozqa.com I hope that we can make use of self-signed certificates with all those various kinds of TLS/SSL versions.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 772052 [details] [diff] [review]
Patch v1.0

Review of attachment 772052 [details] [diff] [review]:
-----------------------------------------------------------------

The question remains here regarding the network.http.keep-alive preference. Does that change the test or has been removed?

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 772052 [details] [diff] [review]
Patch v1.0

Review of attachment 772052 [details] [diff] [review]:
-----------------------------------------------------------------

Please only re-request for review once all questions and comments have been addressed.

Comment 18

[mario garbi]

Comment on attachment 772052 [details] [diff] [review]
Patch v1.0

The preference "network.http.keep-alive" was removed a while back with the bug 770331 (link bellow). 
https://bugzilla.mozilla.org/show_bug.cgi?id=770331

I am requesting review again as Henrik suggested.

Comment 19

[mario garbi]

Attachment: securityTLSPrefHandle_v2.patch

Updated the patch so it applies cleanly again (the change was just in the manifest.ini).

Comment 20

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 777751 [details] [diff] [review]
securityTLSPrefHandle_v2.patch

Review of attachment 777751 [details] [diff] [review]:
-----------------------------------------------------------------

I'm good with this once we solve the comment.

::: tests/functional/testSecurity/testSSLDisabledErrorPage.js
@@ +37,4 @@
>  }
>  
>  /**
>   * Test that SSL and TLS are checked by default

Just one nit, we'll need to update here the description, we set those preferences to an unsupported version so that we get the error page.

Comment 21

[mario garbi]

Attachment: securityTLSPrefHandle_v3.patch

Made the requested nit fix.

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 777771 [details] [diff] [review]
securityTLSPrefHandle_v3.patch

Review of attachment 777771 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good now, landed: 
http://hg.mozilla.org/qa/mozmill-tests/rev/da215fa15fe5 (default)

We need to enable this on the other branches as well, please check them. Thanks!

Comment 23

[mario garbi]

The same patch applies on Aurora too:

Mac - http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d6164680a

Linux - http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d6169a446

Windows - http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d616472b4

Comment 24

[mario garbi]

Attachment: securityTLSPrefHandle_Beta.patch

We only need this for Beta since 23 is the oldest version where we have the new preferences.

Reports:

Mac
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61697d2c

Linux
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d6166992f

Windows
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d6164a297

Comment 25

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 779145 [details] [diff] [review]
securityTLSPrefHandle_Beta.patch

Review of attachment 779145 [details] [diff] [review]:
-----------------------------------------------------------------

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/1c37f9cb80a6 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/49158fd0e5d0 (beta)