Re: secur32: Getting some real functionality into the NTLM provider, try 2

[Juan Lang]

Hi Kai,

In ntlm.c,
+    /* convert the program name to Unicode */
(snip)
+    /* and back to the CP_UNIXCP */

This round-tripping is unnecessary when the text is already ASCII.  ASCII
values are just as valid in UTF8 as they are in CP1252 or whatever. 
Basically, don't bother converting anything you can type.

+            if(pAuthData == NULL)
In this if statement, you duplicate the business of converting the user
name and domain name to CP_UNIXCP in both branches.  You should instead
store a pointer to the user name and domain name, and convert them in one
common branch after the if statement.

+            SEC_CHAR *client_argv[] = 
+            { 
+                program,
+                client_protocol,
+                client_user_arg,
+                client_domain_arg,
+                NULL 
+            };

You must declare variables at the beginning of the block, not in the
middle.  Not all C compilers allow this.

+    if(pszPackage != NULL)
+    {
+        int package_sizeW = MultiByteToWideChar(CP_ACP, 0, pszPackage,
-1,
+                NULL, 0);

Since you don't use pszPackage in ntlm_AcquireCredentialsHandleW, why
bother converting it?  Just pass NULL.

+        if(identity->Flags == SEC_WINNT_AUTH_IDENTITY_ANSI)
(snip)
+        else
+        {
+            memcpy(pAuthDataW, identity,
sizeof(SEC_WINNT_AUTH_IDENTITY_W));
+        }

This looks funny.  Why not just set pAuthDataW to identity in this case? 
That way you avoid the memcpy.  Something like this:
PSEC_WINNT_AUTH_IDENTITY_W pAuthDataW;

if (identity->Flags == SEC_WINNT_AUTH_IDENTITY_ANSI)
{
    /* convert it like you do */
}
else
    pAuthDataW = (PSEC_WINNT_AUTH_IDENTITY_W)identity;
/* call ntlm_AcquireCredentialsHandleW */
if (pAuthDataW != identity)
    HeapFree(GetProcessHeap(), 0, pAuthDataW);

+                    if((ret = encodeBase64(helper->password, 
+                                helper->pwlen-2, buffer+3,
+                                max_len-3, &buffer_len)) != SEC_E_OK)
+                   {
+                        HeapFree(GetProcessHeap(), 0, buffer);
+                        HeapFree(GetProcessHeap(), 0, bin);
+                        return ret;
+                    }

You ought to zero the memory of the password and free it, regardless of
whether encodeBase64 succeeds or fails.  (Really you ought to call
SecureZeroMemory, but wine hasn't implemented that.. oh well.)

In tests/ntlm.c:
+        //lstrcpyA(identity->Password, pass ? pass :"");

No C++ style comments, please.

More generally, on the tests, I like them because they should nearly
always succeed in Windows.   But they're not very likely to succeed in
Wine, are they?  You handle not having an ntlm_auth fairly gracefully, I
think, and also handle not being able to create an inbound context, which
you say may not work without some magic setup.

But this won't generally work out of the box if ntlm_auth works and an
inbound context can be created, can it?  The tests will try to
authenticate with whatever domain name is in the registry, or default to
WORKGROUP if that's empty, and your UNIX username, and no password. 
That's not very likely to succeed.

So, it would be nice to have these tests somewhere for people to check
that this code indeed works.. but I'm not sure they can go into the
regression test suite.  Unless I'm mistaken.

Thanks,
--Juan

____________________________________________________
Start your day with Yahoo! - make it your home page 
http://www.yahoo.com/r/hs