648730 - Fix Kerberos/GSSAPI authentication on OpenBSD

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Landry Breuil (:gaston)]

Attachment: add krb5 and crypto libs to OS_LIBS on OpenBSD

As reported in http://marc.info/?l=openbsd-ports&m=129111364200720&w=2, on OpenBSD we don't have libgssapi_krb5, so auth fails.
Adding -lkrb5 -lcrypto to OS_LIBS fixes it.
That patch is a candidate for 1.9.1, 1.9.2, 2.0 and trunk.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 524821 [details] [diff] [review]
add krb5 and crypto libs to OS_LIBS on OpenBSD

I have no way to test this, but it looks entirely reasonable.

Comment 2

[Landry Breuil (:gaston)]

Comment on attachment 524821 [details] [diff] [review]
add krb5 and crypto libs to OS_LIBS on OpenBSD

Requesting approval for 1.9.1/1.9.2/2.0..

Comment 3

[(no longer active)]

This doesn't apply cleanly to trunk...

Comment 4

[Landry Breuil (:gaston)]

Attachment: hg diff against trunk

Comment 5

[Landry Breuil (:gaston)]

Yeah, sorry, original patch targets 1.9.1, 1.9.2 and 2.0. The only fuzz is the line numbers... hg cant cope with that ?

Comment 6

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/00ebe89c7391

Comment 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/00ebe89c7391

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 524821 [details] [diff] [review]
add krb5 and crypto libs to OS_LIBS on OpenBSD

Approved for 1.9.2.18 and 1.9.1.20, a=dveditz for release-drivers
Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 524821 [details] [diff] [review]
add krb5 and crypto libs to OS_LIBS on OpenBSD

Didn't make 3.6.18

Comment 10

[Landry Breuil (:gaston)]

And what's needed to get that commited ? approval-xxx + checkin-needed is not enough now ?

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> And what's needed to get that commited ? approval-xxx + checkin-needed is
> not enough now ?

You need to find someone with local 1.9.1 / 1.9.2 / 2.0 clones...

Comment 12

[Justin Wood (:Callek)]

(In reply to Daniel Veditz from comment #8)
> Comment on attachment 524821 [details] [diff] [review] [diff] [details] [review]
> add krb5 and crypto libs to OS_LIBS on OpenBSD
> 
> Approved for 1.9.2.18 and 1.9.1.20, a=dveditz for release-drivers
> Approved for the mozilla2.0 repository, a=dveditz for release-drivers

This missed all those approvals, dropping checkin-needed,

1.9.1 is no longer used by Mozilla, same with 2.0, If its necessary to land there, please re-add checkin-needed and say so [with why].

if we still want this for 1.9.2.* please re-request approval, and "we" can land asap.

Comment 13

[Landry Breuil (:gaston)]

Comment on attachment 524821 [details] [diff] [review]
add krb5 and crypto libs to OS_LIBS on OpenBSD

So be it, request approval1.9.2.20... sigh.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 524821 [details] [diff] [review]
add krb5 and crypto libs to OS_LIBS on OpenBSD

Approved for 1.9.2.21, a=dveditz for release-drivers

Comment 15

[Ed Morley [:emorley]]

Comment on attachment 524821 [details] [diff] [review]
add krb5 and crypto libs to OS_LIBS on OpenBSD

This didn't apply cleanly against mozilla-1.9.2, but I resolved locally (only context changes) and manually added the missing author & commit message; to save another back and forth. (I suspect this is why it hasn't been landed sooner).

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/92f1fc0d8e37

Comment 16

[Landry Breuil (:gaston)]

Backed out 2 years later for causing SIGBUS in bug 853364. Gotta find a better workaround using dlopen() at runtime ?

Comment 17

[Landry Breuil (:gaston)]

http://marc.info/?l=openbsd-ports&m=138997571507056&w=2

Looks like we have a potential working fix, i just need to update it against m-c, and add the proper #ifdef dance to accomodate to OpenBSD's specificities (ie no registered interependencies in basesystem libs...)

Comment 18

[jwborc39963]

Since OS X is BSD based, I believe the same issue is affecting OS X.  On web servers configured to only use kerberos for authentication, the authentication fails, where as Safari will work as long as a valid TGT is present.

Comment 19

[Landry Breuil (:gaston)]

Attachment: Load all the depending libs of libgssapi first

I didnt personally test that, but that's what has been posted on http://marc.info/?l=openbsd-ports&m=138997571507056&w=2, has been shipping to users since a month (added to our ports in http://www.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-extensions_auth_nsAuthGSSAPI_cpp?rev=1.4;content-type=text%2Fplain) and properly integrated within a big #ifdef __OpenBSD__ - with a comment added to explain why we need this
No more ugly linking with krb5-crypto like the previous version...

Comment 20

[Landry Breuil (:gaston)]

I dont know what you guys think about it, but re-reading the code and for the sake of clarity, i'd be a bit more comfortable putting the whole CITI libgssapi detection (if it's still needed ? Reading bug 325433 & 321514 makes me shudder... suse 10 ? 2006 code ?) thing outside of the __OpenBSD__ codepath, for more clarity..  in the end, i _think_ on our side we only need the first loop on verLibNames and not the one on libNames.

So maybe having
#ifdef XP_WIN
.. existing win codepath
#elif defined (__OpenBSD__)
.. new openbsd-only codepath with only verLibNames loop, without the !lib test in the for
#else
.. existing linux/osx codepath
#endif

instead of adding a new #ifdef/#else/#endif block ?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8376835 [details] [diff] [review]
Load all the depending libs of libgssapi first

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

I don't really claim to know whether the patch is correct, so I'll just assume it is.

Please add a comment in the loop about the ifdefs above.

Comment 22

[Landry Breuil (:gaston)]

Attachment: Load all the depending libs of libgssapi first - alternative version

You mean in the bottom-most for loop just before the #endif ?

Do you have an opinion on what i proposed in comment 20 ? (Here's the corresponding patch, which i actually find more clearer)

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8382529 [details] [diff] [review]
Load all the depending libs of libgssapi first - alternative version

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

yeah, this is clearer

Comment 24

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc49d96e84a

Comment 25

[Landry Breuil (:gaston)]

Comment on attachment 8382529 [details] [diff] [review]
Load all the depending libs of libgssapi first - alternative version

Oops, forgot to precise the alternate/clearer version was the one landed. Bug can be reclosed on merge to m-c.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fc49d96e84a