Add unit tests for NTLMv1 portable implementation

Add unit tests for existing NTLM portable code.

- Adds tests for existing implementation.
- Minor code change to reject certain invalid messages.

NOTE: The existing code just ignored the target name field if it
had length and/or offset outside the bounds of the message. This
change explicitly rejects that case in order to test that the
behavior is correct.

BUG=chromium:22532
TEST=builds and tests pass

Review-Url: https://codereview.chromium.org/2873673002
Cr-Commit-Position: refs/heads/master@{#486604}
Committed: https://chromium.googlesource.com/chromium/src/+/b89acda495056e4af04adc1b34774bc3069f612a

[zentaro, 2017-05-09 15:33:32]

The CQ bit was checked by zentaro@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 15:34:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zentaro, 2017-05-09 15:49:40]

Description was changed from

==========
NTLM tests in progress

BUG=
==========

to

==========
Add unit tests for existing NTLM portable code.

- Adds tests for existing implementation.
- Minor code change to reject certain invalid messages.

NOTE: The existing code just ignored the target name field if it
had length and/or offset outside the bounds of the message. This
change explicitly rejects that case in order to test that the
behavior is correct.

BUG=22532
TEST=builds and tests pass
==========

[zentaro, 2017-05-09 16:01:23]

zentaro@chromium.org changed reviewers:
+ asanka@chromium.org, rsleevi@chromium.org

[zentaro, 2017-05-09 16:01:30]

PTAL

[Ryan Sleevi, 2017-05-09 20:49:27]

Have you considered writing a fuzzer as well for these message generation
functions?

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:42:
base::ASCIIToUTF16("badpassword"));
Any reason for the SetUp vs CTOR?

(See
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...
)

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:46: GURL gurl("foo.com");
Since you're creating a GURL, let's use a proper URL - e.g. https://foo.com

Implicitly, you're relying on GURL to guess what you want, so better to make it
explicit (and this also makes it clear it's HTTPS vs, currently, implicitly HTTP
- although if you meant HTTP, better to say that :P)

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:54: std::string
CreateType2Token(const char* message, size_t message_len) {
Consider using base::StringPiece ?

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:62: void
HandleAnotherChallenge(std::string challenge) {
Pass this by const-ref? Or did you mean to explicitly make a copy?

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:72: GetCreds(),
&request_info, callback.callback(), token));
There's no way to wait for the callback's result since you're deleting it on 69,
right?

I'm not entirely sure it's safe here, but I haven't dug in yet. Just trying to
flag it early in case you knew it was safe

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:75: int GenerateAuthToken()
{
Why this overload? This is a fairly surprising naming (compared with 68).

It also seems dangerous from a security perspective - if GenerateAuthToken
completes asynchronously, |*token| will be invalidated (because the stack frame
at 76 will have cleaned it up) - that doesn't seem good.

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:137: // Chrome's type 1
message.
How was this computed? What are the values? How can it be reconstructed?

[zentaro, 2017-05-15 20:29:30]

On 2017/05/09 20:49:27, Ryan Sleevi wrote:
> Have you considered writing a fuzzer as well for these message generation
> functions?
> 
>
https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
> File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):
> 
>
https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
> net/http/http_auth_handler_ntlm_portable_unittest.cc:42:
> base::ASCIIToUTF16("badpassword"));
> Any reason for the SetUp vs CTOR?
> 
> (See
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...
> )
> 
>
https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
> net/http/http_auth_handler_ntlm_portable_unittest.cc:46: GURL gurl("foo.com");
> Since you're creating a GURL, let's use a proper URL - e.g. https://foo.com
> 
> Implicitly, you're relying on GURL to guess what you want, so better to make
it
> explicit (and this also makes it clear it's HTTPS vs, currently, implicitly
HTTP
> - although if you meant HTTP, better to say that :P)
> 
>
https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
> net/http/http_auth_handler_ntlm_portable_unittest.cc:54: std::string
> CreateType2Token(const char* message, size_t message_len) {
> Consider using base::StringPiece ?
> 
>
https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
> net/http/http_auth_handler_ntlm_portable_unittest.cc:62: void
> HandleAnotherChallenge(std::string challenge) {
> Pass this by const-ref? Or did you mean to explicitly make a copy?
> 
>
https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
> net/http/http_auth_handler_ntlm_portable_unittest.cc:72: GetCreds(),
> &request_info, callback.callback(), token));
> There's no way to wait for the callback's result since you're deleting it on
69,
> right?
> 
> I'm not entirely sure it's safe here, but I haven't dug in yet. Just trying to
> flag it early in case you knew it was safe
> 
>
https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
> net/http/http_auth_handler_ntlm_portable_unittest.cc:75: int
GenerateAuthToken()
> {
> Why this overload? This is a fairly surprising naming (compared with 68).
> 
> It also seems dangerous from a security perspective - if GenerateAuthToken
> completes asynchronously, |*token| will be invalidated (because the stack
frame
> at 76 will have cleaned it up) - that doesn't seem good.
> 
>
https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
> net/http/http_auth_handler_ntlm_portable_unittest.cc:137: // Chrome's type 1
> message.
> How was this computed? What are the values? How can it be reconstructed?

PTAL

[zentaro, 2017-05-15 20:48:38]

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:42:
base::ASCIIToUTF16("badpassword"));
On 2017/05/09 20:49:27, Ryan Sleevi wrote:
> Any reason for the SetUp vs CTOR?
> 
> (See
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...
> )

Nope. I just copied from the Negotiate one as a starting point. Moved to
constructor.

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:46: GURL gurl("foo.com");
On 2017/05/09 20:49:27, Ryan Sleevi wrote:
> Since you're creating a GURL, let's use a proper URL - e.g. https://foo.com
> 
> Implicitly, you're relying on GURL to guess what you want, so better to make
it
> explicit (and this also makes it clear it's HTTPS vs, currently, implicitly
HTTP
> - although if you meant HTTP, better to say that :P)

Done.

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:54: std::string
CreateType2Token(const char* message, size_t message_len) {
On 2017/05/09 20:49:27, Ryan Sleevi wrote:
> Consider using base::StringPiece ?

Done.

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:62: void
HandleAnotherChallenge(std::string challenge) {
On 2017/05/09 20:49:27, Ryan Sleevi wrote:
> Pass this by const-ref? Or did you mean to explicitly make a copy?

Done.

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:72: GetCreds(),
&request_info, callback.callback(), token));
On 2017/05/09 20:49:27, Ryan Sleevi wrote:
> There's no way to wait for the callback's result since you're deleting it on
69,
> right?
> 
> I'm not entirely sure it's safe here, but I haven't dug in yet. Just trying to
> flag it early in case you knew it was safe

Looks like TestCompletionCallback::GetResult checks if ERR_IO_PENDING is
returned, and if it is it blocks waiting for the callback to fire. So this
effectively forces it to appear synchronous to the test even if it wasn't. Am I
misunderstanding how this works? All the other tests seem to do the same thing.

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:75: int GenerateAuthToken()
{
On 2017/05/09 20:49:27, Ryan Sleevi wrote:
> Why this overload? This is a fairly surprising naming (compared with 68).
> 
> It also seems dangerous from a security perspective - if GenerateAuthToken
> completes asynchronously, |*token| will be invalidated (because the stack
frame
> at 76 will have cleaned it up) - that doesn't seem good.

As above I think this runs synchronously. The overload is because many of the
cases don't care about the token param. I renamed it GetGenerateAuthTokenResult.

https://codereview.chromium.org/2873673002/diff/80001/net/http/http_auth_hand...
net/http/http_auth_handler_ntlm_portable_unittest.cc:137: // Chrome's type 1
message.
On 2017/05/09 20:49:27, Ryan Sleevi wrote:
> How was this computed? What are the values? How can it be reconstructed?

It was just copied from a real transaction. I was just trying to add a known
good message as a sanity check.

In order to more easily add tests that actually poke all the various internal
fields that the server can send I have another CL -
https://codereview.chromium.org/2879353002 which provides some helper functions
to make reading and writing the messages easier. 

They will also be used in the actual implementation to consolidate all the
buffer manipulation primitives rather than the ad hoc buffer/cursor management
the current code does.

I was planning another set of additional tests once those helpers are available.
It's up to you if you want to land this minimal set of tests first or we can
wait until the above CL lands and I'll add the additional tests to this CL.

[zentaro, 2017-05-19 20:28:35]

Hold off on reviewing this one for a little bit. Making it more complete and
fixing some coding convention issues.

[Ryan Sleevi, 2017-06-08 18:47:44]

Mostly LG, but will defer to Asanka to make sure I haven't missed anything.

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client.cc
File net/http/ntlm_client.cc (right):

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client.c...
net/http/ntlm_client.cc:31: ZapBuf(buffer.get(), length);
https://codereview.chromium.org/2926333002/ :)

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client.c...
net/http/ntlm_client.cc:42: uint8_t key1[8];
Should we = {0}; these to ensure they're all zero-initialized? Same for all
these stack vars

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client_u...
File net/http/ntlm_client_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client_u...
net/http/ntlm_client_unittest.cc:22: uint8_t hash1[ntlm::NTLM_HASH_LEN];
= {0}; ?

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client_u...
net/http/ntlm_client_unittest.cc:29: EXPECT_TRUE(memcmp(hash1, hash2,
ntlm::NTLM_HASH_LEN) != 0);
EXPECT_EQ(0, memcmp(...))

[zentaro, 2017-06-12 23:16:55]

PTAL

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client.cc
File net/http/ntlm_client.cc (right):

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client.c...
net/http/ntlm_client.cc:31: ZapBuf(buffer.get(), length);
On 2017/06/08 18:47:43, Ryan Sleevi wrote:
> https://codereview.chromium.org/2926333002/ :)

Done.

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client.c...
net/http/ntlm_client.cc:42: uint8_t key1[8];
On 2017/06/08 18:47:43, Ryan Sleevi wrote:
> Should we = {0}; these to ensure they're all zero-initialized? Same for all
> these stack vars

I assumed it wasn't necessary since we do (should) write to every byte anyway.
If we are leaving part of our hash/crypto inputs uninitialized presumably we get
different hashes and fail anyway. If anything I could see scenarios where
zeroing might make it less likely to find a bug.

Is there some thing else I'm missing?


fn1() {
 byte arr[4] = {0}
 memcpy(arr, stuff, 3)
 return arr
}

fn2() {
 byte arr[4] = {0}
 memcpy(arr, stuff, 3)
 return arr
}

// Won't notice arr[3] never gets initialized with the correct value
assert(fn1() == fn2())

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client_u...
File net/http/ntlm_client_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client_u...
net/http/ntlm_client_unittest.cc:22: uint8_t hash1[ntlm::NTLM_HASH_LEN];
On 2017/06/08 18:47:43, Ryan Sleevi wrote:
> = {0}; ?

Per previous reply.

https://codereview.chromium.org/2873673002/diff/300001/net/http/ntlm_client_u...
net/http/ntlm_client_unittest.cc:29: EXPECT_TRUE(memcmp(hash1, hash2,
ntlm::NTLM_HASH_LEN) != 0);
On 2017/06/08 18:47:43, Ryan Sleevi wrote:
> EXPECT_EQ(0, memcmp(...))

Actually EXPECT_NE. But done and elsewhere.

[asanka, 2017-06-23 21:29:11]

https://codereview.chromium.org/2873673002/diff/460001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2873673002/diff/460001/net/BUILD.gn#newcode294
net/BUILD.gn:294: "http/ntlm_client.h",
I'm guessing this and the tests would be moved to //net/ntlm and would only be
added as a dependency to //net on platforms other than Windows.

https://codereview.chromium.org/2873673002/diff/460001/net/BUILD.gn#newcode4689
net/BUILD.gn:4689: "http/http_auth_handler_ntlm_portable_unittest.cc",
inside the 'test("net_unittests")' definition, add a condition to remove this
source file when 'is_win' is true.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:1: // Copyright (c) 2017
The Chromium Authors. All rights reserved.
No "(c)"

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:5: #include
"net/http/http_auth_handler_ntlm.h"
The primary include file should http_auth_handler_ntlm_portable.h.

If the tests depend on http_auth_handler_ntlm.h, then it should be included in
the general include section below.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:31: #if
defined(NTLM_PORTABLE)
This condition is not necessary and not correct.

The *_ntlm_portable_* stuff should only be getting built on platforms which use
it. It shouldn't be enforced here. 

Also, the NTLM_PORTABLE macro is used by HttpAuthHandlerNtlm to conditionalize
its implementation and shouldn't really be used outside of that. (I know it is,
but it shouldn't.)

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:56: std::string
CreateType2Token(base::StringPiece message) {
Nit: doesn't really create a type2 token. Probably call it something more
generic, like 'GenerateNtlmAuthHeader()' or something.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:63: void
HandleAnotherChallenge(const std::string& challenge,
It's easier to read, even though a bit verbose, if the function returned an
HttpAuth::AuthorizationResult.

I.e. the call site would look like:

   EXPECT_EQ(FOO, HandleAnotherChallenge(challenge));

That way it's clear what the input is and what the expected result is at the
call site. Also, it's much more convenient for the call site to switch to an
ASSERT_* if it doesn't make sense to proceed if this operation fails.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:92:
EXPECT_EQ(sec_buf.length, len);
Should return early if this winds up not being the case. Also, return a bool or
something that the calling function can ASSERT_* on since it's likely that the
test can't proceed past this failure. Here and elsewhere.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:140: static void
MockRandom(uint8_t* output, size_t n) {
Why not memset(output, 4, n) ? https://xkcd.com/221/

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:188: 
The remainder of the test is basically testing the NtlmBufferReader
implementation. :-/ The above comparison against a static string is sufficient
to verify the Type1 token generation.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:224:
"TlRMTVNTUAACAAAADAAMADgAAAAFgokCXziKeNIPIDYAAAAAAAAAAIYAhgBEAAAABgOAJQAA"
The token is irrelevant. Let's go with something shorter.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:236:
HandleAnotherChallenge(CreateType2Token(base::StringPiece(raw, sizeof(raw))));
The memory bots may complain about this because the generated token is likely
based on uninitialized memory. ("branch conditionalized on uninitialized value"
or somesuch).

Let's take a valid Type2 message and truncate it for this test.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:244: char raw[32];
Same as before. Use a known initialized buffer for tests.

Also, the failure could be due to anything. Let's start with a minimal valid
message, and test by changing individual fields thereof. That should increase
our confidence that the ERR_UNEXPECTED is due to the invalid signature.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:304:
ntlm::SecurityBuffer(ntlm::CHALLENGE_HEADER_LEN, 1)));
Note that you are relying on NtlmBufferWriter initializing the extra byte. This
is not the case in the dependent patchset. I added comment in the CL that
introduces NtlmBufferWriter to address that.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:372:
EXPECT_TRUE(reader.SkipSecurityBuffer());
SkipSecurityBufferWithValidation() ?

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:487:
ntlm::GenerateResponsesV1WithSS(base::ASCIIToUTF16(password_ascii_),
While this makes sense for now since the test is exercising two implementations,
it won't make sense when we cut over to a single implementation. At that point
we'd be verifying that the new HttpAuthHandlerNtlmPortable winds up calling
GenerateResponses* at some point. Beyond that, we are comparing the output of
GenerateResponses* with itself.

Let's  test against fixed hand verified static output rather than ones that are
generated by the implementation we are testing.

It's a bit hard to verify the output token against a fixed static token since
the protocol doesn't provide a canonical representation. So I'm not against
using a NtlmBufferReader. But at least the individual fields should be fixed.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:583: // Skip the session
key which isn't used.
?

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.cc
File net/http/ntlm_client.cc (right):

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.c...
net/http/ntlm_client.cc:109: GenerateNtlmHashV1(password, ntlm_hash);
Minor nit: It's easier to follow along with the spec if the names resembled
what's used in the spec. E.g. response_key_nt instead of ntlm_hash.

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h
File net/http/ntlm_client.h (right):

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h...
net/http/ntlm_client.h:23: namespace ntlm {
It's was a bit tricky to locate the spec for each of the functions implemented
below. Shall we add the correct references? That way it's easier for someone
trying to verify the correctness and compliance of the implementation.

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h...
net/http/ntlm_client.h:25: // Generates the NTLMv1 Hash and writes the 16 byte
result to |hash|
NTOWFv1() as defined in Section 3.3.1 of [MS-NLMP].

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h...
net/http/ntlm_client.h:30: // function in the NTLMSSP spec (Section 6 Appendix
A)
[MS-NLMP] rev 28 Section 6

(Section 6 is Appendix A. No need to specify both.)

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h...
net/http/ntlm_client.h:32: // |hash| must contain at least 16 bytes.
"at least" is worrisome wording for a buffer whose length is left unspecified.
Let's drop the "at least" and make sure we don't touch anything outside of that.
Here and elsewhere.

Also give these buffer sizes names so that both the calling code and the called
implementation can use the same constants that govern buffer sizes.

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client_u...
File net/http/ntlm_client_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client_u...
net/http/ntlm_client_unittest.cc:19: 
Please use the test vectors in section 4.2 (Cryptographic values for validation)
for these tests.

These tests are okay as smoke tests, but not sufficient to validate
cryptographic operations.

[zentaro, 2017-07-05 17:57:42]

https://codereview.chromium.org/2873673002/diff/460001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2873673002/diff/460001/net/BUILD.gn#newcode294
net/BUILD.gn:294: "http/ntlm_client.h",
On 2017/06/23 21:29:10, asanka wrote:
> I'm guessing this and the tests would be moved to //net/ntlm and would only be
> added as a dependency to //net on platforms other than Windows.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/BUILD.gn#newcode4689
net/BUILD.gn:4689: "http/http_auth_handler_ntlm_portable_unittest.cc",
On 2017/06/23 21:29:10, asanka wrote:
> inside the 'test("net_unittests")' definition, add a condition to remove this
> source file when 'is_win' is true.

Since I already added a condition for !is_win for the other test I add this
there rather than doing the inverse.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:1: // Copyright (c) 2017
The Chromium Authors. All rights reserved.
On 2017/06/23 21:29:10, asanka wrote:
> No "(c)"

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:5: #include
"net/http/http_auth_handler_ntlm.h"
On 2017/06/23 21:29:10, asanka wrote:
> The primary include file should http_auth_handler_ntlm_portable.h.
> 
> If the tests depend on http_auth_handler_ntlm.h, then it should be included in
> the general include section below.

There is no http_auth_handler_ntlm_portable.h so I moved this below.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:31: #if
defined(NTLM_PORTABLE)
On 2017/06/23 21:29:10, asanka wrote:
> This condition is not necessary and not correct.
> 
> The *_ntlm_portable_* stuff should only be getting built on platforms which
use
> it. It shouldn't be enforced here. 
> 
> Also, the NTLM_PORTABLE macro is used by HttpAuthHandlerNtlm to conditionalize
> its implementation and shouldn't really be used outside of that. (I know it
is,
> but it shouldn't.)

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:56: std::string
CreateType2Token(base::StringPiece message) {
On 2017/06/23 21:29:10, asanka wrote:
> Nit: doesn't really create a type2 token. Probably call it something more
> generic, like 'GenerateNtlmAuthHeader()' or something.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:63: void
HandleAnotherChallenge(const std::string& challenge,
On 2017/06/23 21:29:10, asanka wrote:
> It's easier to read, even though a bit verbose, if the function returned an
> HttpAuth::AuthorizationResult.
> 
> I.e. the call site would look like:
> 
>    EXPECT_EQ(FOO, HandleAnotherChallenge(challenge));
> 
> That way it's clear what the input is and what the expected result is at the
> call site. Also, it's much more convenient for the call site to switch to an
> ASSERT_* if it doesn't make sense to proceed if this operation fails.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:92:
EXPECT_EQ(sec_buf.length, len);
On 2017/06/23 21:29:10, asanka wrote:
> Should return early if this winds up not being the case. Also, return a bool
or
> something that the calling function can ASSERT_* on since it's likely that the
> test can't proceed past this failure. Here and elsewhere.

When I wrote most of the tests I didn't realize that EXPECT_* doesn't stop
execution. I'll just change everything to assert instead. It's not helping
anyone to continue once the test fails or to try and think about what happens if
you do or don't continue on every condition.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:140: static void
MockRandom(uint8_t* output, size_t n) {
On 2017/06/23 21:29:10, asanka wrote:
> Why not memset(output, 4, n) ? https://xkcd.com/221/

Works for me. I copy/pasted it from
net/http/http_network_transaction_unittest.cc

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:188: 
On 2017/06/23 21:29:10, asanka wrote:
> The remainder of the test is basically testing the NtlmBufferReader
> implementation. :-/ The above comparison against a static string is sufficient
> to verify the Type1 token generation.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:224:
"TlRMTVNTUAACAAAADAAMADgAAAAFgokCXziKeNIPIDYAAAAAAAAAAIYAhgBEAAAABgOAJQAA"
On 2017/06/23 21:29:10, asanka wrote:
> The token is irrelevant. Let's go with something shorter.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:236:
HandleAnotherChallenge(CreateType2Token(base::StringPiece(raw, sizeof(raw))));
On 2017/06/23 21:29:10, asanka wrote:
> The memory bots may complain about this because the generated token is likely
> based on uninitialized memory. ("branch conditionalized on uninitialized
value"
> or somesuch).
> 
> Let's take a valid Type2 message and truncate it for this test.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:244: char raw[32];
On 2017/06/23 21:29:11, asanka wrote:
> Same as before. Use a known initialized buffer for tests.
> 
> Also, the failure could be due to anything. Let's start with a minimal valid
> message, and test by changing individual fields thereof. That should increase
> our confidence that the ERR_UNEXPECTED is due to the invalid signature.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:304:
ntlm::SecurityBuffer(ntlm::CHALLENGE_HEADER_LEN, 1)));
On 2017/06/23 21:29:11, asanka wrote:
> Note that you are relying on NtlmBufferWriter initializing the extra byte.
This
> is not the case in the dependent patchset. I added comment in the CL that
> introduces NtlmBufferWriter to address that.
> 

N/A now.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:372:
EXPECT_TRUE(reader.SkipSecurityBuffer());
On 2017/06/23 21:29:10, asanka wrote:
> SkipSecurityBufferWithValidation() ?

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:487:
ntlm::GenerateResponsesV1WithSS(base::ASCIIToUTF16(password_ascii_),
On 2017/06/23 21:29:11, asanka wrote:
> While this makes sense for now since the test is exercising two
implementations,
> it won't make sense when we cut over to a single implementation. At that point
> we'd be verifying that the new HttpAuthHandlerNtlmPortable winds up calling
> GenerateResponses* at some point. Beyond that, we are comparing the output of
> GenerateResponses* with itself.
> 
> Let's  test against fixed hand verified static output rather than ones that
are
> generated by the implementation we are testing.
> 
> It's a bit hard to verify the output token against a fixed static token since
> the protocol doesn't provide a canonical representation. So I'm not against
> using a NtlmBufferReader. But at least the individual fields should be fixed.

Are you OK leaving this for this CL? 

We have validated GenerateResponsesV1WithSS against the test data in the spec.
Most of the content/output tests in this file actually belong against the lower
level NtlmClient class and are moved there in the next CL which switches out the
implementation. In that CL I will add tests against the verbatim binary messages
in the spec. We don't have enough control over the current implementation to be
able to generate the exact spec version of the messages.

HttpAuthHandlerNtlm is only really responsible for base64 encoding and the
challenge response mechanism but this was the only place to put it for now.

https://codereview.chromium.org/2873673002/diff/460001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:583: // Skip the session
key which isn't used.
On 2017/06/23 21:29:10, asanka wrote:
> ?

Clarified the comment a bit. AFAIK it's only for the connectionless mode for the
sealing/signing stuff. We don't send NTLMSSP_NEGOTIATE_KEY_EXCH so it should be
empty.

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h
File net/http/ntlm_client.h (right):

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h...
net/http/ntlm_client.h:23: namespace ntlm {
On 2017/06/23 21:29:11, asanka wrote:
> It's was a bit tricky to locate the spec for each of the functions implemented
> below. Shall we add the correct references? That way it's easier for someone
> trying to verify the correctness and compliance of the implementation.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h...
net/http/ntlm_client.h:25: // Generates the NTLMv1 Hash and writes the 16 byte
result to |hash|
On 2017/06/23 21:29:11, asanka wrote:
> NTOWFv1() as defined in Section 3.3.1 of [MS-NLMP].

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h...
net/http/ntlm_client.h:30: // function in the NTLMSSP spec (Section 6 Appendix
A)
On 2017/06/23 21:29:11, asanka wrote:
> [MS-NLMP] rev 28 Section 6
> 
> (Section 6 is Appendix A. No need to specify both.)

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client.h...
net/http/ntlm_client.h:32: // |hash| must contain at least 16 bytes.
On 2017/06/23 21:29:11, asanka wrote:
> "at least" is worrisome wording for a buffer whose length is left unspecified.
> Let's drop the "at least" and make sure we don't touch anything outside of
that.
> Here and elsewhere.
> 
> Also give these buffer sizes names so that both the calling code and the
called
> implementation can use the same constants that govern buffer sizes.

Done.

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client_u...
File net/http/ntlm_client_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/460001/net/http/ntlm_client_u...
net/http/ntlm_client_unittest.cc:19: 
On 2017/06/23 21:29:11, asanka wrote:
> Please use the test vectors in section 4.2 (Cryptographic values for
validation)
> for these tests.
> 
> These tests are okay as smoke tests, but not sufficient to validate
> cryptographic operations.

I never even noticed they were there!

[Ryan Sleevi, 2017-07-12 17:56:39]

Quick pass, I'm not sure if Asanka wants to continue as primary reviewer

https://codereview.chromium.org/2873673002/diff/740001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2873673002/diff/740001/net/BUILD.gn#newcode1774
net/BUILD.gn:1774: "ntlm/ntlm_constants.h",
Apologies that I missed this with the first CL, but my understanding from
brettw@ is the preference is to list all sources (in the sources) and then
subjectively subtract them, rather than add them.

The reporting doesn't seem to follow this pattern, but if you look at the
various build config below, the majority follow this.

(The += tends to be for stuff that is otherwise filtered out by the GNI configs)

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable.cc (right):

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable.cc:371: if (offset + target_len >
offset && offset + target_len <= in_len) {
if (target_len <= in_len && in_len - offset >= target_len) ?

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:178: // part of the message
is the flags and Chrome always offers the same
s/chrome/this implementation/?

(We're in //net, it can be used outside of Chrome the Product)

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/md4.cc
File net/ntlm/md4.cc (right):

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/md4.cc#newcode2
net/ntlm/md4.cc:2: // changes to port it to our source tree.
The style formatting means much larger changes.

Perhaps there should be a /* clang-format off */ directive?

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h
File net/ntlm/ntlm.h (right):

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode59
net/ntlm/ntlm.h:59: // NOTE: This should not be used. It will only get used in
V1 if the
This says "should not be used" (which is good, documents how), but then says "it
will only get used" - which documents the implementation.

Perhaps its worth rewording to indicate the 'safe'/expected and
unsafe/unexpected pattern?

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode60
net/ntlm/ntlm.h:60: // |negotiate_flags_| passed to the constructor omit the
"the constructor" ?

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode80
net/ntlm/ntlm.h:80: NET_EXPORT_PRIVATE void GenerateLMResponseV1WithSS(
"WithSS", while saving, leaves it ambiguous as to what that abbreviation is.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode97
net/ntlm/ntlm.h:97: // the same as without extended session security except the
challenge
s/security except/security, except/

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode99
net/ntlm/ntlm.h:99: // just |server_challenge|. See [MS-NLMP] Section 3.3.1.
This reads really weirdly - the "see" and "see" and just trying to grammatically
fit this. It also seems like multiple other functions have to be read - but
perhaps I've misunderstood?

It might be easier to document what it is.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm_test_data.h
File net/ntlm/ntlm_test_data.h (right):

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm_test_dat...
net/ntlm/ntlm_test_data.h:27: const std::string NTLM_HOSTNAME_ASCII("COMPUTER");
So, both of these violate the
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
rule

That said, you could use

constexpr base::char16 NTLM_DOMAIN[] = { 'D', 'o', 'm', 'a', 'i', 'n', 0 };
constexpr char NTLM_DOMAIN_ASCII[] = "Domain";

However, this also runs afoul of the constant naming rules in
https://google.github.io/styleguide/cppguide.html#Constant_Names

kNtlmDomainAscii ?

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm_test_dat...
net/ntlm/ntlm_test_data.h:51: constexpr uint8_t
EXPECTED_V1_WITH_SS_LM_RESPONSE[RESPONSE_V1_LEN] = {
Same naming concerns throughout

[asanka, 2017-07-12 19:53:16]

Looking good. I only have nits.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:145: const uint8_t
challenge_header_[ntlm::CHALLENGE_HEADER_LEN] = {
Let's call this challenge_message_ or something like that. The HttpAuthHandlers
already have a concept of headers which refer to HTTP headers. A token in a
header would be Base64 encoded.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:145: const uint8_t
challenge_header_[ntlm::CHALLENGE_HEADER_LEN] = {
Also, 'static const'. Otherwise this would be equivalent to having a const array
as a member that's initialized in the constructor.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:185: 
For consistency with usage, there would be a GenerateAuthToken() call here.
Otherwise this test is feeding a Type2 message to a handler that hasn't been
spun up to generate a Type1 message.

Here and elsewhere.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:230:
TEST_F(HttpAuthHandlerNtlmPortableTest, Type2MessageNoSig) {
Type2MessageWrongSignature

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:562: }
These are pretty neat tests!

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm_test_data.h
File net/ntlm/ntlm_test_data.h (right):

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm_test_dat...
net/ntlm/ntlm_test_data.h:27: const std::string NTLM_HOSTNAME_ASCII("COMPUTER");
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> So, both of these violate the
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
> rule
> 
> That said, you could use
> 
> constexpr base::char16 NTLM_DOMAIN[] = { 'D', 'o', 'm', 'a', 'i', 'n', 0 };
> constexpr char NTLM_DOMAIN_ASCII[] = "Domain";
> 
> However, this also runs afoul of the constant naming rules in
> https://google.github.io/styleguide/cppguide.html#Constant_Names
> 
> kNtlmDomainAscii ?
> 
> 

Also, let's make it obvious that these are specific test vectors. NTLM_DOMAIN or
NtlmDomain can be confusing when referred to as ntlm::NTLM_DOMAIN or
ntlm::kNtlmDomain* from outside this source file.

Maybe something like kTestingNtlmDomain? kExampleNtlmDomain?

[zentaro, 2017-07-13 17:10:20]

https://codereview.chromium.org/2873673002/diff/740001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2873673002/diff/740001/net/BUILD.gn#newcode1774
net/BUILD.gn:1774: "ntlm/ntlm_constants.h",
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> Apologies that I missed this with the first CL, but my understanding from
> brettw@ is the preference is to list all sources (in the sources) and then
> subjectively subtract them, rather than add them.
> 
> The reporting doesn't seem to follow this pattern, but if you look at the
> various build config below, the majority follow this.
> 
> (The += tends to be for stuff that is otherwise filtered out by the GNI
configs)

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:145: const uint8_t
challenge_header_[ntlm::CHALLENGE_HEADER_LEN] = {
On 2017/07/12 19:53:16, asanka wrote:
> Also, 'static const'. Otherwise this would be equivalent to having a const
array
> as a member that's initialized in the constructor. 

This get's added to ntlm_test_data in the next CL anyway so I just moved it
there now.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:145: const uint8_t
challenge_header_[ntlm::CHALLENGE_HEADER_LEN] = {
On 2017/07/12 19:53:16, asanka wrote:
> Let's call this challenge_message_ or something like that. The
HttpAuthHandlers
> already have a concept of headers which refer to HTTP headers. A token in a
> header would be Base64 encoded.

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:178: // part of the message
is the flags and Chrome always offers the same
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> s/chrome/this implementation/?
> 
> (We're in //net, it can be used outside of Chrome the Product)

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:185: 
On 2017/07/12 19:53:15, asanka wrote:
> For consistency with usage, there would be a GenerateAuthToken() call here.
> Otherwise this test is feeding a Type2 message to a handler that hasn't been
> spun up to generate a Type1 message.
> 
> Here and elsewhere.

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:230:
TEST_F(HttpAuthHandlerNtlmPortableTest, Type2MessageNoSig) {
On 2017/07/12 19:53:16, asanka wrote:
> Type2MessageWrongSignature

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/md4.cc
File net/ntlm/md4.cc (right):

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/md4.cc#newcode2
net/ntlm/md4.cc:2: // changes to port it to our source tree.
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> The style formatting means much larger changes.
> 
> Perhaps there should be a /* clang-format off */ directive?

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h
File net/ntlm/ntlm.h (right):

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode59
net/ntlm/ntlm.h:59: // NOTE: This should not be used. It will only get used in
V1 if the
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> This says "should not be used" (which is good, documents how), but then says
"it
> will only get used" - which documents the implementation.
> 
> Perhaps its worth rewording to indicate the 'safe'/expected and
> unsafe/unexpected pattern?

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode60
net/ntlm/ntlm.h:60: // |negotiate_flags_| passed to the constructor omit the
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> "the constructor" ?

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode80
net/ntlm/ntlm.h:80: NET_EXPORT_PRIVATE void GenerateLMResponseV1WithSS(
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> "WithSS", while saving, leaves it ambiguous as to what that abbreviation is.

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode97
net/ntlm/ntlm.h:97: // the same as without extended session security except the
challenge
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> s/security except/security, except/

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm.h#newcode99
net/ntlm/ntlm.h:99: // just |server_challenge|. See [MS-NLMP] Section 3.3.1.
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> This reads really weirdly - the "see" and "see" and just trying to
grammatically
> fit this. It also seems like multiple other functions have to be read - but
> perhaps I've misunderstood?
> 
> It might be easier to document what it is.

Done.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm_test_data.h
File net/ntlm/ntlm_test_data.h (right):

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm_test_dat...
net/ntlm/ntlm_test_data.h:27: const std::string NTLM_HOSTNAME_ASCII("COMPUTER");
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> So, both of these violate the
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
> rule
> 
> That said, you could use
> 
> constexpr base::char16 NTLM_DOMAIN[] = { 'D', 'o', 'm', 'a', 'i', 'n', 0 };
> constexpr char NTLM_DOMAIN_ASCII[] = "Domain";
> 
> However, this also runs afoul of the constant naming rules in
> https://google.github.io/styleguide/cppguide.html#Constant_Names
> 
> kNtlmDomainAscii ?
> 
> 

Done. Put the test data in a test:: namespace.

https://codereview.chromium.org/2873673002/diff/740001/net/ntlm/ntlm_test_dat...
net/ntlm/ntlm_test_data.h:51: constexpr uint8_t
EXPECTED_V1_WITH_SS_LM_RESPONSE[RESPONSE_V1_LEN] = {
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> Same naming concerns throughout

Done.

[asanka, 2017-07-13 17:26:13]

LGTM

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable.cc (right):

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable.cc:371: if (offset + target_len >
offset && offset + target_len <= in_len) {
On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> if (target_len <= in_len && in_len - offset >= target_len) ?

Ping

[asanka, 2017-07-13 17:26:55]

.. but wait for rsleevi and also address the unaddressed comment.

[zentaro, 2017-07-13 17:36:21]

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable.cc (right):

https://codereview.chromium.org/2873673002/diff/740001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable.cc:371: if (offset + target_len >
offset && offset + target_len <= in_len) {
On 2017/07/13 17:26:13, asanka wrote:
> On 2017/07/12 17:56:38, Ryan Sleevi wrote:
> > if (target_len <= in_len && in_len - offset >= target_len) ?
> 
> Ping

Sorry. I didn't see this one.

Changed it. Though this code was always here. Plus it's about to get deleted :)

[Ryan Sleevi, 2017-07-13 17:39:54]

LGTM % nits

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:82:
EXPECT_EQ(sec_buf.length, len);
ASSERT_EQ ?

If .length > len, this would be a crash bug, right?

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:90:
EXPECT_TRUE(reader->ReadSecurityBuffer(&sec_buf));
ASSERT_TRUE ?

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:92: uint8_t
raw[sec_buf.length];
I don't believe this is legal C++, because it's a variable array (that is, the
compiler cannot reserve sufficient space on the stack, because .length will
vary)

VLAs are a GCC extension and, while part of C99, not part of C++11

scoped_ptr<uint8_t[]> raw(new uint8_t[sec_buf.length])

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:107: uint8_t
raw[sec_buf.length];
Ditto re: VLAs

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:110: #if IS_BIG_ENDIAN
The code elsewhere (e.g. http_auth_handler_ntlm_portable) uses

#ifdef IS_BIG_ENDIAN

rather than #if

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/des.cc
File net/ntlm/des.cc (right):

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/des.cc#newcode51
net/ntlm/des.cc:51: /* clang-format off */
I would move this to line 14

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/md4.cc
File net/ntlm/md4.cc (right):

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/md4.cc#newcode52
net/ntlm/md4.cc:52: /* clang-format off */
I would move this to line 6

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/ntlm.h
File net/ntlm/ntlm.h (right):

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/ntlm.h#newcode80
net/ntlm/ntlm.h:80: // |lm_response| must contain |kResponseLenV1| bytes.
You introduce a newline in the other bits of documentation (e.g. 89, 100, 115) -
for consistency, the same here?

[zentaro, 2017-07-13 18:20:12]

The CQ bit was checked by zentaro@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-13 18:20:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zentaro, 2017-07-13 18:20:31]

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm_portable_unittest.cc (right):

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:82:
EXPECT_EQ(sec_buf.length, len);
On 2017/07/13 17:39:54, Ryan Sleevi wrote:
> ASSERT_EQ ?
> 
> If .length > len, this would be a crash bug, right?

Done.

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:90:
EXPECT_TRUE(reader->ReadSecurityBuffer(&sec_buf));
On 2017/07/13 17:39:53, Ryan Sleevi wrote:
> ASSERT_TRUE ?

Done.

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:92: uint8_t
raw[sec_buf.length];
On 2017/07/13 17:39:53, Ryan Sleevi wrote:
> I don't believe this is legal C++, because it's a variable array (that is, the
> compiler cannot reserve sufficient space on the stack, because .length will
> vary)
> 
> VLAs are a GCC extension and, while part of C99, not part of C++11
> 
> scoped_ptr<uint8_t[]> raw(new uint8_t[sec_buf.length])

Done.

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:107: uint8_t
raw[sec_buf.length];
On 2017/07/13 17:39:54, Ryan Sleevi wrote:
> Ditto re: VLAs

Done.

https://codereview.chromium.org/2873673002/diff/780001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm_portable_unittest.cc:110: #if IS_BIG_ENDIAN
On 2017/07/13 17:39:53, Ryan Sleevi wrote:
> The code elsewhere (e.g. http_auth_handler_ntlm_portable) uses
> 
> #ifdef IS_BIG_ENDIAN
> 
> rather than #if

Done.

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/des.cc
File net/ntlm/des.cc (right):

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/des.cc#newcode51
net/ntlm/des.cc:51: /* clang-format off */
On 2017/07/13 17:39:54, Ryan Sleevi wrote:
> I would move this to line 14

Done.

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/md4.cc
File net/ntlm/md4.cc (right):

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/md4.cc#newcode52
net/ntlm/md4.cc:52: /* clang-format off */
On 2017/07/13 17:39:54, Ryan Sleevi wrote:
> I would move this to line 6

Done.

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/ntlm.h
File net/ntlm/ntlm.h (right):

https://codereview.chromium.org/2873673002/diff/780001/net/ntlm/ntlm.h#newcode80
net/ntlm/ntlm.h:80: // |lm_response| must contain |kResponseLenV1| bytes.
On 2017/07/13 17:39:54, Ryan Sleevi wrote:
> You introduce a newline in the other bits of documentation (e.g. 89, 100, 115)
-
> for consistency, the same here?

Done.

[zentaro, 2017-07-13 18:21:32]

Description was changed from

==========
Add unit tests for existing NTLM portable code.

- Adds tests for existing implementation.
- Minor code change to reject certain invalid messages.

NOTE: The existing code just ignored the target name field if it
had length and/or offset outside the bounds of the message. This
change explicitly rejects that case in order to test that the
behavior is correct.

BUG=22532
TEST=builds and tests pass
==========

to

==========
Add unit tests for existing NTLM portable code.

- Adds tests for existing implementation.
- Minor code change to reject certain invalid messages.

NOTE: The existing code just ignored the target name field if it
had length and/or offset outside the bounds of the message. This
change explicitly rejects that case in order to test that the
behavior is correct.

BUG=chromium:22532
TEST=builds and tests pass
==========

[commit-bot: I haz the power, 2017-07-13 20:02:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-13 20:02:18]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[zentaro, 2017-07-13 20:38:41]

The CQ bit was checked by zentaro@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-13 20:39:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-13 22:42:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-13 22:42:08]

Dry run: This issue passed the CQ dry run.

[zentaro, 2017-07-14 01:19:16]

The CQ bit was checked by zentaro@chromium.org

[zentaro, 2017-07-14 01:19:17]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2873673002/#ps840001
(title: "Rebase")

[commit-bot: I haz the power, 2017-07-14 01:19:40]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-14 01:28:59]

CQ is committing da patch.

Bot data: {"patchset_id": 840001, "attempt_start_ts": 1499995156384000,
"parent_rev": "8f05f8ddc10dfbb30ccff7e43c183b7ce9891fac", "commit_rev":
"b89acda495056e4af04adc1b34774bc3069f612a"}

[commit-bot: I haz the power, 2017-07-14 01:29:20]

Description was changed from

==========
Add unit tests for existing NTLM portable code.

- Adds tests for existing implementation.
- Minor code change to reject certain invalid messages.

NOTE: The existing code just ignored the target name field if it
had length and/or offset outside the bounds of the message. This
change explicitly rejects that case in order to test that the
behavior is correct.

BUG=chromium:22532
TEST=builds and tests pass
==========

to

==========
Add unit tests for existing NTLM portable code.

- Adds tests for existing implementation.
- Minor code change to reject certain invalid messages.

NOTE: The existing code just ignored the target name field if it
had length and/or offset outside the bounds of the message. This
change explicitly rejects that case in order to test that the
behavior is correct.

BUG=chromium:22532
TEST=builds and tests pass

Review-Url: https://codereview.chromium.org/2873673002
Cr-Commit-Position: refs/heads/master@{#486604}
Committed:
https://chromium.googlesource.com/chromium/src/+/b89acda495056e4af04adc1b3477...
==========

[commit-bot: I haz the power, 2017-07-14 01:29:23]

Committed patchset #43 (id:840001) as
https://chromium.googlesource.com/chromium/src/+/b89acda495056e4af04adc1b3477...