Password-less NTLM authentication via Samba (single sign-on)

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In the latest versions of Samba, when a user logs into Linux via Samba's Active Directory PAM bridge, Samba can perform NTLM authentication on behalf of applications using that user's stored credentials. Thus, applications that support this can perform NTLM authentication without requesting a password.

I will attach a patch that implements this support in Necko.
Created attachment 247144 [details] [diff] [review]
patch

The patch is not complex. It just spawns Samba's ntlm_auth helper tool. If the helper is not available or fails in some way, we fall back to the builtin NTLM support, which will work fine except it must request a password.

My main concern with this patch is that it pulls in some Unix-specific IPC code. Any suggestions on how this should be configured/rewritten?
Attachment #247144 - Flags: review?(cbiesinger)
+++ extensions/auth/nsAuthSambaNTLM.cpp	14 Aug 2006 07:51:33 -0000
+ * Portions created by the Initial Developer are Copyright (C) 2005

200_5_?

+    // ntlm_auth reads from stdin regularly so closing our file handles
+    // should cause it to exit. Trying to kill it with a signal would be
+    // problematic because another process could have spawned with that pid
+    // if it has unexpectedly quit.

So, that comment is wrong, since the process can't actually die until you
waited for it. But closing the FDs should be sufficient indeed.

+        // XXX this isn't 100% safe because the process could conceivably have
+        // died and some other Gecko subsystem might have spawned a new child
+        // with the same PID. If that child process was waiting for a message
+        // from Gecko, we coud deadlock while waiting for it to exit.
+        // But what else can I do here?

This one is wrong too.

+    // Make stdin read from toChildPipe
+    int result0 = dup2(toChildPipe[0], 0);
+    // Make stdout write to fromChildPipe
+    int result1 = dup2(fromChildPipe[1], 1);

I'd close the old fds (toChildPipe[0], fromChildPipe[1])

+    nsPromiseFlatCString flat(aString);
+    const char* s = flat.get();

You could avoid the flat string by just doing:
  const char* s = aString.BeginReading();
(you don't need nulltermination here)

Looks like the same is true for ExtractMessage

+    return NS_REINTERPRET_CAST(PRUint8*, PL_Base64Decode(flat.get() + 3, length - 4, nsnull));

Looks to me like you could just pass s here. Also, you already substracted 4
from length...

+    char* domain = strndup(username, slash - username);

strndup is a GNU extension

+    // We're done. Close our file descriptors now and reap the helper
+    // process.
+    Shutdown();
+    return NS_OK;

Shouldn't this return NS_SUCCESS_AUTH_FINISHED?

btw, the unix-specific IPC doesn't bother me. samba is unix-specific too.
guess the question is whether this compiles on something like OS/2...
Comment on attachment 247144 [details] [diff] [review]
patch

r=biesi with that stuff fixed
Attachment #247144 - Flags: review?(cbiesinger) → review+
Created attachment 255849 [details] [diff] [review]
updated patch

It seemed like the best thing to do is to rewrite the IPC code to use NSPR and avoid any new configure tests or complex makefile options. So I did that. The other requested changes have been made too.
Attachment #247144 - Attachment is obsolete: true
Attachment #255849 - Flags: superreview?
Attachment #255849 - Flags: superreview? → superreview?(cbiesinger)
Comment on attachment 255849 [details] [diff] [review]
updated patch

+        LOG(("ntlm_auth exec failure"));

might be useful to log the reason for the failure too (PR_GetError())
Attachment #255849 - Flags: superreview?(cbiesinger) → superreview+
Thanks. Checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 9

7 years ago
This doesn't seem to work, or I'm missing something.

I have ntlm_auth installed and the related prefs set on Firefox. However since execve is called without the complete path to the binary the call always fails.

Output from strace -f:

[pid  7459] execve("ntlm_auth", ["ntlm_auth", "--helper-protocol", "ntlmssp-client-1", "--use-cached-creds", "--username", "user1"], [/* 46 vars */]) = -1 ENOENT (No such file or directory)
[pid  7459] exit_group(1)   

From SpawnNTLMAuthHelper:

 char* args[] = {
        "ntlm_auth",
        "--helper-protocol", "ntlmssp-client-1",
        "--use-cached-creds",
        "--username", const_cast<char*>(username),
        nsnull
    };


The call stack seems to be as follows:

nsSambaNTLMAuthConstructor(nsISupports *outer, REFNSIID iid, void **result)
nsAuthSambaNTLM::SpawnNTLMAuthHelper()
SpawnIOChild(args, &mChildPID, &mFromChildFD, &mToChildFD)
PR_CreateProcess(aArgs[0], aArgs, nsnull, attr);

Environment:

Ubuntu 10.04
Firefox 3.6.18 (default package from Canonical)

Comment 10

7 years ago
Note that 

PR_CreateProcess(aArgs[0], aArgs, nsnull, attr);

uses the first arg ("ntlm_auth") as the executable path. This would only work if it was located in the filesystem (eg: type ntlm_auth) before being used.

Comment 11

7 years ago
It is confirmed to be a path problem. A symlink like

/home/user/ntlm_auth -> /usr/bin/ntlm_auth

makes it work.

Comment 12

7 years ago
I can work on a patch, but how do you want to have this fixed? Perform a path lookup? Set a pref with the path prefix?
ntlm_auth needs to be in your PATH. So, add the directory it's in to your PATH.

Comment 14

7 years ago
@Robert

ntlm_auth is in the PATH. But the system call the Firefox uses to call it does NOT care about the PATH. Can you take a quick look at the manpage of execve to see what I mean?
OK, here's what I think we need to do:

-- Get the nsExternalHelperAppService
-- Call nsExternalHelperAppService::GetFileTokenForPath to get an nsIFile ntlm_auth
-- Use that file's absolute path to create the process

Comment 17

7 years ago
Thanks. Will look into it.

Comment 18

7 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> OK, here's what I think we need to do:
> 
> -- Get the nsExternalHelperAppService
> -- Call nsExternalHelperAppService::GetFileTokenForPath to get an nsIFile
> ntlm_auth
> -- Use that file's absolute path to create the process

Hi @Robert,

your suggestion is using in the file extensions/auth/nsAuthSamba.cpp something like this expression?

nsresult nsExternalHelperAppService::GetFileTokenForPath ( const PRUnichar* platformAppPath,nsIFile ** aFile)

How can i put the path into the nsIFile?

Thanks in advanced,
Susana
Something like
  nsCOMPtr<nsIExternalHelperAppService> service =
    do_GetService(NS_EXTERNALHELPERAPPSERVICE_CONTRACTID, &rv);
  NS_ENSURE_SUCCESS(rv, rv);
  NS_NAMED_LITERAL_STRING(name, "ntlm_auth");
  nsCOMPtr<nsIFile> file;
  rv = service->GetFileTokenForPath(name.get(), getter_AddRefs(file));
  NS_ENSURE_SUCCESS(rv, rv);
  nsCAutoString nativePath;
  rv = file->GetNativePath(&nativePath);
  NS_ENSURE_SUCCESS(rv, rv);
  args[0] = nativePath.get();

Comment 20

7 years ago
The problem is that nsIExternalHelperAppService does not expose GetFileTokenForPath. I've checked and it is not present on the IDL for nsIExternalHelperAppService even though it is implemented in the class nsExternalHelperAppService.

Seems that the method is there but not as the implementation of an interface. Furthermore we couldn't find similar code in the Firefox tree.

What do you suggest?
You can expose it in the IDL, just add it as a method to the IDL and change the signature of the existing method to match (using NS_IMETHODIMP). I think the IDL would be

  nsIFile getFileTokenForPath(in wstring aPath);

Comment 22

7 years ago
Hi,

the new development in the file nsAuthSambaNTLM.cpp with the class nsIExternalHelperAppService give an error when i try to run "make"(error:the file or directory does not exists). 

So, to make it work it's necessary to add a new path to "make" command (in this directory).
I'm try to add a new path (../../uriloader/exthandler) that should be called by "make" in the (....)/build-tree/mozilla/extensions/auth. Something like this: c++ -o nsAuthSambaNTLM.o -c -I../../uriloader/exthandler -I../../dist/system_wrappers


I've been watching to the Makefile (in: (....)/build-tree/mozilla/extensions/auth(Makefile.in) and to the configure.mk (in: (...)/(....)/build-tree/mozilla/config/) but i didn't understand where i can make this change. Can you help me? 

Thanks in advanced,
Susana
You shouldn't need to mess with makefiles. Have you tried doing a clean build with your changes? Is it failing to find nsIExternalHelperAppService.h? That should be at dist/include/nsIExternalHelperAppService.h, and that directory should be on the include path when building nsAuthSambaNTLM.o.
You need to log in before you can comment on or make changes to this bug.