Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2879353002:
Add a buffer reader/writer for NTLM. (Closed)
DescriptionAdd a buffer reader/writer for NTLM.
These will be used in new implementation of NTLMv2 and
consolidate the ad hoc buffer manipulation in the new and existing code.
Additionally useful as helpers for generating/validating data for
unit tests.
BUG=chromium:22532
TEST=builds and tests pass
Review-Url: https://codereview.chromium.org/2879353002
Cr-Commit-Position: refs/heads/master@{#486565}
Committed: https://chromium.googlesource.com/chromium/src/+/b747956839b2d8621c5267c44a9fe7f8765cc2d6
Patch Set 1 #Patch Set 2 : Accidentally left some dead code. #Patch Set 3 : More helper functions #Patch Set 4 : More helpers #Patch Set 5 : More tests #Patch Set 6 : String piece constructor #Patch Set 7 : Fix incorrect out param convention #Patch Set 8 : Enough implementation for ntlm v1 #Patch Set 9 : Cleanup, const, docs #Patch Set 10 : Cleanup #Patch Set 11 : Add a buffer reader/writer for NTLM. #
Total comments: 35
Patch Set 12 : Add a buffer reader/writer for NTLM. #Patch Set 13 : Refactoring and review feedback #Patch Set 14 : Missing comments #Patch Set 15 : Rebase and make security buffer a struct #Patch Set 16 : More unit tests #Patch Set 17 : Add a buffer reader/writer for NTLM. #
Total comments: 105
Patch Set 18 : Review feedback #Patch Set 19 : Renaming and some more const in test buffers #Patch Set 20 : More cleanup #Patch Set 21 : Rebase #
Total comments: 16
Patch Set 22 : Review feedback #Patch Set 23 : Typo #Patch Set 24 : Add additional ntlm reference to comments #
Total comments: 6
Patch Set 25 : Review feedback. Next CL will move files. #Patch Set 26 : Move implementation to //net/ntlm #Patch Set 27 : Add a buffer reader/writer for NTLM. #Patch Set 28 : Remove dead code #Patch Set 29 : EXPECT_* -> ASSERT_* #
Total comments: 6
Patch Set 30 : Feedback and rebase #Patch Set 31 : Rename header #Patch Set 32 : Add a buffer reader/writer for NTLM. #
Total comments: 63
Patch Set 33 : Review feedback #Patch Set 34 : Nits #Patch Set 35 : Cleanup comment #Patch Set 36 : Add a buffer reader/writer for NTLM. #Patch Set 37 : Move build config back to net #Patch Set 38 : Fix constant naming convention #Patch Set 39 : Fix naming conventions #Patch Set 40 : Fix include guard #Patch Set 41 : Update build -= not += #Patch Set 42 : Remove ReleaseBufferPtr #Dependent Patchsets: Messages
Total messages: 58 (31 generated)
Description was changed from ========== Add a buffer reader/writer for NTLM. These will be used in new implementation of NTLMv2 and consolidate the ad hoc buffer manipulation in the existing code. Additionally useful as helpers for generating/validating data for unit tests. BUG=chromium:22532 TEST=builds and tests pass ========== to ========== Add a buffer reader/writer for NTLM. These will be used in new implementation of NTLMv2 and consolidate the ad hoc buffer manipulation in the new and existing code. Additionally useful as helpers for generating/validating data for unit tests. BUG=chromium:22532 TEST=builds and tests pass ==========
zentaro@chromium.org changed reviewers: + rsleevi@chromium.org
On 2017/05/15 20:30:11, zentaro wrote: > mailto:zentaro@chromium.org changed reviewers: > + mailto:rsleevi@chromium.org PTAL
Hold off on reviewing this one for a little bit. Making it more complete and fixing some coding convention issues.
On 2017/05/19 20:29:02, zentaro wrote: > Hold off on reviewing this one for a little bit. Making it more complete and > fixing some coding convention issues. Ready yet? :)
Lots of first-pass feedback. Given how mirrored the two files are, I focused on the reader rather than the writer :) https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:120: #if IS_BIG_ENDIAN Is this right? That you swap the string based on the client encoding? https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:145: DCHECK(SetCursor(old_cursor)); Anything inside a DCHECK() can be optimized out - so it looks like 'nothing' will be read in a non-DCHECK version. The same applies throughout the rest of the file. Should they all be replaced with "return SetCursor(offset) && ReadUnicodeString(value, length) && SetCursor(old_cursor)" (or equivalent) https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:55: // signatures or message identifiers. I can't tell - are these aligned to the maximal use of horizontal space? It doesn't seem to be, judging by the code below. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:64: NtlmBufferReader(const base::StringPiece buffer); style: "explicit" - https://google.github.io/styleguide/cppguide.html#Implicit_Conversions https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:64: NtlmBufferReader(const base::StringPiece buffer); style: you can drop the const since you're passing by-val https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:65: NtlmBufferReader(const uint8_t* ptr, size_t len); Since you accept a base::StringPiece, is this overload needed? https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:78: const uint8_t* GetBufferAtCursor() const { return GetBufferPtr() + cursor_; } Both this and the function on line 72 seem like they introduce potentially error prone possibilities of reading off more than they can chew (that is, because of the distinct "CanRead" pattern that can be easily omitted) Did you consider making these variants where the caller must pre-declare the size of what they're reading/accessing, to force size checks, and omitting the CanRead/CanReadFrom? This is different than ReadBytes() [which seems to copy - is that necessary if you had that here?]. That is, something like "bool GetBuffer(uint8_t** buffer, size_t desired)" or something? https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:89: bool ReadUInt16(uint16_t* value); Document a bit more - for example, what's the endianness being read here (I'm guessing it's related to the NTLM specification in line 58 - if so, document) https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:106: bool ReadUnicodeString(base::string16* value, size_t len); I suspect you're borrowing from the NTLM language, but it would probably help to be more explicit here that you're talking about a UTF-16 string (or is it a BMP string?) https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:117: bool ReadBytesPayload(uint8_t* buffer, size_t buffer_len); How are the *Payload functions different from the *String / *Bytes functions? https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:121: // Skips over the security buffer without returning the values grammar: "returning the values, " (add a comma) https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:100: // the resulting 16 bit characters. This comment I found a bit confusing. Could you describe more what's going on? That is, is the string 7-bit (ASCII) or 8-bit (with a codepage? UTF-8?). Is it going UTF8 to UTF16, or something else? What's the special case here :) Assume someone isn't as familiar with the spec as you - is there a part in the spec that describes this behaviour (or any other behaviours relavant to understanding)? It might help to mention them 'for further reading' https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h File net/http/ntlm_client.h (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:29: // TODO(zentaro): Follow up CLs implement NTLMv1 and then NTLMv2. I'm not sure I understand the TODO in this file - unnecessary? And/or should they be part of the subsequent CL that implements || part of a refactoring that uses? https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:31: // delete line https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:36: class NET_EXPORT NtlmClient { NET_EXPORT_PRIVATE ? https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:40: NtlmClient(uint32_t negotiate_flags); explicit https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:44: static void GenerateNtlmHashV1(const base::string16& password, uint8_t* hash); See https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... regarding the use of static functions "Prefer grouping functions with a namespace instead of using a class as if it were a namespace. Static methods of a class should generally be closely related to instances of the class or the class's static data." https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_message.h File net/http/ntlm_message.h (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_message.... net/http/ntlm_message.h:17: class NET_EXPORT NtlmMessage { see https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... Sounds like these all belong in a namespace?
PTAAL https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:120: #if IS_BIG_ENDIAN On 2017/05/30 19:02:22, Ryan Sleevi wrote: > Is this right? That you swap the string based on the client encoding? I think so. I added a comment. Do we ever run unit tests on anything big endian? https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:145: DCHECK(SetCursor(old_cursor)); On 2017/05/30 19:02:22, Ryan Sleevi wrote: > Anything inside a DCHECK() can be optimized out - so it looks like 'nothing' > will be read in a non-DCHECK version. > > The same applies throughout the rest of the file. Should they all be replaced > with > > "return SetCursor(offset) && ReadUnicodeString(value, length) && > SetCursor(old_cursor)" (or equivalent) Oops. Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:55: // signatures or message identifiers. On 2017/05/30 19:02:22, Ryan Sleevi wrote: > I can't tell - are these aligned to the maximal use of horizontal space? It > doesn't seem to be, judging by the code below. Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:64: NtlmBufferReader(const base::StringPiece buffer); On 2017/05/30 19:02:22, Ryan Sleevi wrote: > style: you can drop the const since you're passing by-val Done. I previously was passing by const-ref then saw something saying don't do that for StringPiece and forgot to remove the const. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:64: NtlmBufferReader(const base::StringPiece buffer); On 2017/05/30 19:02:22, Ryan Sleevi wrote: > style: "explicit" - > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:65: NtlmBufferReader(const uint8_t* ptr, size_t len); On 2017/05/30 19:02:22, Ryan Sleevi wrote: > Since you accept a base::StringPiece, is this overload needed? I could I guess try and convert all the callers to construct a StringPiece. It's really clunky with all the casting though. Even on the other overload StringPiece doesn't feel right to use with uint8_t. Ideally it should be something like a 'span' or 'array_view' though I don't see anything like that in chrome already? That would also solve something else I don't really like is that in many places I just pass naked uint8_t* to buffers with implied length . ie most of the various fields are all fixed size inputs/hashes and their end destination is usually to a crypto function which makes the same assumption that it gets a naked pointer with an assumed min length. Some of these params could also std::array though in most cases the naked pointer would end up being pulled out and sent to a crypto/hash anyway and there are several cases where it's preferable to pass pointers into a larger array which would require extra copying to use std::array as a param. Thoughts? The nice thing about a span/array_view is that you can just pass a std::array or std::vector to it or have to_span(ptr, len) for the cases where you need to pull a piece out of another buffer. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:78: const uint8_t* GetBufferAtCursor() const { return GetBufferPtr() + cursor_; } On 2017/05/30 19:02:22, Ryan Sleevi wrote: > Both this and the function on line 72 seem like they introduce potentially error > prone possibilities of reading off more than they can chew (that is, because of > the distinct "CanRead" pattern that can be easily omitted) > > Did you consider making these variants where the caller must pre-declare the > size of what they're reading/accessing, to force size checks, and omitting the > CanRead/CanReadFrom? > > This is different than ReadBytes() [which seems to copy - is that necessary if > you had that here?]. > > That is, something like "bool GetBuffer(uint8_t** buffer, size_t desired)" or > something? I managed to refactor out all the public uses of these. I made the unit tests use a helper that wraps up all the casting over a stringpiece. And in other cases (in the future CLs) things like backfilling the MIC I just do on the naked pointer that is being returned anyway. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:89: bool ReadUInt16(uint16_t* value); On 2017/05/30 19:02:22, Ryan Sleevi wrote: > Document a bit more - for example, what's the endianness being read here (I'm > guessing it's related to the NTLM specification in line 58 - if so, document) Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:117: bool ReadBytesPayload(uint8_t* buffer, size_t buffer_len); On 2017/05/30 19:02:22, Ryan Sleevi wrote: > How are the *Payload functions different from the *String / *Bytes functions? Added some additional docs to clarify. But the 'payload' variants perform a two step process. First read the security buffer at the cursor (ie offset and length), then perform ReadString/ReadBytes from the payload (after the all the fixed position headers). The cursor never navigates directly through the payload because there is no context there on how to do it. The security buffer describes how to extract the that fields bytes from the payload.. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:121: // Skips over the security buffer without returning the values On 2017/05/30 19:02:22, Ryan Sleevi wrote: > grammar: "returning the values, " (add a comma) Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:100: // the resulting 16 bit characters. On 2017/05/30 19:02:23, Ryan Sleevi wrote: > This comment I found a bit confusing. Could you describe more what's going on? > > That is, is the string 7-bit (ASCII) or 8-bit (with a codepage? UTF-8?). Is it > going UTF8 to UTF16, or something else? What's the special case here :) > > Assume someone isn't as familiar with the spec as you - is there a part in the > spec that describes this behaviour (or any other behaviours relavant to > understanding)? It might help to mention them 'for further reading' I tried to clarify. In practice I think nothing ever really cares about the encodings of the strings we write. I'd have to try and dig through the spec a little more but from what I can tell the only string that both sides need to agree on how to interpret as a string is the SPN Technically when unicode isn't negotiated (which would be a really old windows server or maybe some other open source impl) the strings are "OEM Character set" which I think it inherently just "assumes compatibility". That's why I think nothing can ever be reading them and trying to match them to something on the other side. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h File net/http/ntlm_client.h (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:29: // TODO(zentaro): Follow up CLs implement NTLMv1 and then NTLMv2. On 2017/05/30 19:02:23, Ryan Sleevi wrote: > I'm not sure I understand the TODO in this file - unnecessary? And/or should > they be part of the subsequent CL that implements || part of a refactoring that > uses? It was meant to state that this class is pretty useless right now but gets filled in with subsequent CLs :) After some of the other refactoring - this class moved to a future CL and the statics are pulled out to a namespace. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:31: // On 2017/05/30 19:02:23, Ryan Sleevi wrote: > delete line Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:36: class NET_EXPORT NtlmClient { On 2017/05/30 19:02:23, Ryan Sleevi wrote: > NET_EXPORT_PRIVATE ? Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:40: NtlmClient(uint32_t negotiate_flags); On 2017/05/30 19:02:23, Ryan Sleevi wrote: > explicit Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_client.h... net/http/ntlm_client.h:44: static void GenerateNtlmHashV1(const base::string16& password, uint8_t* hash); On 2017/05/30 19:02:23, Ryan Sleevi wrote: > See > https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... > regarding the use of static functions > > "Prefer grouping functions with a namespace instead of using a class as if it > were a namespace. Static methods of a class should generally be closely related > to instances of the class or the class's static data." Done. https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_message.h File net/http/ntlm_message.h (right): https://codereview.chromium.org/2879353002/diff/200001/net/http/ntlm_message.... net/http/ntlm_message.h:17: class NET_EXPORT NtlmMessage { On 2017/05/30 19:02:23, Ryan Sleevi wrote: > see > https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a... > > Sounds like these all belong in a namespace? Done.
https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h File net/http/ntlm.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode1 net/http/ntlm.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c) needed https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode17 net/http/ntlm.h:17: struct SecurityBuffer { Document? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:25: DCHECK(buffer.data() != nullptr); DCHECK(buffer.data()) ? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:42: return (offset + len > offset) && (offset + len <= GetLength()); This usually triggers a pattern matching warning (you're checking for wrap-around, right?) return (len <= GetLength() && offset <= GetLength() - len) Same thing, right? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:95: if (ReadUInt16(&sec_buf->length) && SkipBytes(sizeof(uint16_t)) && heh, I'm torn on whether sizeof(uint16_t) is really better than just 2 ;) https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:166: ReadBytes(buffer, sec_buf.length) && SetCursor(old_cursor); Note: if ReadBytes() fails, SetCursor(old_cursor) won't happen, and the invariant in the header won't be correct. But from reading this, I'm not sure the documentation *is* correct, since it says: " 159 // In the case of failure the cursor will remain as if it never read the 160 // security buffer. In the case of success the cursor will point after the 161 // security buffer." But that's not the case - in the case of success, the cursor points to the current location, in the case of failure, the cursor state is undefined (since it could be old - if SetCursor failed, new - if ReadBytes failed) https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:25: // Delete newline? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:34: // Delete newline? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:35: // Read*Payload methods first read a security buffer see |ReadSecurityBuffer| nit: s/see |ReadSecurityBuffer|/(see ReadSecurityBuffer())| That is, grammatically, it doesn't flow - so we should either integrate into the sentence, such as: Read*Payload methods first read a security buffer, using ReadSecurityBuffer(), and then read the requested payload from ... https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:44: // had not been read. From looking at your documentatino for Read*Payload methods, they seem to adequately cover this - I think you can delete this block? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:45: // Delete newline? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:48: // equivalent Read method would without reading or returning the values. Deletable? :) https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:50: // Delete newline? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:52: // expected values, such as signatures or message identifiers. Deletable? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:54: // Delete newline? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:123: // First reads a security buffer (see |ReadSecurityBuffer|), then verifies s/First reads/Reads/ https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:180: bool MatchSignature(); Do Match() methods update the cursor or not? From the lack of explicitly mentioning it, I would assume "no" https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:33: if (GetBufferPtr() == nullptr) if (!GetBufferPtr()) return false; ? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:36: DCHECK(cursor_ <= GetLength()); DCHECK_LE(cursor_, GetLength()); https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:41: return (cursor_ + len > cursor_) && (cursor_ + len <= GetLength()); len <= GetLength() && cursor_ <= GetLength() - len https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:45: if ((GetBufferPtr() == nullptr) || (cursor > GetLength())) if (!GetBufferPtr() || (cursor > GetLength())) https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:164: static size_t GetStringPayloadLength(const std::string& str, bool is_unicode); Do these need to be public-static? Could they just be in the unnamed namespace in the .cc? Why or why not? They feel like an implementation detail :)
asanka@chromium.org changed reviewers: + asanka@chromium.org
https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h File net/http/ntlm.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode6 net/http/ntlm.h:6: #define NET_BASE_NTLM_H_ Nit: I'd suggest using the ntlm.h header for the API that callers are supposed to use to interact with this implementation and move the protocol constants out to a different file. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode37 net/http/ntlm.h:37: // Defined in NTLMSSP Spec 2.2.2.5 Let's refer to the spec as [MS-NLMP] which is what MS uses to refer to their specification. NTLMSSP is the name of a protocol and doesn't identify a spec. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode39 net/http/ntlm.h:39: enum NegotiateFlags { Let's make these typed enums. Here and elsewhere. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode52 net/http/ntlm.h:52: static constexpr size_t NEGOTIATE_MESSAGE_LEN = 32; Recent versions of Windows use a 40 byte NEGOTIATE message, no? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:95: if (ReadUInt16(&sec_buf->length) && SkipBytes(sizeof(uint16_t)) && nit: if (X) return true; return false; ---> return X; https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:119: #if IS_BIG_ENDIAN Do we build and test on BIG_ENDIAN? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:139: cursor_ -= ntlm::SECURITY_BUFFER_LEN; nit: store and restore the original cursor instead of undoing the cursor movement that ReadSecurityBuffer() supposedly did. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:191: uint32_t temp; everything is temporary :-) . This is the raw message value. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:259: size_t old_cursor = cursor_; Nit: let's be consistent about GetCursor()/SetCursor() vs. accessing cursor_ directly. I'm not opposed to getting rid of GetCursor()/SetCursor() if no outside consumer needs it. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:61: explicit NtlmBufferReader(base::StringPiece buffer); Obvious, but add a short note here and below that the reader shouldn't outlive the buffer. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:193: // security buffer. It's not clear to me whether the an empty security buffer is all zeros. Some parts of the spec say that an empty buffer should be encoded with length=0, and offset=<where the buffer would be if the length was not zero>. Have you looked at messages generated from a Windows box to see what they currently do?
https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader_unittest.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:34: buf[0] = expected & 0xff; Nit: it seems easier to read buf[] was initialized to { 0x22, 0x11 }; here and elsewhere. That way the reader doesn't need to mentally verify that the buffer construction in the test is correct. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:137: EXPECT_FALSE(reader.ReadSecurityBuffer(&sec_buf)); Check cursor for failing Read* calls? Here and elsewhere. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:168: expected.assign(expected_ascii.begin(), expected_ascii.end()); This test assumes that string16 encodes in UTF-16LE inside the buffer, but string16 makes no such claim. In fact it would use BE on a big-endian architecture since that's what uint16_t would be. Once again, I'd suggest initializing buf with something like { '1', 0, '2', 0 ... }. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:235: memset(buf, 0, ntlm::SECURITY_BUFFER_LEN); Yup. Definitely initialize buf directly. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:270: TEST(NtlmBufferReaderTest, SkipSecurityBufferWithValidationEmpty) { We are permissive about buffers with a length of zero and an OOB offset, right? It's fine either way, but I think we should be explicit about what should happen there. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:96: if (WriteUInt16(sec_buf.length) && WriteUInt16(sec_buf.length) && Nit: if (X) return true; return false ---> return X; https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:108: bool NtlmBufferWriter::WriteNarrowString(const std::string& str) { Nit: Rewrite this using WriteBytes() https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:120: std::string utf8 = base::SysWideToNativeMB(base::UTF16ToWide(str)); Why not Utf16ToUtf8()? That way the behavior won't be sensitive to the CP in use, which isn't necessarily UTF8. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:148: unicode.assign(str.begin(), str.end()); not the same as Utf8ToUtf16. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:189: return base::SysWideToNativeMB(base::UTF16ToWide(str)).length(); Utf16ToUtf8(). https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:196: // the string is assumed ASCII and the characters are zero-padded. Seems a bit inconsistent whether input strings are required to be ASCII, or UTF-8. If this is a hard requirement, it'll probably be better to call out "Ascii" / "Utf8" in the function name. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:56: uint8_t* ReleaseBufferPtr() { Why not something like a std::vector<uint8_t> ? That way the caller doesn't need to know exactly how big the buffer needs to be before the writes. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:120: // If unicode was not negotiated, username and domain get converted to UTF8 UTF-8? Shouldn't it need to be CP-1252? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:125: bool WriteString16AsUTF8(const base::string16& str); Nit: prefer to avoid stretches of upper case characters in names. I.e. call it WriteString16AsUtf8() https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:138: bool WriteNarrowStringAsString16(const std::string& str); Nit: Call it WriteUtf8AsUnicodeString since we are calling the previous method WriteString16AsUtf8 and treating the input as UTF-8. Also, the next method calls the output format "UnicodeString" which is defined more narrowly as UTF-16 LE w/o BOM. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer_unittest.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer_unittest.cc:75: EXPECT_EQ(expected & 0xff, GetByteFromBuffer(writer, 0)); As mentioned elsewhere, use literal buffers instead of constructions like this. That way the tests are easier to evaluate and don't need to be conditioned on the endianness.
https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader_unittest.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:178: } For UnicodeString reads and writes, let's add some tests that are "interesting". I.e. ones where there are non-ASCII inputs and outputs.
Ok. I'm done for this round :-) So, a high level comment before we go too far... HTTP treats NTLM/Negotiate shenanigans as opaque other than the semantics around 401/407 handling where there's no token in the header (i.e. a rejection). So how about we move the NTLM implementation into //net/ntlm ? That way we can have source sets that are included/excluded as a group based on whether we need the implementation for the corresponding build. This is in addition to keeping //net/http focused on HTTP.
On 2017/06/08 19:45:50, asanka wrote: > Ok. I'm done for this round :-) > > So, a high level comment before we go too far... > > HTTP treats NTLM/Negotiate shenanigans as opaque other than the semantics around > 401/407 handling where there's no token in the header (i.e. a rejection). > > So how about we move the NTLM implementation into //net/ntlm ? That way we can > have source sets that are included/excluded as a group based on whether we need > the implementation for the corresponding build. This is in addition to keeping > //net/http focused on HTTP. PTAL. As we discussed offline - the file moving/renaming will get done later once the content is reviewed.
PTAL: As discuss the file moving will happen later. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h File net/http/ntlm.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode1 net/http/ntlm.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/08 18:36:33, Ryan Sleevi wrote: > No (c) needed Done. Here and others. I assume this is new since most of the other files have it. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode6 net/http/ntlm.h:6: #define NET_BASE_NTLM_H_ On 2017/06/08 18:44:57, asanka wrote: > Nit: I'd suggest using the ntlm.h header for the API that callers are supposed > to use to interact with this implementation and move the protocol constants out > to a different file. TODO(zentaro): I'm going to do this round of feedback without moving it to keep the diffs easier to follow. Will follow up on specifics at the end of this round. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode17 net/http/ntlm.h:17: struct SecurityBuffer { On 2017/06/08 18:36:33, Ryan Sleevi wrote: > Document? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode37 net/http/ntlm.h:37: // Defined in NTLMSSP Spec 2.2.2.5 On 2017/06/08 18:44:57, asanka wrote: > Let's refer to the spec as [MS-NLMP] which is what MS uses to refer to their > specification. NTLMSSP is the name of a protocol and doesn't identify a spec. Done. And others. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode39 net/http/ntlm.h:39: enum NegotiateFlags { On 2017/06/08 18:44:57, asanka wrote: > Let's make these typed enums. Here and elsewhere. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode52 net/http/ntlm.h:52: static constexpr size_t NEGOTIATE_MESSAGE_LEN = 32; On 2017/06/08 18:44:57, asanka wrote: > Recent versions of Windows use a 40 byte NEGOTIATE message, no? Yes. It will be in the CL that adds NTLMv2 (and it will apply to v1 too). I tried to sequence the CL's so that I could land a set of unit tests against the old implementation, then next CL would pass all those tests (ie. be functionally the same), then make the upgrades in the following CL. But it's an optional field (that wasn't in older versions) and since we never set NTLMSSP_NEGOTIATE_VERSION in the flags the server won't try to read it. Plus the field is entirely windows specific and supposedly for debugging only, so in theory the server should not base any logic on it. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:25: DCHECK(buffer.data() != nullptr); On 2017/06/08 18:36:33, Ryan Sleevi wrote: > DCHECK(buffer.data()) ? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:42: return (offset + len > offset) && (offset + len <= GetLength()); On 2017/06/08 18:36:33, Ryan Sleevi wrote: > This usually triggers a pattern matching warning (you're checking for > wrap-around, right?) > > return (len <= GetLength() && offset <= GetLength() - len) > > Same thing, right? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:95: if (ReadUInt16(&sec_buf->length) && SkipBytes(sizeof(uint16_t)) && On 2017/06/08 18:36:33, Ryan Sleevi wrote: > heh, I'm torn on whether sizeof(uint16_t) is really better than just 2 ;) Seemed more natural here because it makes it more obvious that this is essentially SkipUInt16(). I don't feel really strongly about it! https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:119: #if IS_BIG_ENDIAN On 2017/06/08 18:44:58, asanka wrote: > Do we build and test on BIG_ENDIAN? No idea! If the rest of chrome doesn't support it, I'd love to rip these code paths out and hard fail at compile time. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:139: cursor_ -= ntlm::SECURITY_BUFFER_LEN; On 2017/06/08 18:44:58, asanka wrote: > nit: store and restore the original cursor instead of undoing the cursor > movement that ReadSecurityBuffer() supposedly did. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:166: ReadBytes(buffer, sec_buf.length) && SetCursor(old_cursor); On 2017/06/08 18:36:33, Ryan Sleevi wrote: > Note: if ReadBytes() fails, SetCursor(old_cursor) won't happen, and the > invariant in the header won't be correct. > > But from reading this, I'm not sure the documentation *is* correct, since it > says: > > " 159 // In the case of failure the cursor will remain as if it never read the > 160 // security buffer. In the case of success the cursor will point after > the > 161 // security buffer." > > But that's not the case - in the case of success, the cursor points to the > current location, in the case of failure, the cursor state is undefined (since > it could be old - if SetCursor failed, new - if ReadBytes failed) Set cursor should never fail. Because we already checked that in the if statement above (CanReadFrom would have failed). We DCHECK the final result just below too. However I realized it was redundant in every case to return a bool from SetCursor and modified it so that it internally DCHECKs and returns nothing. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:191: uint32_t temp; On 2017/06/08 18:44:58, asanka wrote: > everything is temporary :-) . This is the raw message value. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:259: size_t old_cursor = cursor_; On 2017/06/08 18:44:58, asanka wrote: > Nit: let's be consistent about GetCursor()/SetCursor() vs. accessing cursor_ > directly. I'm not opposed to getting rid of GetCursor()/SetCursor() if no > outside consumer needs it. Yep. SetCursor() it is. It was added in a refactor so I guess I missed a few spots. Also added an AdvanceCursor for the other cases where cursor_ is being moved directly. Considered adding a tiny helper class eg. CursorRestorer() that would automatically restore when it went out of scope unless told otherwise. But seemed maybe overkill? https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:25: // On 2017/06/08 18:36:34, Ryan Sleevi wrote: > Delete newline? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:34: // On 2017/06/08 18:36:33, Ryan Sleevi wrote: > Delete newline? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:35: // Read*Payload methods first read a security buffer see |ReadSecurityBuffer| On 2017/06/08 18:36:34, Ryan Sleevi wrote: > nit: > s/see |ReadSecurityBuffer|/(see ReadSecurityBuffer())| > > That is, grammatically, it doesn't flow - so we should either integrate into the > sentence, such as: > > Read*Payload methods first read a security buffer, using ReadSecurityBuffer(), > and then read the requested payload from ... Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:44: // had not been read. On 2017/06/08 18:36:34, Ryan Sleevi wrote: > From looking at your documentatino for Read*Payload methods, they seem to > adequately cover this - I think you can delete this block? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:45: // On 2017/06/08 18:36:33, Ryan Sleevi wrote: > Delete newline? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:48: // equivalent Read method would without reading or returning the values. On 2017/06/08 18:36:33, Ryan Sleevi wrote: > Deletable? :) Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:50: // On 2017/06/08 18:36:33, Ryan Sleevi wrote: > Delete newline? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:52: // expected values, such as signatures or message identifiers. On 2017/06/08 18:36:34, Ryan Sleevi wrote: > Deletable? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:54: // On 2017/06/08 18:36:33, Ryan Sleevi wrote: > Delete newline? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:61: explicit NtlmBufferReader(base::StringPiece buffer); On 2017/06/08 18:44:58, asanka wrote: > Obvious, but add a short note here and below that the reader shouldn't outlive > the buffer. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:123: // First reads a security buffer (see |ReadSecurityBuffer|), then verifies On 2017/06/08 18:36:33, Ryan Sleevi wrote: > s/First reads/Reads/ Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:180: bool MatchSignature(); On 2017/06/08 18:36:33, Ryan Sleevi wrote: > Do Match() methods update the cursor or not? From the lack of explicitly > mentioning it, I would assume "no" They do. Made more explicit. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:193: // security buffer. On 2017/06/08 18:44:58, asanka wrote: > It's not clear to me whether the an empty security buffer is all zeros. Some > parts of the spec say that an empty buffer should be encoded with length=0, and > offset=<where the buffer would be if the length was not zero>. > > Have you looked at messages generated from a Windows box to see what they > currently do? I've seen the part of the spec that says that but IMO it's ambiguous for several reasons. (1) Do multiple empty buffers point to the same place? Do they all point to the start of the payload section? (2) Do they point to the end of the payload that preceded them in the header section? (3) Do they point to the end of the payload section? Payloads can be in any order. They could even overlap I assume. So any of the those alternatives could be considered correct. It seems like that is just a description of how it is implemented not a meaningful requirement since no read will happen if it is zero length anyway. I assume they just have a payload_upto variable in their implementation and write it unconditionally. I haven't looked at a windows machine though (I suspect it does (2)). The mozilla implementation of v1 and 2 write zero. So I don't think it matters and at least the current way it's implemented it would need additional work to do option (2). https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader_unittest.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:34: buf[0] = expected & 0xff; On 2017/06/08 19:40:09, asanka wrote: > Nit: it seems easier to read buf[] was initialized to { 0x22, 0x11 }; here and > elsewhere. That way the reader doesn't need to mentally verify that the buffer > construction in the test is correct. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:137: EXPECT_FALSE(reader.ReadSecurityBuffer(&sec_buf)); On 2017/06/08 19:40:09, asanka wrote: > Check cursor for failing Read* calls? Here and elsewhere. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:168: expected.assign(expected_ascii.begin(), expected_ascii.end()); On 2017/06/08 19:40:09, asanka wrote: > This test assumes that string16 encodes in UTF-16LE inside the buffer, but > string16 makes no such claim. In fact it would use BE on a big-endian > architecture since that's what uint16_t would be. > > Once again, I'd suggest initializing buf with something like { '1', 0, '2', 0 > ... }. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:235: memset(buf, 0, ntlm::SECURITY_BUFFER_LEN); On 2017/06/08 19:40:09, asanka wrote: > Yup. Definitely initialize buf directly. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader_unittest.cc:270: TEST(NtlmBufferReaderTest, SkipSecurityBufferWithValidationEmpty) { On 2017/06/08 19:40:09, asanka wrote: > We are permissive about buffers with a length of zero and an OOB offset, right? > It's fine either way, but I think we should be explicit about what should happen > there. Yes. Added explicit test. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:33: if (GetBufferPtr() == nullptr) On 2017/06/08 18:36:34, Ryan Sleevi wrote: > if (!GetBufferPtr()) > return false; > > ? Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:36: DCHECK(cursor_ <= GetLength()); On 2017/06/08 18:36:34, Ryan Sleevi wrote: > DCHECK_LE(cursor_, GetLength()); Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:41: return (cursor_ + len > cursor_) && (cursor_ + len <= GetLength()); On 2017/06/08 18:36:34, Ryan Sleevi wrote: > len <= GetLength() && cursor_ <= GetLength() - len Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:45: if ((GetBufferPtr() == nullptr) || (cursor > GetLength())) On 2017/06/08 18:36:34, Ryan Sleevi wrote: > if (!GetBufferPtr() || (cursor > GetLength())) Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:96: if (WriteUInt16(sec_buf.length) && WriteUInt16(sec_buf.length) && On 2017/06/08 19:40:09, asanka wrote: > Nit: if (X) return true; return false ---> return X; I wrote it like that so I do NOTREACHED() but I reorganized to DCHECK the result. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:108: bool NtlmBufferWriter::WriteNarrowString(const std::string& str) { On 2017/06/08 19:40:09, asanka wrote: > Nit: Rewrite this using WriteBytes() Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:120: std::string utf8 = base::SysWideToNativeMB(base::UTF16ToWide(str)); On 2017/06/08 19:40:09, asanka wrote: > Why not Utf16ToUtf8()? That way the behavior won't be sensitive to the CP in > use, which isn't necessarily UTF8. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:148: unicode.assign(str.begin(), str.end()); On 2017/06/08 19:40:09, asanka wrote: > not the same as Utf8ToUtf16. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:189: return base::SysWideToNativeMB(base::UTF16ToWide(str)).length(); On 2017/06/08 19:40:09, asanka wrote: > Utf16ToUtf8(). Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:196: // the string is assumed ASCII and the characters are zero-padded. On 2017/06/08 19:40:09, asanka wrote: > Seems a bit inconsistent whether input strings are required to be ASCII, or > UTF-8. If this is a hard requirement, it'll probably be better to call out > "Ascii" / "Utf8" in the function name. Per our offline discussion regarding the 8 bit version being OEM and that we just treat all 8 bit strings as UTF8 or "don't care" https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:56: uint8_t* ReleaseBufferPtr() { On 2017/06/08 19:40:10, asanka wrote: > Why not something like a std::vector<uint8_t> ? That way the caller doesn't need > to know exactly how big the buffer needs to be before the writes. If the output format was more general/variable like a file encoder I think a vector which can size as needed would be better. But in this case the format is small and well defined in size. I feel like it makes it easier to verify if it is fixed. Plus at the end of all this the ownership of of a uint8_t* needs to be given to the auth handler so we'd have to make a copy to a naked buffer anyway. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:120: // If unicode was not negotiated, username and domain get converted to UTF8 On 2017/06/08 19:40:10, asanka wrote: > UTF-8? Shouldn't it need to be CP-1252? Per other discussion it's actually OEM (basically arbitrary) and for simplicity of explanation we are just going to consider that our OEM character set is UTF8. I think most reasonable servers support unicode by now. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:125: bool WriteString16AsUTF8(const base::string16& str); On 2017/06/08 19:40:09, asanka wrote: > Nit: prefer to avoid stretches of upper case characters in names. I.e. call it > WriteString16AsUtf8() Done. Normally I would but I just followed the conventions of all the methods in base/strings that write UTF all upper case. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:138: bool WriteNarrowStringAsString16(const std::string& str); On 2017/06/08 19:40:10, asanka wrote: > Nit: Call it WriteUtf8AsUnicodeString since we are calling the previous method > WriteString16AsUtf8 and treating the input as UTF-8. > > Also, the next method calls the output format "UnicodeString" which is defined > more narrowly as UTF-16 LE w/o BOM. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer_unittest.cc (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer_unittest.cc:75: EXPECT_EQ(expected & 0xff, GetByteFromBuffer(writer, 0)); On 2017/06/08 19:40:10, asanka wrote: > As mentioned elsewhere, use literal buffers instead of constructions like this. > That way the tests are easier to evaluate and don't need to be conditioned on > the endianness. Done.
Pretty close. I think once we figure out the buffer ownership stuff for the writer and the string encoding issues for the Reader, I'll be okay with landing this. Let's not overspecify this API since it's likely to change as our requirements change as the implementation progresses. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h File net/http/ntlm.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode17 net/http/ntlm.h:17: struct SecurityBuffer { On 2017/06/12 23:12:04, zentaro wrote: > On 2017/06/08 18:36:33, Ryan Sleevi wrote: > > Document? > > Done. Add that the offset is relative to the start of the message. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode39 net/http/ntlm.h:39: enum NegotiateFlags { On 2017/06/12 23:12:04, zentaro wrote: > On 2017/06/08 18:44:57, asanka wrote: > > Let's make these typed enums. Here and elsewhere. > > Done. Thanks for doing this. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:193: // security buffer. On 2017/06/12 23:12:06, zentaro wrote: > On 2017/06/08 18:44:58, asanka wrote: > > It's not clear to me whether the an empty security buffer is all zeros. Some > > parts of the spec say that an empty buffer should be encoded with length=0, > and > > offset=<where the buffer would be if the length was not zero>. > > > > Have you looked at messages generated from a Windows box to see what they > > currently do? > > I've seen the part of the spec that says that but IMO it's ambiguous for several > reasons. (1) Do multiple empty buffers point to the same place? Do they all > point to the start of the payload section? (2) Do they point to the end of the > payload that preceded them in the header section? (3) Do they point to the end > of the payload section? Payloads can be in any order. They could even overlap I > assume. So any of the those alternatives could be considered correct. > > It seems like that is just a description of how it is implemented not a > meaningful requirement since no read will happen if it is zero length anyway. I > assume they just have a payload_upto variable in their implementation and write > it unconditionally. > > I haven't looked at a windows machine though (I suspect it does (2)). The > mozilla implementation of v1 and 2 write zero. > > So I don't think it matters and at least the current way it's implemented it > would need additional work to do option (2). It seems the spec if pretty consistent about requiring that the offset SHOULD point to where the buffer would've been if it were not empty. E.g.: "WorkstationBufferOffset field SHOULD be set to the offset from the beginning of the NEGOTIATE_MESSAGE to where the WorkstationName would be in Payload if it was present." https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:56: uint8_t* ReleaseBufferPtr() { On 2017/06/12 23:12:08, zentaro wrote: > On 2017/06/08 19:40:10, asanka wrote: > > Why not something like a std::vector<uint8_t> ? That way the caller doesn't > need > > to know exactly how big the buffer needs to be before the writes. > > If the output format was more general/variable like a file encoder I think a > vector which can size as needed would be better. But in this case the format is > small and well defined in size. I feel like it makes it easier to verify if it > is fixed. But the caller now needs to know the encoding overheads involved or overshoot considerably so it doesn't need to worry. Also if the caller needs to specify the buffer size, it might as well pass in a pointer to the buffer as well. That way there's no ambiguity around ownership as described below. > > Plus at the end of all this the ownership of of a uint8_t* needs to be given to > the auth handler so we'd have to make a copy to a naked buffer anyway. I'm not getting why we'd need to make a copy, but I'm also not super concerned about it since we need to b64 encode it anyway. But it's much more obvious from an API perspective to transfer a container rather than a raw pointer. Returning a unique_ptr<uint_8[]> would be an improvement in that it clearly conveys ownership and hard to get wrong, but a vector would also carry the size with it which makes consumption much easier. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:11: namespace net { Curious. Why aren't these in namespace ntlm {} ? https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:46: DCHECK(cursor <= GetLength()); DCHECK_LE https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:175: bool result = ReadBytes(buffer, sec_buf.length); Seems unnecessary to invoke ReadBytes() since we are undoing or ignoring everything it's doing other than the memcpy(). https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:271: SetCursor(old_cursor); I'm guessing that in practice all of these failure cases would be "discard the buffer and give up" cases and the recovery work that's being done here isn't necessary. There's nothing to salvage if a received buffer is invalid, right? If so, I think we can greatly simplify most of this code. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:74: bool ReadUInt16(uint16_t* value); To be pedantic, the Read*s, Skip*s, and Write*s should be annotated with WARN_UNUSED_RESULT. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:131: bool ReadUtf8Payload(std::string* value); Yeah, I was only referring to the Write* cases when I recommended always sticking with UTF 8. The Read* case is different in that we shouldn't do anything that relies on the buffer being valid UTF-8. Apologies if there was a miscommunication. For the Read* cases, it should be evident from the callsite that we are reading an octet stream of unknown encoding. I don't think even the UTF-16 version is safe in that it could still be invalid UTF-16. I think we can get away with not interpreting these strings, right? If so, let's stick to the ReadBytes*() and SkipBytes*() stuff and not introduce a dangerous API. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:72: bool WriteUInt16(uint16_t value); Same comment about WARN_UNUSED_RESULT. The buffer is pretty much a unsalvageable if any of these fail. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:82: // Casts |flags| to |uint32_t| and calls |WriteUInt32|. Unnecessary to describe an implementation detail.
Let me know if we're ready to do the code move to a new directory. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h File net/http/ntlm.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm.h#newcode17 net/http/ntlm.h:17: struct SecurityBuffer { On 2017/06/16 03:21:28, asanka wrote: > On 2017/06/12 23:12:04, zentaro wrote: > > On 2017/06/08 18:36:33, Ryan Sleevi wrote: > > > Document? > > > > Done. > > Add that the offset is relative to the start of the message. Done. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:193: // security buffer. On 2017/06/16 03:21:28, asanka wrote: > On 2017/06/12 23:12:06, zentaro wrote: > > On 2017/06/08 18:44:58, asanka wrote: > > > It's not clear to me whether the an empty security buffer is all zeros. Some > > > parts of the spec say that an empty buffer should be encoded with length=0, > > and > > > offset=<where the buffer would be if the length was not zero>. > > > > > > Have you looked at messages generated from a Windows box to see what they > > > currently do? > > > > I've seen the part of the spec that says that but IMO it's ambiguous for > several > > reasons. (1) Do multiple empty buffers point to the same place? Do they all > > point to the start of the payload section? (2) Do they point to the end of the > > payload that preceded them in the header section? (3) Do they point to the end > > of the payload section? Payloads can be in any order. They could even overlap > I > > assume. So any of the those alternatives could be considered correct. > > > > It seems like that is just a description of how it is implemented not a > > meaningful requirement since no read will happen if it is zero length anyway. > I > > assume they just have a payload_upto variable in their implementation and > write > > it unconditionally. > > > > I haven't looked at a windows machine though (I suspect it does (2)). The > > mozilla implementation of v1 and 2 write zero. > > > > So I don't think it matters and at least the current way it's implemented it > > would need additional work to do option (2). > > It seems the spec if pretty consistent about requiring that the offset SHOULD > point to where the buffer would've been if it were not empty. E.g.: > > "WorkstationBufferOffset field SHOULD be set to the offset from the beginning of > the > NEGOTIATE_MESSAGE to where the WorkstationName would be in Payload if it was > present." > Well it says the length MUST be zero but the offset SHOULD be 'where it would have been'. But it also says payloads can be in any order (and different implementations do use different orders) and that they can have arbitrary amounts of padding before and after each payload. So any value >= OLDEST_VERSION_HEADER_LENGTH could be 'where it would have been' depending on your implementation. "Payload data can be present in any order within the Payload field, with variable-length padding before or after the data. The data that can be present in the Payload field of this message, in no particular order, are:" I've changed this function to just match that the length is zero and the offset is within the message. This is only really used to validate during testing. We never have a reason to try and enforce that a field is empty for a real message. I'll change the writing code to always point to the payload. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:56: uint8_t* ReleaseBufferPtr() { On 2017/06/16 03:21:28, asanka wrote: > On 2017/06/12 23:12:08, zentaro wrote: > > On 2017/06/08 19:40:10, asanka wrote: > > > Why not something like a std::vector<uint8_t> ? That way the caller doesn't > > need > > > to know exactly how big the buffer needs to be before the writes. > > > > If the output format was more general/variable like a file encoder I think a > > vector which can size as needed would be better. But in this case the format > is > > small and well defined in size. I feel like it makes it easier to verify if it > > is fixed. > > But the caller now needs to know the encoding overheads involved or overshoot > considerably so it doesn't need to worry. Also if the caller needs to specify > the buffer size, it might as well pass in a pointer to the buffer as well. That > way there's no ambiguity around ownership as described below. > > > > > Plus at the end of all this the ownership of of a uint8_t* needs to be given > to > > the auth handler so we'd have to make a copy to a naked buffer anyway. > > I'm not getting why we'd need to make a copy, but I'm also not super concerned > about it since we need to b64 encode it anyway. But it's much more obvious from > an API perspective to transfer a container rather than a raw pointer. Returning > a unique_ptr<uint_8[]> would be an improvement in that it clearly conveys > ownership and hard to get wrong, but a vector would also carry the size with it > which makes consumption much easier. The caller needs to know the lengths anyway. The first thing written to the message are the security buffers that write the offset and length of each payload. So in order to write even the fixed size message header it already knows the length of the whole message. Having the caller supply the buffer also seems reasonable. (I did consider making the writer more complex and initializing with a 'payload_start_index' and maintaining a second cursor into the payload and adding additional checking that the header section didn't stray into the payload. But in the end it seemed like added complexity to deal with something the caller already knows) Plus in the NTLMv2 case the length of the NTLM response depends on multiple factors that rely on knowledge way outside the scope of this class that the caller already has to calculate. I updated to use a unique_ptr. The copy I was referring to is that HttpAuthHandlerNTLM::GetNextToken wants to be given ownership of a naked ptr. https://codereview.chromium.org/2879353002/diff/320001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:164: static size_t GetStringPayloadLength(const std::string& str, bool is_unicode); On 2017/06/08 18:36:34, Ryan Sleevi (slow through 6-27 wrote: > Do these need to be public-static? > > Could they just be in the unnamed namespace in the .cc? Why or why not? They > feel like an implementation detail :) Done. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:11: namespace net { On 2017/06/16 03:21:29, asanka wrote: > Curious. Why aren't these in namespace ntlm {} ? Oversight. Done. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:46: DCHECK(cursor <= GetLength()); On 2017/06/16 03:21:28, asanka wrote: > DCHECK_LE Done. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:175: bool result = ReadBytes(buffer, sec_buf.length); On 2017/06/16 03:21:28, asanka wrote: > Seems unnecessary to invoke ReadBytes() since we are undoing or ignoring > everything it's doing other than the memcpy(). Done. Was just trying to keep it consistent with the other Read*Payloads. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.cc:271: SetCursor(old_cursor); On 2017/06/16 03:21:28, asanka wrote: > I'm guessing that in practice all of these failure cases would be "discard the > buffer and give up" cases and the recovery work that's being done here isn't > necessary. There's nothing to salvage if a received buffer is invalid, right? > > If so, I think we can greatly simplify most of this code. I was just trying to leave things in a predictable state. But you're right in all the actual uses you never continue in this case. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... File net/http/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:74: bool ReadUInt16(uint16_t* value); On 2017/06/16 03:21:29, asanka wrote: > To be pedantic, the Read*s, Skip*s, and Write*s should be annotated with > WARN_UNUSED_RESULT. Done. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_r... net/http/ntlm_buffer_reader.h:131: bool ReadUtf8Payload(std::string* value); On 2017/06/16 03:21:29, asanka wrote: > Yeah, I was only referring to the Write* cases when I recommended always > sticking with UTF 8. The Read* case is different in that we shouldn't do > anything that relies on the buffer being valid UTF-8. Apologies if there was a > miscommunication. > > For the Read* cases, it should be evident from the callsite that we are reading > an octet stream of unknown encoding. I don't think even the UTF-16 version is > safe in that it could still be invalid UTF-16. > > I think we can get away with not interpreting these strings, right? If so, let's > stick to the ReadBytes*() and SkipBytes*() stuff and not introduce a dangerous > API. Read*String and Read*StringPayload methods were only actually used by tests to validate the content of messages we generated. So I removed them all since the actual implementation never needs to interpret these payloads. In the next CL I just added helpers to the test code to simplify the validation. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:72: bool WriteUInt16(uint16_t value); On 2017/06/16 03:21:29, asanka wrote: > Same comment about WARN_UNUSED_RESULT. The buffer is pretty much a unsalvageable > if any of these fail. Done. https://codereview.chromium.org/2879353002/diff/400001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.h:82: // Casts |flags| to |uint32_t| and calls |WriteUInt32|. On 2017/06/16 03:21:29, asanka wrote: > Unnecessary to describe an implementation detail. Done.
LGTM. Let's proceed. I'm still of the opinion that the buffer writer shouldn't take on the responsibility of allocating the buffer it is also not responsible for determining the length of the buffer. https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm.h File net/http/ntlm.h (right): https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm.h#newcode10 net/http/ntlm.h:10: #include <type_traits> https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm.h#newcode56 net/http/ntlm.h:56: using TFlagsInt = std::underlying_type<NegotiateFlags>::type; Nit: this can go inside the functions. There'll be two declarations, but they won't leak into net::ntlm
by "proceed" I meant with the directory move. We still need to make sure the new code doesn't get built on platforms where the portable ntlm implementation is not used.
https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.cc (right): https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:17: buffer_.reset(new uint8_t[buffer_len]); buffer_ now contains a default initialized array. I.e. the buffer is uninitialized since the default initialization for uint8_t is nothing. Instead, do: new uint8_t[buffer_len]() (note the extra set of parenthesis).
On 2017/06/23 20:05:33, asanka wrote: > https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm_buffer_w... > File net/http/ntlm_buffer_writer.cc (right): > > https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm_buffer_w... > net/http/ntlm_buffer_writer.cc:17: buffer_.reset(new uint8_t[buffer_len]); > buffer_ now contains a default initialized array. I.e. the buffer is > uninitialized since the default initialization for uint8_t is nothing. > > Instead, do: > > new uint8_t[buffer_len]() > > (note the extra set of parenthesis). Made those changes. Now starting the move.
https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm.h File net/http/ntlm.h (right): https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm.h#newcode10 net/http/ntlm.h:10: On 2017/06/23 17:59:14, asanka wrote: > #include <type_traits> Done. https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm.h#newcode56 net/http/ntlm.h:56: using TFlagsInt = std::underlying_type<NegotiateFlags>::type; On 2017/06/23 17:59:14, asanka wrote: > Nit: this can go inside the functions. There'll be two declarations, but they > won't leak into net::ntlm Done. https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm_buffer_w... File net/http/ntlm_buffer_writer.cc (right): https://codereview.chromium.org/2879353002/diff/460001/net/http/ntlm_buffer_w... net/http/ntlm_buffer_writer.cc:17: buffer_.reset(new uint8_t[buffer_len]); On 2017/06/23 20:05:32, asanka wrote: > buffer_ now contains a default initialized array. I.e. the buffer is > uninitialized since the default initialization for uint8_t is nothing. > > Instead, do: > > new uint8_t[buffer_len]() > > (note the extra set of parenthesis). Done.
https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/BUILD.gn File net/ntlm/BUILD.gn (right): https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/BUILD.gn#newc... net/ntlm/BUILD.gn:15: ] Eventually we can add a 'public' section here and only list the header file that declares the public API for the NTLM implementation. https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/BUILD.gn#newc... net/ntlm/BUILD.gn:26: "//net/ntlm", :ntlm https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/OWNERS File net/ntlm/OWNERS (right): https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/OWNERS#newcode1 net/ntlm/OWNERS:1: zentaro@chromium.org Add: # COMPONENT: Internals>Network>Auth
https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/BUILD.gn File net/ntlm/BUILD.gn (right): https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/BUILD.gn#newc... net/ntlm/BUILD.gn:15: ] On 2017/07/06 03:36:01, asanka wrote: > Eventually we can add a 'public' section here and only list the header file that > declares the public API for the NTLM implementation. Acknowledged. https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/BUILD.gn#newc... net/ntlm/BUILD.gn:26: "//net/ntlm", On 2017/07/06 03:36:01, asanka wrote: > :ntlm Done. https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/OWNERS File net/ntlm/OWNERS (right): https://codereview.chromium.org/2879353002/diff/560001/net/ntlm/OWNERS#newcode1 net/ntlm/OWNERS:1: zentaro@chromium.org On 2017/07/06 03:36:01, asanka wrote: > Add: > > # COMPONENT: Internals>Network>Auth Done.
The CQ bit was checked by zentaro@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by zentaro@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
You know things are good when we're in the comment-nit phase :) LGTM with lots of nits :) https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/OWNERS File net/ntlm/OWNERS (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/OWNERS#newcode1 net/ntlm/OWNERS:1: # COMPONENT: Internals>Network>Auth Could you also add # TEAM: net-dev@chromium.org And looking at other files, this appears to tend to be at the bottom of the file, rather than the top. Finally, alphasort :) https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... File net/ntlm/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.cc:64: bool NtlmBufferReader::ReadUInt(T* value) { Please keep the order of declarations and definitions consistent (i.e. have the .cc match the .h :D) (See "Function declaration order" in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md ) https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... File net/ntlm/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:23: // The class supports sequential read of a provided buffer. All reads perform s/supports sequential/supports the sequential/ Alternatively s/sequential read/sequential reading/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:51: // This class does not take ownership of |ptr| so the caller must ensure s/|ptr| so/|ptr|, so/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:75: // more bits available it returns false. s/available it/available, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:79: // more bits available it returns false. s/available it/available, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:83: // more bits available it returns false. s/available it/available, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:95: // buffer then the call fails. s/buffer then/buffer, then/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:126: // Skips over |count| bytes in the buffer. Returns false if there is not s/is not/are not/ (subject/verb agreement with plural bytes) https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:148: // the offset is within the message. On failure the buffer is invalid s/failure the/failure, the/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:158: // cursor to the start of the payload indicated by the security buffer. comment nit: You're describing how this is used (an implementation detail), rather than how to use it (a documentation detail) That said, I'm not sure how to reword this better, so I'm happy to leave as-is, unless asanka@ has suggestions. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... File net/ntlm/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:73: // more bits available in the buffer it returns false. s/buffer it/buffer, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:77: // more bits available in the buffer it returns false. s/buffer it/buffer, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:81: // more bits available in the buffer it returns false. s/buffer it/buffer, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:88: // the buffer it returns false. s/buffer it/buffer, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:92: // bytes in the buffer it returns false. s/buffer it/buffer, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:96: // more bytes in available in the buffer it returns false. s/buffer it/buffer, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:111: // When unicode was not negotiated only the hostname string will go through s/unicode/Unicode/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:119: // If unicode was not negotiated, username and domain get converted to UTF8 s/unicode/Unicode/ s/username and domain/the username and domain/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:121: // hash the encoding doesn't matter. In practice only a very old or non- s/hash the/hash, the/ s/practice only/practice, only/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:122: // windows server might trigger this code path since we always attempt to s/windows/Windows/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:123: // negotiate unicode and servers are supposed to honor that request. s/unicode/Unicode/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:131: // One case is the hostname. When the the unicode flag has been set during s/unicode/Unicode/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:132: // negotiation the hostname needs to appear in the message with 16 bit s/negotiation the/negotiation, the/ s/16 bit/16-bit/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:133: // chars. The high byte of each char is just set to 0. s/chars/characters/ ? This one is iffy. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:135: // The other case is the spn. With EPA enabled it appears in the target info s/spn/Service Principal Name/ ? s/EPA/Extended Protection for Authentication/ s/enabled it/enabled, it/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:136: // inside an AV Pair where strings always have 16 bit chars. s/Pair where/Pair, where/ s/16 bit/16-bit/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:139: // Writes UTF-16 LE characters to the buffer. For these strings such as s/strings such/strings, such/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:140: // username and domain the actual encoding isn't really important. They are s/domain the/domain, the/ s/really// s/important. They/important; they/ https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_constants.h File net/ntlm/ntlm_constants.h (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_constant... net/ntlm/ntlm_constants.h:27: SecurityBuffer() : SecurityBuffer(0, 0) {} style nit: Maybe not a newline on 26, but instead on 27 (e.g. both ctors w/o newlines, then members after newline)
https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/OWNERS File net/ntlm/OWNERS (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/OWNERS#newcode1 net/ntlm/OWNERS:1: # COMPONENT: Internals>Network>Auth On 2017/07/10 15:07:56, Ryan Sleevi wrote: > Could you also add > > # TEAM: mailto:net-dev@chromium.org > > And looking at other files, this appears to tend to be at the bottom of the > file, rather than the top. > > Finally, alphasort :) Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... File net/ntlm/ntlm_buffer_reader.cc (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.cc:64: bool NtlmBufferReader::ReadUInt(T* value) { On 2017/07/10 15:07:56, Ryan Sleevi wrote: > Please keep the order of declarations and definitions consistent (i.e. have the > .cc match the .h :D) > > (See "Function declaration order" in > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md ) Done. Here and the writer. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... File net/ntlm/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:23: // The class supports sequential read of a provided buffer. All reads perform On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/supports sequential/supports the sequential/ > > Alternatively > s/sequential read/sequential reading/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:51: // This class does not take ownership of |ptr| so the caller must ensure On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/|ptr| so/|ptr|, so/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:75: // more bits available it returns false. On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/available it/available, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:79: // more bits available it returns false. On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/available it/available, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:83: // more bits available it returns false. On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/available it/available, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:95: // buffer then the call fails. On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/buffer then/buffer, then/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:126: // Skips over |count| bytes in the buffer. Returns false if there is not On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/is not/are not/ (subject/verb agreement with plural bytes) Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:148: // the offset is within the message. On failure the buffer is invalid On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/failure the/failure, the/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:158: // cursor to the start of the payload indicated by the security buffer. On 2017/07/10 15:07:56, Ryan Sleevi wrote: > comment nit: You're describing how this is used (an implementation detail), > rather than how to use it (a documentation detail) > > That said, I'm not sure how to reword this better, so I'm happy to leave as-is, > unless asanka@ has suggestions. Acknowledged. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... File net/ntlm/ntlm_buffer_writer.h (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:73: // more bits available in the buffer it returns false. On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/buffer it/buffer, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:77: // more bits available in the buffer it returns false. On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/buffer it/buffer, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:81: // more bits available in the buffer it returns false. On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/buffer it/buffer, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:88: // the buffer it returns false. On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/buffer it/buffer, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:92: // bytes in the buffer it returns false. On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/buffer it/buffer, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:96: // more bytes in available in the buffer it returns false. On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/buffer it/buffer, it/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:111: // When unicode was not negotiated only the hostname string will go through On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/unicode/Unicode/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:119: // If unicode was not negotiated, username and domain get converted to UTF8 On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/unicode/Unicode/ > s/username and domain/the username and domain/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:121: // hash the encoding doesn't matter. In practice only a very old or non- On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/hash the/hash, the/ > s/practice only/practice, only/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:122: // windows server might trigger this code path since we always attempt to On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/windows/Windows/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:123: // negotiate unicode and servers are supposed to honor that request. On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/unicode/Unicode/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:131: // One case is the hostname. When the the unicode flag has been set during On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/unicode/Unicode/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:132: // negotiation the hostname needs to appear in the message with 16 bit On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/negotiation the/negotiation, the/ > s/16 bit/16-bit/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:133: // chars. The high byte of each char is just set to 0. On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/chars/characters/ ? This one is iffy. Done. The comment about high bytes set to 0 was stale. Removed. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:136: // inside an AV Pair where strings always have 16 bit chars. On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/Pair where/Pair, where/ > s/16 bit/16-bit/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:139: // Writes UTF-16 LE characters to the buffer. For these strings such as On 2017/07/10 15:07:56, Ryan Sleevi wrote: > s/strings such/strings, such/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_w... net/ntlm/ntlm_buffer_writer.h:140: // username and domain the actual encoding isn't really important. They are On 2017/07/10 15:07:57, Ryan Sleevi wrote: > s/domain the/domain, the/ > s/really// > s/important. They/important; they/ Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_constants.h File net/ntlm/ntlm_constants.h (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_constant... net/ntlm/ntlm_constants.h:27: SecurityBuffer() : SecurityBuffer(0, 0) {} On 2017/07/10 15:07:57, Ryan Sleevi wrote: > style nit: Maybe not a newline on 26, but instead on 27 (e.g. both ctors w/o > newlines, then members after newline) Done.
https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... File net/ntlm/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:96: bool ReadBytesFrom(const SecurityBuffer& sec_buf, Should note that this method doesn't affect the cursor. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:158: // cursor to the start of the payload indicated by the security buffer. On 2017/07/11 14:00:29, zentaro wrote: > On 2017/07/10 15:07:56, Ryan Sleevi wrote: > > comment nit: You're describing how this is used (an implementation detail), > > rather than how to use it (a documentation detail) > > > > That said, I'm not sure how to reword this better, so I'm happy to leave > as-is, > > unless asanka@ has suggestions. > > Acknowledged. I'd be okay with leaving out the "this is used when ..." part which will quickly become incorrect if we add a call site. The caveat about the caller being responsible for bounds checking is good but strictly speaking isn't enough. I'd note that the caller should only set the cursor to a known field boundary as returned by a previous call to GetCursor(). BTW, is this called at all?
https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... File net/ntlm/ntlm_buffer_reader.h (right): https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:96: bool ReadBytesFrom(const SecurityBuffer& sec_buf, On 2017/07/11 15:19:16, asanka wrote: > Should note that this method doesn't affect the cursor. Done. https://codereview.chromium.org/2879353002/diff/620001/net/ntlm/ntlm_buffer_r... net/ntlm/ntlm_buffer_reader.h:158: // cursor to the start of the payload indicated by the security buffer. On 2017/07/11 15:19:16, asanka wrote: > On 2017/07/11 14:00:29, zentaro wrote: > > On 2017/07/10 15:07:56, Ryan Sleevi wrote: > > > comment nit: You're describing how this is used (an implementation detail), > > > rather than how to use it (a documentation detail) > > > > > > That said, I'm not sure how to reword this better, so I'm happy to leave > > as-is, > > > unless asanka@ has suggestions. > > > > Acknowledged. > > I'd be okay with leaving out the "this is used when ..." part which will quickly > become incorrect if we add a call site. The caveat about the caller being > responsible for bounds checking is good but strictly speaking isn't enough. I'd > note that the caller should only set the cursor to a known field boundary as > returned by a previous call to GetCursor(). > > BTW, is this called at all? Removed the "this is used" because we removed all cursor rollback and moved to using ReadFrom. So it was wrong anyway. It is currently called in one place AdvanceCursor.
The CQ bit was checked by zentaro@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zentaro@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zentaro@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zentaro@chromium.org
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/2879353002/#ps810001 (title: "Remove ReleaseBufferPtr")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 810001, "attempt_start_ts": 1499991053968700,
"parent_rev": "442c8bcfb36c27135e6918f7ff352e23de4413e0", "commit_rev":
"1024d46b5c84399aa4815b124877d5eec327de63"}
CQ is committing da patch.
Bot data: {"patchset_id": 810001, "attempt_start_ts": 1499991053968700,
"parent_rev": "0c6966dfc39175050e55d5ffb90a885650bdb45d", "commit_rev":
"2ac04b3f1945ba81666433c9c5b31d98e84c1f99"}
CQ is committing da patch.
Bot data: {"patchset_id": 810001, "attempt_start_ts": 1499991053968700,
"parent_rev": "8aee8166f3e2d569971a5866734cc017b52483eb", "commit_rev":
"b747956839b2d8621c5267c44a9fe7f8765cc2d6"}
Message was sent while issue was closed.
Description was changed from ========== Add a buffer reader/writer for NTLM. These will be used in new implementation of NTLMv2 and consolidate the ad hoc buffer manipulation in the new and existing code. Additionally useful as helpers for generating/validating data for unit tests. BUG=chromium:22532 TEST=builds and tests pass ========== to ========== Add a buffer reader/writer for NTLM. These will be used in new implementation of NTLMv2 and consolidate the ad hoc buffer manipulation in the new and existing code. Additionally useful as helpers for generating/validating data for unit tests. BUG=chromium:22532 TEST=builds and tests pass Review-Url: https://codereview.chromium.org/2879353002 Cr-Commit-Position: refs/heads/master@{#486565} Committed: https://chromium.googlesource.com/chromium/src/+/b747956839b2d8621c5267c44a9f... ==========
Message was sent while issue was closed.
Committed patchset #42 (id:810001) as https://chromium.googlesource.com/chromium/src/+/b747956839b2d8621c5267c44a9f... |
