Closed Bug 1409275 Opened 6 years ago Closed 6 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
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: 6 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.