curl-library
RE: Request to review the code changes for NTLMv2 Support in Curl
Date: Sat, 25 Jan 2014 15:54:39 +0000
On Fri, 24 Jan 2014, Prash Dush wrote:
> We have incorporated the comments provided by you.
Many thanks for the update.
> Could you please help us in writing the helper for Big Endian for
> the below line:
I have attached 7 GIT .patch files which are:
1) Prash's updated .diff as a .patch file with appropriate commit comments,
timestamp and author
2) Copyright information updated to 2014 for these updated files
3) Corrections to the usage of LLU for Visual Studio to ULL - See [1], [2]
and [3] for more information
4) Corrects the #include order as "memdebug.h" should be the last header
file to be included
5) Moved the #defines to the top of the source - as personally I don't like
to see local #defines in functions
6) Minor tidy up to replace NULL comparison to not operator, corrections to
whitespace and update of comments and function arguments to make better
usage of the 80 char limit as well as consistency
7) Implemented endian agnostic support for writing the timestamp into the
message buffer - hopefully I have this correct ;-)
I have these patches in a local branch here and am ready to apply them after
the pending release - unless anyone else has any other comments. Note: I
would recommend we combine some of the first 6 patches into a single patch
as I don't think there is any need to show the copyright, and other minor
corrections as separate commits but I wanted to list them individually here
so everyone can see the differences.
I have tested this with an instance of Microsoft Exchange Server
authenticating an SMTP log in and compared the message generation to that of
the original NTLMv1 code as well as Windows SSPI generated messages.
As this update will change the message format in the next release, it will
break our existing NTLM test harnesses as they stand. Given this I think we
should consider:
Option 1:
Update the generated Type 3 message in the existing test harnesses to
contain the extra NTLMv2 information.
Option 2:
a) Update the Type 2 server response in the existing tests so they don't
include the target Information - This will then force curl to generate a
NTLMv1 type 3 message rather than a NTLMv2 message.
b) Add new NTLMv2 tests that include this target Information in the Type 2
server response so curl generates a type 3 messages containing the NTLMv2
information. The new tests would obviously have this type 3 message in them
rather than the existing type 3 ;-)
Option 3:
a) Add support for USE_NTLM_V2 so that developers can turn v2 support on or
off
b) Add "NTLMv2" as a string in the curl features list - as displayed with
"curl --version"
c) Add some NTLM v2 tests
I don't think there is any need for option 3, as it makes the build more
complicated and introduces more build variants... It also means that if we
update our NTLM support in the future to more closely match the output
generated by Windows SSPI (I'm not too sure what those differences are at
the moment) then the build variants would also increase then as well.
My own preference is probably option 2 - although I'm not sure how we take
the existing type 2 response and remove the target information from it. As
such option 1 will be the easiest and least amount of work but it doesn't
test the differences in code / message generation :( I'm sure I / we can
figure this out - for example, start by base 64 decoding the string, seeing
what the generated binary output is, modify the flags (if need be), update
the target info header to have a size of zero, find the offset to the target
info, remove it, base64 encode the result and see if curl will decode it !!
As a timestamp is included in the NTLMv2 information - the code will need a
minor tweak so that this timestamp is consistent in DEBUG builds and doesn't
vary - similar to what I have done with the MD5-DIGEST tests [4] for IMAP,
POP3 and SMTP. This will mean that the same timestamp is used under debug
builds so that the message generation is consistent.
Kind Regards
Steve
[1] =
http://connect.microsoft.com/VisualStudio/feedback/details/774715/llu-suffix
-not-recognized
[2] = http://curl.haxx.se/mail/lib-2012-03/0263.html
[3] = I believe GCC and other compilers support both but we should probably
verify whether this is the best fix
[4] = curl_sasl.c line 354 in Curl_sasl_create_digest_md5_message()
Received on 2014-01-25