Fix Kerberos/GSSAPI authentication on OpenBSD

RESOLVED FIXED in mozilla6

Status

()

Core
Security
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

unspecified
mozilla6
x86
OpenBSD
Points:
---

Firefox Tracking Flags

(status1.9.2 .23-fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

6 years ago
Created attachment 524821 [details] [diff] [review]
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.

Updated

6 years ago
Attachment #524821 - Attachment is patch: true
Attachment #524821 - Attachment mime type: application/octet-stream → text/plain
Attachment #524821 - Flags: review?(dtownsend)
Attachment #524821 - Flags: review?(dtownsend) → review?(khuey)
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.
Attachment #524821 - Flags: review?(khuey) → review+
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → landry
(Assignee)

Comment 2

6 years ago
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..
Attachment #524821 - Flags: approval2.0?
Attachment #524821 - Flags: approval1.9.2.18?
Attachment #524821 - Flags: approval1.9.1.20?
This doesn't apply cleanly to trunk...
Keywords: checkin-needed
(Assignee)

Comment 4

6 years ago
Created attachment 525488 [details] [diff] [review]
hg diff against trunk
(Assignee)

Comment 5

6 years ago
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 ?
Keywords: checkin-needed
http://hg.mozilla.org/projects/cedar/rev/00ebe89c7391
Keywords: checkin-needed
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla6
http://hg.mozilla.org/mozilla-central/rev/00ebe89c7391
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
(Assignee)

Updated

6 years ago
Blocks: 650665
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
Attachment #524821 - Flags: approval2.0?
Attachment #524821 - Flags: approval2.0+
Attachment #524821 - Flags: approval1.9.2.18?
Attachment #524821 - Flags: approval1.9.2.18+
Attachment #524821 - Flags: approval1.9.1.20?
Attachment #524821 - Flags: approval1.9.1.20+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Comment on attachment 524821 [details] [diff] [review]
add krb5 and crypto libs to OS_LIBS on OpenBSD

Didn't make 3.6.18
Attachment #524821 - Flags: approval1.9.2.18+ → approval1.9.2.18-
(Assignee)

Comment 10

6 years ago
And what's needed to get that commited ? approval-xxx + checkin-needed is not enough now ?
(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...
(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.
Keywords: checkin-needed
(Assignee)

Comment 13

6 years ago
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.
Attachment #524821 - Flags: approval1.9.2.20?
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
Attachment #524821 - Flags: approval1.9.2.20? → approval1.9.2.21+
Keywords: checkin-needed
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
status1.9.2: --- → .21-fixed
Keywords: checkin-needed
(Assignee)

Comment 16

4 years ago
Backed out 2 years later for causing SIGBUS in bug 853364. Gotta find a better workaround using dlopen() at runtime ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 17

3 years ago
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

3 years ago
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.
(Assignee)

Comment 19

3 years ago
Created attachment 8376835 [details] [diff] [review]
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...
Attachment #8376835 - Flags: review?(khuey)
(Assignee)

Comment 20

3 years ago
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 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.
Attachment #8376835 - Flags: review?(khuey) → review+
(Assignee)

Comment 22

3 years ago
Created attachment 8382529 [details] [diff] [review]
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)
Attachment #8382529 - Flags: review?(khuey)
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
Attachment #8382529 - Flags: review?(khuey) → review+
(Assignee)

Comment 24

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc49d96e84a
(Assignee)

Comment 25

3 years ago
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.
Attachment #8382529 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/7fc49d96e84a
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.