-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix - User Principal Name (UPN) usernames should send up an empty "TargetName" in the authentication message #6
Conversation
identifying in the authentication message encoding process
message encoders to use the new method of target name identification
* If the username is in the "UPN" (Kerberos) format, the target name should be empty | ||
* | ||
* @link https://msdn.microsoft.com/en-us/library/windows/desktop/aa380525(v=vs.85).aspx | ||
* @link http://davenport.sourceforge.net/ntlm.html#nameVariations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice nice.
LGTM 👍 ty for doing this. |
* @link https://msdn.microsoft.com/en-us/library/windows/desktop/aa380525(v=vs.85).aspx | ||
* @link http://davenport.sourceforge.net/ntlm.html#nameVariations | ||
*/ | ||
if (strpos($username, static::USER_PRINCIPAL_NAME_SEPARATOR)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed this in the doc for strpos
: http://php.net/manual/en/function.strpos.php#80363
As strpos may return either FALSE (substring absent) or 0 (substring at start of string), strict versus loose equivalency operators must be used very carefully.
We're good with this "truthy" style comparison here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh whoops! Weird, I'm usually the one reminding people of this one. Yea, great call. Let me fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
strict equals, thanks to @mcos reminding me. 👍
STILL lgtm 👍 |
LGTM 👍 |
…-empty-targetname Bugfix - User Principal Name (UPN) usernames should send up an empty "TargetName" in the authentication message
❤️ Thanks! |
Listing a few projects/code-snippets here for later reference: |
This one was confusing/frustrating. Why? Probably because it is, again, a behavior that is: undocumented in the specification, seemingly implicit, and somewhat nonsensical.
So, as we've known for quite some time, there are 2 formats for "user name" designations:
DOMAIN\username
username@dnsdomainname.com
These formats are even documented on MSDN.
While this library doesn't attempt to hold anyone's hand in how these designations are separated into their respective username and domain name values (although maybe it should at some point), we've known for quite a while that we should separate these format values into two distinct values. Currently, in an application that we've built using this library, we look for any
\
character or@
character and separate the strings into$domain
and$username
values, however we've noticed some authentication errors for certain setups when using this method of inference.This style of data separation seems to even be supported by the aforementioned documentation (note the documents use of "part" designation). However, after some more extensive testing, it's become obvious that this just doesn't work. Unfortunately, we've instead noticed that domains and usernames should only be inferred from these compound representations when in the first, "Down-Level Logon Name" format. Not only does our testing support this, but the thankfully incredible unofficial documentation provided by Eric Glass (Davenport) also supports this practice:
So, contrary to any official documentation or sense, we'll just have to trust both the reverse-engineered Eric Glass documentation and the functional integration testing that we've completed here.
NTLM... you're quite the challenge.