Closed Bug 861521 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

(firefox23 fixed, firefox24 fixed, firefox25 fixed)

RESOLVED FIXED
Tracking Status
firefox23 --- fixed
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: AndreeaMatei, Assigned: mario.garbi)

References

()

Details

(Keywords: regression, Whiteboard: [mozmill-test-failure][sprint2013-31])

Attachments

(3 files, 2 obsolete files)

Also started today, on Nightly, all platforms and locales. I will investigate in a bit.
Whiteboard: [mozmill-test-failure]
Attached patch skip testSplinter Review
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.
Attachment #737140 - Flags: review?(hskupin)
Attachment #737140 - Flags: review?(dave.hunt)
Attachment #737140 - Flags: review?(hskupin)
Attachment #737140 - Flags: review?(dave.hunt)
Attachment #737140 - Flags: review+
Priority: -- → P2
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped][sprint2013-31]
Can we make some progress here?
I'm taking this and will come with an update as soon as possible.
Assignee: nobody → mario.garbi
Updating flags since we skip this both on Aurora 23 and Nightly 24.
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).
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(bsmith)
(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.
Flags: needinfo?(brian)
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?
Attachment #772052 - Flags: review?(andreea.matei) → review-
(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.
(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.
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
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.
Attachment #772052 - Flags: review?(hskupin)
Attachment #772052 - Flags: review?(andreea.matei)
Attachment #772052 - Flags: review-
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 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.
Attachment #772052 - Flags: review?(hskupin)
Attachment #772052 - Flags: review?(andreea.matei)
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.
Attachment #772052 - Flags: review?(andreea.matei)
Status: NEW → ASSIGNED
Attached patch securityTLSPrefHandle_v2.patch (obsolete) — Splinter Review
Updated the patch so it applies cleanly again (the change was just in the manifest.ini).
Attachment #772052 - Attachment is obsolete: true
Attachment #772052 - Flags: review?(andreea.matei)
Attachment #777751 - Flags: review?(andreea.matei)
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.
Attachment #777751 - Flags: review?(andreea.matei) → review-
Made the requested nit fix.
Attachment #777751 - Attachment is obsolete: true
Attachment #777771 - Flags: review?(andreea.matei)
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!
Attachment #777771 - Flags: review?(andreea.matei) → review+
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)
Attachment #779145 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint2013-31] → [mozmill-test-failure][sprint2013-31]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.