User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20130109164724 Steps to reproduce: Unchanged configuration: Server requires Client authentication Use firefox sync with own server + client certificate Actual results: Firefox sync stopped working after upgrade to FF18. I downloaded the old 17.0.0 release and tested with an fresh profile: firefox sync with my own server works: tested with fresh profile 18.0.0 -> firefox sync displays "Choose an valid server" browsing to my sync manually works as it seems not to trigger an Re-negotation. i have attached the apache ssl debug logs for both requests. Apache displays: Re-negotiation handshake failed: Not accepted by client!? Expected results: sync should work like in < FF18

in the past i was affacted by an similar failure: https://bugzilla.mozilla.org/show_bug.cgi?id=616757 the given workaround: setting security.ssl3.dhe_rsa_aes_256_sha = false does not work (i tested with both settings)

i used bisect to nail down the commit: The first bad revision is: changeset: 106951:541a789f3912 user: Allison Naaktgeboren <ally@mozilla.com> date: Wed Sep 12 15:08:07 2012 -0700 summary: Bug 644734; r=gps This is the first revision where my server name https://sync.... is rejected in the "Setup Sync" custom server dialog: "Please enter a valid server URL" is there any further information i can provide to fix the issue?

Thank you very much for bisecting it ! bug 644734 is a security sensitive bug and I hope that Allison or Gregory can explain if this is an expected result of the fix.

I'm guessing adding LOAD_ANONYMOUS caused this to break somehow. I'm guessing bsmith may have a theory or two.

There are certainly auth interactions with LOAD_ANONYMOUS, IIRC -- I think bsmith mentioned something about authenticating proxies at some point.

Right, because of LOAD_ANONYMOUS, Firefox Sync doesn't support servers that require cookies or client certificates as of bug 644734. Changing to the Sync product because it's up to the Sync team about prioritizing support for such configurations.

The solution here, then, is to find a different way to not send cookies in every request. That's all we really use LOAD_ANONYMOUS for. I will happily review a patch, but I'm unlikely to prioritize doing the work. Thanks for the triage, bsmith!

If somebody roughly explains where this can be fixed in the sync code i can try to create a patch! would services/sync/modules/resource.js be the right file for the fix?

(In reply to Bernhard Redl from comment #10) > If somebody roughly explains where this can be fixed in the sync code i can > try to create a patch! > > would services/sync/modules/resource.js be the right file for the fix? And services/common/rest.js. You'll be touching RESTRequest and AsyncResource. You would need to find an equivalent to LOAD_ANONYMOUS that sends client certificates but not cookies.

According to bug 466080 it was planed to not send ClientAuth if LOAD_ANONYMOUS is set. i'm not sure why LOAD_ANONYMOUS was added here (cause i can't view bug 644734) i fear i may reopen a (fixed) security hole if i don't know precisely what bug 644734 was about. So are just Cookies the bad thing we want to prevent in rest.js? Or also Authorisation headers and stuff like that? is the correct way to create a http-on-opening-request observer in JS and remove the Cookie header? Or am i supposed to touch nsHttpChannel::AsyncOpen nsHttpChannel::AsyncOpen .. AddCookiesToRequest(); // notify "http-on-opening-request" observers, but not if this is a redirect if (!(mLoadFlags & LOAD_REPLACE)) { gHttpHandler->OnOpeningRequest(this); } ... ps: bugzilla forced me to specify a version and a component again.

(In reply to Bernhard Redl from comment #12) > i'm not sure why LOAD_ANONYMOUS was added here (cause i can't view bug > 644734) > i fear i may reopen a (fixed) security hole if i don't know precisely what > bug 644734 was about. It was added to avoid automatically sending cookies with Sync requests. We never wish to do so. > So are just Cookies the bad thing we want to prevent in rest.js? Or also > Authorisation headers and stuff like that? Sync sets its own Auth header, but Necko filling them automatically is annoying: Bug 646686. > is the correct way to create a http-on-opening-request observer in JS and > remove the Cookie header? Or am i supposed to touch nsHttpChannel::AsyncOpen Looking at Bug 701019, Bug 627616, and <http://adblockplus.org/blog/why-you-do-not-want-to-use-the-load_anonymous-flag>, I can imagine four options: * Try adding an observer and delete the cookie header. Seems kinda painful. * Add a new load flag (or two) which means "don't automatically add Authorization or Cookie headers from the auth/cookie service". One for Bug 646686 and one for cookies would be great. * Add to nsIChannelEventSink an API which allows the channel event sink to permit or deny the addition of auth or cookie headers from those services. * WONTFIXing this bug. My suggestion is to take the second route, because this has come up several times. That would be a new bug in the Necko component, with the work in this bug being to change the selected load flags. I suggest getting input from the Necko folks before starting work, though.

i have not marked this as patch because it BREAKS firefox cookies handling. Maybe someone can give me feedback why my patch has such an big negativ affect. Also i seek feedback where to put testcases and what they should cover.

Comment on attachment 719791 [details] [diff] [review] Patch - needs work Flagging Honza for some feedback.

Comment on attachment 719791 [details] [diff] [review] Patch - needs work Review of attachment 719791 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsIRequest.idl @@ +218,5 @@ > + /** > + * When set, this flag indicates that no cookies are accepted > + * or added to requests > + */ > + const unsigned long LOAD_NOCOOKIES = 1 << 16; The definitions of the loadflags are spread across nsIRequest.idl, nsIChannel.idl, and nsICachingChannel.idl. So << 16 actually is already taken by LOAD_DOCUMENT_URI and >>17 by LOAD_RETARGETED_DOCUMENT_URI. As discussed on IRC with bz, we only currently have one bit free (1<< 24). So it's time to suck it up and turn the definition of nsLoadFlags (in nsIRequest.idl) into 'unsigned long long'. That'll probably mean a lot of tedious changing of code in C++ that uses the API just to change the type. I've filed bug 846629 specifically for switching the loadFlags to 64 bit. Then we can add the LOAD_NO_COOKIES (please use the _ after NO) in this bug.

Comment on attachment 719791 [details] [diff] [review] Patch - needs work Review of attachment 719791 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1221,5 @@ > > NS_IMETHODIMP > HttpBaseChannel::SetCookie(const char *aCookieHeader) > { > + if (mLoadFlags & LOAD_ANONYMOUS || mLoadFlags & LOAD_NOCOOKIES) In a perfect world we'd change LOAD_ANONYMOUS to map to three flags: NO_COOKIES, NO_AUTH, and some new "don't send SSL client certs" flag. But we don't want to go there, as there's possibly a lot of hard-coded JS out there that relies on the current value of 4. So we'll have to check the flags separately like you do here.

(Whoops--re: last comment: current value of LOAD_ANONYMOUS is 1<<14, not <<4. Got it confused with NS_HTTP_LOAD_ANONYMOUS.)

Comment on attachment 719791 [details] [diff] [review] Patch - needs work Review of attachment 719791 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1221,5 @@ > > NS_IMETHODIMP > HttpBaseChannel::SetCookie(const char *aCookieHeader) > { > + if (mLoadFlags & LOAD_ANONYMOUS || mLoadFlags & LOAD_NOCOOKIES) So sad... :((

(In reply to Jason Duell (:jduell) from comment #17) > In a perfect world we'd change LOAD_ANONYMOUS to map to three flags: > NO_COOKIES, NO_AUTH, and some new "don't send SSL client certs" flag. But > we don't want to go there, as there's possibly a lot of hard-coded JS out > there that relies on the current value of 4. So we'll have to check the > flags separately like you do here. What about to: - change LOAD_ANONYMOUS flag to be build from the flags we want - introduce LOAD_ANONYMOUS_DEPRECATED with value 4 - fix this in SetLoadFlags: on LOAD_ANONYMOUS_DEPRECATED set also all the "new" LOAD_ANONYMOUS flags Yes, wasting of a flag tho.. :(

Sync now uses `fetch` without credentials to make HTTP requests. Please reopen if this is still an issue.