Closed Bug 1409275 Opened 7 years ago Closed 7 years ago

64 bit windows firefox uses wrong default gssapi library name "gssapi32" instead of "gssapi64" DLL

Categories

(Core :: Networking, defect, P1)

Product:
Core
Component:
Networking
Version:
56 Branch
Platform:
x86_64
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
relnote-firefox --- 56+
firefox-esr52 --- unaffected
firefox56 + fixed
firefox57 blocking fixed
firefox58 + fixed

People

(Reporter: odenbach, Assigned: u408661)

References

Details

(Keywords: regression, Whiteboard: [necko-triaged])

Attachments

(1 file)

Bug 1409275 - Default to gssapi64 on 64-bit windows
59 bytes, text/x-review-board-request
valentin
: review+
ritu
: approval-mozilla-beta+
lizzard
: approval-mozilla-release+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170928210252

Steps to reproduce:

We use MIT Kerberos for authentication. Firefox used to work nearly out of the box, only had to tweak two settings:

network.auth.use-sspi (set to "false")
network.negotiate-auth.trusted-uris (set to "https://")

After the upgrade to version 56 and therefore the switch to the 64 bit build GSSAPI authentication did not work anymore. After searching for a while I found that I had to modify the setting

network.negotiate-auth.gsslib (set to "gssapi64.dll")

to make it work again.

I think the bug is in extensions/auth/nsAuthGSSAPI.cpp:

#ifdef XP_WIN
        char *libName = PR_GetLibraryName(nullptr, "gssapi32");
        if (libName) {
            lib = PR_LoadLibrary("gssapi32");
            PR_FreeLibraryName(libName);

No decision is made there between 32 and 64 bit builds.


Actual results:

GSSAPI authentication did not work because the default library name gssapi64.dll is not set.


Expected results:

Firefox should have looked automatically for gssapi64.dll, just as the 32 bit version automatically looked for gssapi32.dll.
Chris, could you have a look at this 64 bits bug reported about our Kerberos integration? Thanks
Flags: needinfo?(cpeterson)
Flags: needinfo?(lhenry)
Status: UNCONFIRMED → NEW
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox58: --- → affected
Ever confirmed: true
tracking-firefox56: --- → +
tracking-firefox57: --- → +
Flags: needinfo?(lhenry)
Keywords: regression
We can release note this for 56, and should fix it for 57.
We could consider a dot release. Chris, what do you think?
tracking-firefox57: + → blocking
tracking-firefox58: --- → +
@ odenbach: thanks for reporting this bug and including a clear diagnosis! :) In the meantime, you can work around the 64-bit problem by re-installing 32-bit Firefox 56.0.1 ("Windows 32-bit") from https://www.mozilla.org/firefox/all/ . You will not be migrated to 64-bit again after you are running version 56.0.1 or later.

@ Patrick: who is the module owner for GSSAPI and Kerberos authentication?
status-firefox-esr52: --- → unaffected
Component: Untriaged → Networking
Flags: needinfo?(cpeterson) → needinfo?(mcmanus)
OS: Unspecified → Windows
Product: Firefox → Core
Hardware: Unspecified → x86_64
Summary: 64 bit windows firefox uses wrong default gssapi library name → 64 bit windows firefox uses wrong default gssapi library name "gssapi32" instead of "gssapi64" DLL
Hi,

I would like to point out that there is no need to work around the bug by installing 32-bit firefox. GSSAPI authentication works perfectly in 64-bit Firefox, the only thing to do is enter "gssapi64.dll" in the network.negotiate-auth.gsslib setting. This is easier and faster than a new install - and leaves you with all the advantages that 64-bit Firefox offers.

I would therefore recommend a different wording for the release notes:

[Suggested wording]:
GSSAPI and Kerberos authentication _do_ work in 64-Bit Windows Firefox. Currently however it requires the advanced setting "network.negotiate-auth.gsslib" to contain the string "gssapi64.dll" because the default value (which is chosen if left empty) is wrong. Future versions of Firefox will contain a correct default value.


Cheers,

Christopher
Thanks for the correction. I tagged my comment as obsolete and suggest we use yours. :)
if its as simple as comment 0 suggests, I bet Honza can fix this up asap and take care of uplifts.
Flags: needinfo?(mcmanus) → needinfo?(honzab.moz)
The workaround is as simple as I said. A real solution however would be to put some #ifdef statements around the call

            lib = PR_LoadLibrary("gssapi32");

so something like

#ifdef _WIN64
            lib = PR_LoadLibrary("gssapi64");
#else
            lib = PR_LoadLibrary("gssapi32");
#endif

(I did not check this myself). If someone wants me to test a version containing this I will happily assist.
Honza's out sick. I'll take care of writing this patch, per Jason's request.
Assignee: nobody → hurley
Flags: needinfo?(honzab.moz)
Priority: -- → P1
Whiteboard: [necko-triaged]
Attachment #8919882 - Flags: review?(valentin.gosu)
Comment on attachment 8919882 [details]
Bug 1409275 - Default to gssapi64 on 64-bit windows

https://reviewboard.mozilla.org/r/190826/#review196006

::: extensions/auth/nsAuthGSSAPI.cpp:132
(Diff revision 1)
>          if (libName) {
> +            #ifdef _WIN64
> +            lib = PR_LoadLibrary("gssapi64");
> +            #else
>              lib = PR_LoadLibrary("gssapi32");
> +            #endif

nit: would be cleaner if we defined just one kLibName at the top of the file and avoid duplicating code and multiple ifdefs.
Attachment #8919882 - Flags: review?(valentin.gosu) → review+
@odenbach, could you try a build from https://tools.taskcluster.net/index/artifacts/gecko.v2.try.revision.ec8d7b2ca9e3afc007685a6b81d9c304ed8c9731.firefox/win64-opt (look for the "setup.exe" file, I think, I've never used the taskcluster index before) to verify that this uses the appropriate library? Thanks!
Flags: needinfo?(odenbach)
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/121933d763b4
Default to gssapi64 on 64-bit windows r=valentin
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #14)
> @odenbach, could you try a build from
> https://tools.taskcluster.net/index/artifacts/gecko.v2.try.revision.
> ec8d7b2ca9e3afc007685a6b81d9c304ed8c9731.firefox/win64-opt (look for the
> "setup.exe" file, I think, I've never used the taskcluster index before) to
> verify that this uses the appropriate library? Thanks!

@hurley I just tried the setup.exe from above, but it does not install anything apart from a nearly empty "Nightly" directory. install.log contains:

��Nightly Installation Started: 2017-10-19 11:24:08
-------------------------------------------------------------------------------

-------------------------------------------------------------------------------
Installation Details
-------------------------------------------------------------------------------
  Install Dir: C:\Program Files\Nightly
  Locale     : en-US
  App Version: 58.0a1
  GRE Version: 58.0a1
  OS Name    : Windows 7
  Target CPU : x64

-------------------------------------------------------------------------------
Installing Main Files
-------------------------------------------------------------------------------

-------------------------------------------------------------------------------
DLL Registration
-------------------------------------------------------------------------------
  ** ERROR Registering: C:\Program Files\Nightly\AccessibleMarshal.dll **
  ** ERROR Registering: C:\Program Files\Nightly\AccessibleHandler.dll **

[...]
Flags: needinfo?(odenbach)
https://hg.mozilla.org/mozilla-central/rev/121933d763b4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #17)
> https://hg.mozilla.org/mozilla-central/rev/121933d763b4

How can I verify your change? Code looks good to me, I just would like to check a build.
This should make it into the next nightly build that gets built. They happen twice a day now so that will be later today. If you reload the hg.mozilla.org URL, it should have links to the builds once it makes it into a build.
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #14)
> @odenbach, could you try a build from
> https://tools.taskcluster.net/index/artifacts/gecko.v2.try.revision.
> ec8d7b2ca9e3afc007685a6b81d9c304ed8c9731.firefox/win64-opt (look for the
> "setup.exe" file, I think, I've never used the taskcluster index before) to
> verify that this uses the appropriate library? Thanks!

Almost! You wanted target.installer.exe:
https://index.taskcluster.net/v1/task/gecko.v2.try.revision.ec8d7b2ca9e3afc007685a6b81d9c304ed8c9731.firefox.win64-opt/artifacts/public/build/install/sea/target.installer.exe

or target.zip to get a non-installer package:
https://index.taskcluster.net/v1/task/gecko.v2.try.revision.ec8d7b2ca9e3afc007685a6b81d9c304ed8c9731.firefox.win64-opt/artifacts/public/build/target.zip

odenbach: you can test the try build from one of those links above if you don't want to wait for it to make a nightly build.

(setup.exe is some internal installer bit that gets used for repackaging builds or something)
Added to the release notes with the proposed wording.
relnote-firefox: ? → 56+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20)
> (In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org)
> from comment #14)
> > @odenbach, could you try a build from
> > https://tools.taskcluster.net/index/artifacts/gecko.v2.try.revision.
> > ec8d7b2ca9e3afc007685a6b81d9c304ed8c9731.firefox/win64-opt (look for the
> > "setup.exe" file, I think, I've never used the taskcluster index before) to
> > verify that this uses the appropriate library? Thanks!
> 
> Almost! You wanted target.installer.exe:
> https://index.taskcluster.net/v1/task/gecko.v2.try.revision.
> ec8d7b2ca9e3afc007685a6b81d9c304ed8c9731.firefox.win64-opt/artifacts/public/
> build/install/sea/target.installer.exe

Just tried it - and it works! Thanks a lot!
ted - thanks for setting me straight!

odenbach - thanks for verifying this works!

I'll get the uplift requested so we can get this into 57.
Comment on attachment 8919882 [details]
Bug 1409275 - Default to gssapi64 on 64-bit windows

Approval Request Comment
[Feature/Bug causing the regression]: 64-bit windows builds
[User impact if declined]: gssapi auth doesn't work without a workaround
[Is this code covered by automated tests?]: not to my knowledge
[Has the fix been verified in Nightly?]: yes (see a couple comments up)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple change to a lookup the 64-bit library on 64-bit platforms
[String changes made/needed]: none
Attachment #8919882 - Flags: approval-mozilla-beta?
Comment on attachment 8919882 [details]
Bug 1409275 - Default to gssapi64 on 64-bit windows

Must fix, Beta57+
Attachment #8919882 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Can you request uplift to m-r?  I'm getting a 56 dot release ready now, and think this would make a good ridealong issue to fix, especially as we keep increasing the percentage of users getting 64-bit over the next couple of weeks.
Flags: needinfo?(hurley)
Comment on attachment 8919882 [details]
Bug 1409275 - Default to gssapi64 on 64-bit windows

Approval Request Comment: see comment 24
Flags: needinfo?(hurley)
Attachment #8919882 - Flags: approval-mozilla-release?
Comment on attachment 8919882 [details]
Bug 1409275 - Default to gssapi64 on 64-bit windows

Taking this as a ridealong for 56.0.2.
Attachment #8919882 - Flags: approval-mozilla-release? → approval-mozilla-release+
Isn't this needed for ESR too?
> Isn't this needed for ESR too?
Nope, we are not migrating the ESR users.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: