NTLM authentication with SSPI on windows #2871
Conversation
…hentication process
|
I'm relatively new to Go, so any help to bring this into a state that could be merged in would be really appreciated 😄. |
stffabi
force-pushed
the
sspi-ntlm
branch
2 times, most recently
from
23fbd9b to
474c8db
Compare
There was a problem hiding this comment.
Hi @stffabi, thanks for taking this on! Sorry that it's taken me a bit to look it over. I think that this change looks great to me, aside from a few minor comments pertaining to when to define methods and return types in terms of *T rather than T.
Can you take a look and ping me?
lfsapi/ntlm.go
Outdated
|
|
||
| // Defines if the default credentials of the currently logged in user | ||
| // should be used. | ||
| func (c ntmlCredentials) IsDefaultCredentials() bool { |
There was a problem hiding this comment.
Could you define this method to be received on the pointer type, like:
func (c *ntmlCredentials) IsDefaultCredentials() bool { ... }
// ^
lfsapi/ntlm.go
Outdated
| // Defines if the default credentials of the currently logged in user | ||
| // should be used. | ||
| func (c ntmlCredentials) IsDefaultCredentials() bool { | ||
| return c == ntmlCredentials{} |
There was a problem hiding this comment.
Going off of the above, I think that it might be clearer to define the body as:
return c.domain == "" && c.username == "" && c.password == ""
lfsapi/ntlm.go
Outdated
| @@ -163,4 +125,21 @@ func rewoundRequestBody(req *http.Request) (io.ReadCloser, error) { | |||
| return body, err | |||
| } | |||
|
|
|||
| const ntlmNegotiateMessage = "NTLM TlRMTVNTUAABAAAAB7IIogwADAAzAAAACwALACgAAAAKAAAoAAAAD1dJTExISS1NQUlOTk9SVEhBTUVSSUNB" | |||
| func ntlmGetCredentials(creds Creds) (ntmlCredentials, error) { | |||
There was a problem hiding this comment.
I think going along with my suggestion about this method could return a (*ntlmCredentials, error), which would contain a 👍 return value indicating the absence of NTLM credentials.
There was a problem hiding this comment.
@ttaylorr thanks so much for reviewing the code. I really appreciate your feedback.
I don't quite understand what you mean by "which would contain a (thumbsup) return value". Do you mean that we could return (nil, nil) instead of (&ntlmCredentials{}, nil) in case no NTLM credentials were provided?
This would also mean we could get rid of IsDefaultCredentials, so no NTLM credentials would for windows simply mean use the default ones of the current user.
There was a problem hiding this comment.
I don't quite understand what you mean by "which would contain a (thumbsup) return value". Do you mean that we could return
(nil, nil)instead of(&ntlmCredentials{}, nil)in case no NTLM credentials were provided?
Your intuition is totally correct -- and my apologies for not being as clear as I could have been. My intention was to suggest that nil is a clearer representation of "I couldn't find credentials" than an empty ntlmCredentials instance.
This would also mean we could get rid of
IsDefaultCredentials, so no NTLM credentials would for windows simply mean use the default ones of the current user.
Indeed 👍.
This brings support for SingleSignOn on windows using the default credentials of the currently logged in user if an empty username and empty password is provided from gitcredentials. This plays well with the Git for windows implementation which stores an empty username and password if it should use the default credentials.
|
@ttaylorr I've fixed your suggestions and it's ready for another review. |
This introduces the use of SSPI on windows to authenticate via NTLM. This allows git-lfs to authenticate through NTLM by using the default credentials of the current logged in user in the case GCM returns empty credentials.
Fixes #2234