GNOME Bugzilla – Bug 650940
NTLM single-sign-on support in libsoup
Last modified: 2011-08-04 12:22:27 UTC
When using Samba's 'winbind' dæmon, NTLM single-sign-on is supported by delegating the NTLM challenge/response protocol to a helper in /usr/bin/ntlm_auth. We're working on a gnome-keyring extension which provides the same facility, with the same interface. And there's a really simple test hack at http://david.woodhou.se/ntlm_auth_v2.c which does the same thing. We need support for this in libsoup, especially for things like Evolution-EWS which use libsoup for communication with the mail server.
Created attachment 188436 [details] [review] Patch (from Mandy.Wu@intel.com) to implement NTLM single-sign-on using /usr/bin/ntlm_auth This is a patch to implement SSO support, tested (in libsoup 2.32) with Evolution-EWS. The communication with ntlm_auth is blocking, for now. In fact the responses will always come immediately, so that's not really an issue in practice. But a second patch is being worked on, to handle the response from the ntlm_auth pipe asynchronously.
Comment on attachment 188436 [details] [review] Patch (from Mandy.Wu@intel.com) to implement NTLM single-sign-on using /usr/bin/ntlm_auth Is it possible to do the child process stuff using glib APIs? Eg, g_spawn_async_with_pipes(). I notice that you use socketpair() rather than pipe(), but it looks you don't actually *need* bidirectionality; you're just doing it so you only have one fd to keep track of instead of two. (If it's almost-but-not-quite possible to make it work with gspawn, then it would be nice to just add some GSpawnFlags or whatever to make it fully possible.) >+#define NTLM_AUTH_HELPER "/usr/bin/ntlm_auth" This should be handled via AC_PATH_PROG(). Also, there should be a comment somewhere in the file giving a URL to the documentation of the protocol used to communicate with that program. >+ arg_username = g_strdup_printf ("--username '%s'", username); >+ execl (NTLM_AUTH_HELPER, "--helper-protocol ntlmssp-client-1", "--use-cached-creds", arg_username, arg_domain, NULL); This is gibberishy. You only need to put quotes around arguments if you are passing them to the shell. And passing "--username" and the username as a single exec argument with a space between them, rather than as two separate arguments, shouldn't work anyway. Unless /usr/bin/ntlm_auth is a badly-written shell script or something...
(In reply to comment #2) > (From update of attachment 188436 [details] [review]) > Is it possible to do the child process stuff using glib APIs? Eg, > g_spawn_async_with_pipes(). I notice that you use socketpair() rather than > pipe(), but it looks you don't actually *need* bidirectionality; you're just > doing it so you only have one fd to keep track of instead of two. Actually I does need bidirectionality, since I need send input to ntlm_auth, and get back its output. pipe() is unidirectional, can't do that. > (If it's almost-but-not-quite possible to make it work with gspawn, then it > would be nice to just add some GSpawnFlags or whatever to make it fully > possible.) It looks possible. I'd like to implement that after non-blocking patch is done, since I am in the middle of non-blocking coding. > >+#define NTLM_AUTH_HELPER "/usr/bin/ntlm_auth" > This should be handled via AC_PATH_PROG(). > Also, there should be a comment somewhere in the file giving a URL to the > documentation of the protocol used to communicate with that program. > >+ arg_username = g_strdup_printf ("--username '%s'", username); > >+ execl (NTLM_AUTH_HELPER, "--helper-protocol ntlmssp-client-1", "--use-cached-creds", arg_username, arg_domain, NULL); > This is gibberishy. You only need to put quotes around arguments if you are > passing them to the shell. And passing "--username" and the username as a > single exec argument with a space between them, rather than as two separate > arguments, shouldn't work anyway. Unless /usr/bin/ntlm_auth is a badly-written > shell script or something... just follow ntlm_auth arguments format...
Created attachment 189440 [details] [review] Add support for NTLM single-sign-on using /usr/bin/ntlm_auth Modified according to Dan and David's feedbacks.
Please ignore this patch. I sent it by mistake. Sorry.
Created attachment 189444 [details] [review] Updated patch to implement NTLM single-sign-on using /usr/bin/ntlm_auth This is modified according to Dan and David's feedbacks. Please help review it. Thanks in advance.
I don't like the way you're checking for ntlm_auth and refusing to include the support if it's not installed at build time. You should fall back to using the path /usr/bin/ntlm_auth. That is a known path, part of the defined interface for this helper. Think of it like /usr/lib/sendmail. Have a way to override it to point elsewhere in strange configurations, by all means, but don't rely on it being there at *build* time just to include this support in libsoup.
The protocol is partly documented (this is actually the server side, not the client side) at http://devel.squid-cache.org/ntlm/squid_helper_protocol.html
Created attachment 189959 [details] [review] patch to implement NTLM single-sign-on using /usr/bin/ntlm_auth Add --with-ntlm-auth to configure script. Delete checking path for ntlm_auth, instead use /usr/bin/ntlm_auth by default unless user use --with-ntlm-auth to set it to other place.
Created attachment 189964 [details] [review] patch to implement NTLM single-sign-on using /usr/bin/ntlm_auth Add --with-ntlm-auth to configure script. Delete checking path for ntlm_auth, instead use /usr/bin/ntlm_auth by default unless user use --with-ntlm-auth to set it to other place.
Comment on attachment 189964 [details] [review] patch to implement NTLM single-sign-on using /usr/bin/ntlm_auth >+AC_ARG_WITH(ntlm-auth, this needs to default to off/no/false on Windows (ie, if test $os_win32 = yes) >+typedef struct { >+ gint fd_in; >+ gint fd_out; >+ GPid pid; >+} NTLMHelper; "int", not "gint". (Likewise everywhere else.) >+ NTLMHelper *sso_data; >+#endif > } SoupNTLMConnection; You don't really need NTLMHelper; you should just add those fields directly to SoupNTLMConnection (and then pass the connection rather than the sso_data as the user_data to the various callbacks). >+ sleep (1); >+ break; You can't do this; that will cause the main thread to block. I thought we had decided that ntlm_auth never blocks? >+ if (access (NTLM_AUTH, X_OK)) >+ goto done; Say "if (access (NTLM_AUTH, X_OK) != 0)". Otherwise if you're skimming the code quickly it looks like we goto done if the file *is* accessible. >+ /* Add watch function to catch termination of the process */ >+ g_child_watch_add (sso_data->pid, (GChildWatchFunc) sso_child_watch, sso_data); (a) This will not necessarily work when using SoupSessionSync, since there's no guarantee that a main loop is running in that case. (b) It won't work when using a non-default GMainContext either, because it's always adding the source to the default context. I'm not sure you really need this anyway; you should be able to just let gspawn handle reaping the child, and you can detect when it has exited because you'll get a 0 read or EPIPE when reading/writing to it, and you can then restart it if necessary. This should work for sync or async. >+ if (domain) >+ g_free (domain); g_free() checks if (!NULL), so you don't need the "if (domain)" >+sso_ntlm_response (NTLMHelper *sso_data, const char* input, SoupNTLMState conn_state) "const char *input", not "const char* input" >+ if (write (sso_data->fd_in, input, strlen (input)) < 0 || >+ (size = read (sso_data->fd_out, buf, 1024)) < 5 ) { This is not interrupt-safe; read() and write() will return -1/EINTR if they are interrupted by a signal, and you need to retry in that case. (This is another reason why using higher-level glib/gio functions is nice.) >+ response = g_strndup (buf + 3, size - 4); >+ *(response + size - 4) = '\0'; g_strndup() already '\0'-terminates its result >+ response = g_strconcat ("NTLM ", response, NULL); You can combine these two into one: response = g_strdup_printf ("NTLM %.*s", size - 4, buf + 3);
Created attachment 190445 [details] [review] patch to implement NTLM single-sign-on using /usr/bin/ntlm_auth Thanks Dan for reviewing last patch in details. Here is the new patch modified according to the feedbacks.
Created attachment 190731 [details] [review] patch to implement NTLM single-sign-on using /usr/bin/ntlm_auth (against upstream code)
Created attachment 190764 [details] [review] patch to implement NTLM single-sign-on using /usr/bin/ntlm_auth Fix string array increment error.
Comment on attachment 188436 [details] [review] Patch (from Mandy.Wu@intel.com) to implement NTLM single-sign-on using /usr/bin/ntlm_auth (marking older patch obsolete)
Comment on attachment 190764 [details] [review] patch to implement NTLM single-sign-on using /usr/bin/ntlm_auth Please attach the patch as "git format-patch" output (see https://live.gnome.org/Git/Developers#Contributing_patches) Do you have GNOME git access or will you need me to commit the final version? >+AC_ARG_WITH(ntlm-auth, >+ AC_HELP_STRING([--with-ntlm-auth=PATH],[Where to look for ntlm_auth, path points to ntlm_auth installation (default: /usr/bin/ntlm_auth);]) >+ AC_HELP_STRING([--without-ntlm-auth], [disable ntlm single-sign-on by using ntlm_auth]), You can't include two AC_HELP_STRING arguments (look at the "./configure --help" output; the second one doesn't line up right). But the existence of a --without flag is implied anyway, so you can just delete that line. >+static void >+sso_ntlm_close (SoupNTLMConnection *conn) Is it actually necessary to wait for ntlm-auth to die? You've closed its stdin and stdout, so it will eventually try to read something and get a SIGHUP, or write something and get a SIGPIPE, and die on its own. (And then you don't need the pid field at all.) >+ if (access (NTLM_AUTH, X_OK) != 0) >+ goto done; You should do this in soup_auth_manager_ntlm_init(), and remember the result, so you don't have to re-test every time sso_ntlm_initiate() gets called. >+ /* Samba/winbind installed but not configured */ >+ if (conn_state == SOUP_NTLM_NEW && >+ g_ascii_strcasecmp (buf, "PW") == 0) { >+ response = g_strdup ("PW"); >+ goto done; >+ } That comment is only true if the if statement evaluates to true, so it should be inside the {}s, not before the if. (Likewise with the one just below it.) >+ response = g_strdup_printf ("NTLM %.*s", size - 4, buf + 3); soup-auth-manager-ntlm.c: In function 'sso_ntlm_response': soup-auth-manager-ntlm.c:395:2: warning: field precision specifier '.*' expects argument of type 'int', but argument 2 has type 'ssize_t' [-Wformat]
(In reply to comment #16) > (From update of attachment 190764 [details] [review]) > Please attach the patch as "git format-patch" output (see > https://live.gnome.org/Git/Developers#Contributing_patches) Will use it. > Do you have GNOME git access or will you need me to commit the final version? I have GNOME account, but I have some problem with access the git rep using it. I will try to resolve it. I might still need your help to commit the final version. :) > You can't include two AC_HELP_STRING arguments (look at the "./configure > --help" output; the second one doesn't line up right). But the existence of a > --without flag is implied anyway, so you can just delete that line. Fixed. > Is it actually necessary to wait for ntlm-auth to die? You've closed its stdin > and stdout, so it will eventually try to read something and get a SIGHUP, or > write something and get a SIGPIPE, and die on its own. (And then you don't need > the pid field at all.) Yes, you are right. My initial thought is to explicitly close/free everything when not needed. > >+ if (access (NTLM_AUTH, X_OK) != 0) > >+ goto done; > > You should do this in soup_auth_manager_ntlm_init(), and remember the result, > so you don't have to re-test every time sso_ntlm_initiate() gets called. Modified. > That comment is only true if the if statement evaluates to true, so it should > be inside the {}s, not before the if. (Likewise with the one just below it.) Fixed. > >+ response = g_strdup_printf ("NTLM %.*s", size - 4, buf + 3); > > soup-auth-manager-ntlm.c: In function 'sso_ntlm_response': > soup-auth-manager-ntlm.c:395:2: warning: field precision specifier '.*' expects > argument of type 'int', but argument 2 has type 'ssize_t' [-Wformat] why I am not getting this warning? Is it because I am using 32bit machine, yours are 64bit?
(In reply to comment #17) > > >+ response = g_strdup_printf ("NTLM %.*s", size - 4, buf + 3); > > > > soup-auth-manager-ntlm.c: In function 'sso_ntlm_response': > > soup-auth-manager-ntlm.c:395:2: warning: field precision specifier '.*' expects > > argument of type 'int', but argument 2 has type 'ssize_t' [-Wformat] > why I am not getting this warning? Is it because I am using 32bit machine, > yours are 64bit? yeah, I guess. just doing (int)(size - 4) should fix it
Created attachment 193079 [details] [review] Support NTLM Single Sign On Finish all the changes and generate patch with git format-patch HEAD^. I just build my box with Fedora 15, and still got problem with my GNOME account. Dan, could you please help commit it if you think the patch needs no more fixing?
Committed and pushed to master. Sorry this took so long. Attachment 193079 [details] pushed as ade9d54 - Support NTLM Single Sign On