code review 8541047: os/user: faster user lookup on Windows

os/user: faster user lookup on Windows

Trying to lookup user's display name with directory services can
take several seconds when user's computer is not in a domain.
As a workaround, check if computer is joined in a domain first,
and don't use directory services if it is not.
Additionally, don't leak tokens in user.Current().
Fixes issue 5298.

[snaury, 2013-04-17 16:29:28]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-04-17 16:30:38]

The issue discussed a Windows API to ask whether your computer was part of
a domain. Why aren't you using that?

Is checking whether the computer's name == domain name a good heuristic for
domain membership?  Seems non-ideal.



On Wed, Apr 17, 2013 at 9:29 AM, <snaury@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> os/user: faster user lookup on Windows
>
> Trying to lookup user's display name with directory services can
> take a several seconds when user's computer is not in a domain.
> As a workaround, check if domain name matches computer name
> and don't use directory services for local user names.
> Fixes issue 5298.
>
> Please review this at
https://codereview.appspot.**com/8541047/<https://codereview.appspot.com/8541...
>
> Affected files:
>   M src/pkg/os/user/lookup_**windows.go
>
>
> Index: src/pkg/os/user/lookup_**windows.go
> ==============================**==============================**=======
> --- a/src/pkg/os/user/lookup_**windows.go
> +++ b/src/pkg/os/user/lookup_**windows.go
> @@ -6,38 +6,43 @@
>
>  import (
>         "fmt"
> +       "strings"
>         "syscall"
>         "unsafe"
>  )
>
>  func lookupFullName(domain, username, domainAndUser string) (string,
> error) {
> -       // try domain controller first
> -       name, e := syscall.TranslateAccountName(**domainAndUser,
> -               syscall.NameSamCompatible, syscall.NameDisplay, 50)
> +       computer, e := syscall.ComputerName()
> +       if e != nil || !strings.EqualFold(domain, computer) {
> +               // try domain controller first
> +               name, e := syscall.TranslateAccountName(**domainAndUser,
> +                       syscall.NameSamCompatible, syscall.NameDisplay, 50)
> +               if e == nil {
> +                       return name, nil
> +               }
> +       }
> +       // domain lookup failed or it's a local computer
> +       d, e := syscall.UTF16PtrFromString(**domain)
>         if e != nil {
> -               // domain lookup failed, perhaps this pc is not part of
> domain
> -               d, e := syscall.UTF16PtrFromString(**domain)
> -               if e != nil {
> -                       return "", e
> -               }
> -               u, e := syscall.UTF16PtrFromString(**username)
> -               if e != nil {
> -                       return "", e
> -               }
> -               var p *byte
> -               e = syscall.NetUserGetInfo(d, u, 10, &p)
> -               if e != nil {
> -                       // path executed when a domain user is
> disconnected from the domain
> -                       // pretend username is fullname
> -                       return username, nil
> -               }
> -               defer syscall.NetApiBufferFree(p)
> -               i := (*syscall.UserInfo10)(unsafe.**Pointer(p))
> -               if i.FullName == nil {
> -                       return "", nil
> -               }
> -               name = syscall.UTF16ToString((*[1024]**
> uint16)(unsafe.Pointer(i.**FullName))[:])
> +               return "", e
>         }
> +       u, e := syscall.UTF16PtrFromString(**username)
> +       if e != nil {
> +               return "", e
> +       }
> +       var p *byte
> +       e = syscall.NetUserGetInfo(d, u, 10, &p)
> +       if e != nil {
> +               // path executed when a domain user is disconnected from
> the domain
> +               // pretend username is fullname
> +               return username, nil
> +       }
> +       defer syscall.NetApiBufferFree(p)
> +       i := (*syscall.UserInfo10)(unsafe.**Pointer(p))
> +       if i.FullName == nil {
> +               return "", nil
> +       }
> +       name := syscall.UTF16ToString((*[1024]**uint16)(unsafe.Pointer(i.*
> *FullName))[:])
>         return name, nil
>  }
>
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[snaury, 2013-04-17 16:50:09]

On 2013/04/17 16:30:38, bradfitz wrote:
> The issue discussed a Windows API to ask whether your computer was part of
> a domain. Why aren't you using that?

Several reasons. First, I would need to add a new API (GetNetworkParams) to
syscall, rather complex structure and code to marshal that, as well as probably
sync.Once to call that only once. Next, if machine is part of a domain doesn't
really tell us anything: logged in user might be a local user on a home network,
and the problem would be the same, trying to query domain controller when
there's none.

It's very difficult to have a computer name same as a domain name, unless you
are on a domain controller (I can't test what really happens then), and if you
are looking up an account on a local computer, then you have to use
NetUserGetInfo anyway.

The ideal solution would probably be to compare sid prefixes between user and
computer, but I'm not sure which API can give us sid of a local computer.

> Is checking whether the computer's name == domain name a good heuristic for
> domain membership?  Seems non-ideal.

Since user name is only remote if it's not on a current computer it's probably
good, though I'm not entirely sure. :(

[bradfitz, 2013-04-17 16:53:46]

On Wed, Apr 17, 2013 at 9:50 AM, <snaury@gmail.com> wrote:

> On 2013/04/17 16:30:38, bradfitz wrote:
>
>> The issue discussed a Windows API to ask whether your computer was
>>
> part of
>
>> a domain. Why aren't you using that?
>>
>
> Several reasons. First, I would need to add a new API (GetNetworkParams)
> to syscall, rather complex structure and code to marshal that, as well
> as probably sync.Once to call that only once.


Those reasons alone aren't compelling enough. We can help you with those.

Let's figure out what's the correct behavior first, and then worry about
implementation.


> Next, if machine is part
> of a domain doesn't really tell us anything: logged in user might be a
> local user on a home network, and the problem would be the same, trying
> to query domain controller when there's none.
>
> It's very difficult to have a computer name same as a domain name,
> unless you are on a domain controller (I can't test what really happens
> then), and if you are looking up an account on a local computer, then
> you have to use NetUserGetInfo anyway.
>
> The ideal solution would probably be to compare sid prefixes between
> user and computer, but I'm not sure which API can give us sid of a local
> computer.
>
>
>  Is checking whether the computer's name == domain name a good
>>
> heuristic for
>
>> domain membership?  Seems non-ideal.
>>
>
> Since user name is only remote if it's not on a current computer it's
> probably good, though I'm not entirely sure. :(
>
>
https://codereview.appspot.**com/8541047/<https://codereview.appspot.com/8541...
>

[snaury, 2013-04-17 17:07:42]

On 2013/04/17 16:53:46, bradfitz wrote:
> Let's figure out what's the correct behavior first, and then worry about
> implementation.

The correct behavior is rather obvious. If username is on current or another
computer, we should use NetUserGetInfo. If username is on a domain, we should
use TranslateAccountName. The problem is how to distinguish between computer and
domain names. :)

[brainman, 2013-04-18 01:51:56]

On 2013/04/17 16:50:09, snaury wrote:
> 
> ..., if machine is part of a domain doesn't
> really tell us anything: logged in user might be a local user on a home
network,
> and the problem would be the same, trying to query domain controller when
> there's none.
> 

What about using NetGetJoinInformation (http://play.golang.org/p/bFA02H1cwE)?
The function will help us to determine, if computer is part of a domain. And
will tell us what the domain name is, if PC is part of a domain.

Here is our logic:

if !PC_is_part_of_a_domain {
	// PC is not part of domain
	lookup_account_localy()
	return
}
if PC_domain_name != USER_LOGIN_domain_name {
	// PC is part of domain, but user is logged in locally
	lookup_account_localy()
	return
}
lookup_account_in_domain()
return

Perhaps, lookup_account_in_domain could still be slow, if user is logged in
without domain controller present. But, I believe, nothing we could do about
that - that case is undetectable. Also, since user is logged in with some
"cached credentials", perhaps our calls won't be slow after all. Perhaps, they
will use some "cached" information instead of looking for domain controllers.

Alex

[snaury, 2013-04-18 04:13:41]

On 2013/04/18 01:51:56, brainman wrote:
> What about using NetGetJoinInformation

Oh! It's actually very interesting!

> if PC_domain_name != USER_LOGIN_domain_name {
> 	// PC is part of domain, but user is logged in locally
> 	lookup_account_localy()
> 	return
> }

It is possible to have multiple domains on the network. Such a check would work
for user.Current(), but would not return correct data for
user.Lookup("OTHERDOMAIN\\username").

Maybe we don't even have to compare any strings, but use a much simpler logic
based on BufferType: always use lookup_account_in_domain  first if
BufferType==NetSetupDomainName, and never otherwise. What do you think?

[lucio, 2013-04-18 04:20:19]

Out of curiosity from somebody who seldom uses Windows, what goes
wrong if one does an unconditional "local lookup" and the host is part
of a domain?

Lucio.

On 4/18/13, snaury@gmail.com <snaury@gmail.com> wrote:
> On 2013/04/18 01:51:56, brainman wrote:
>> What about using NetGetJoinInformation
>
> Oh! It's actually very interesting!
>
>> if PC_domain_name != USER_LOGIN_domain_name {
>> 	// PC is part of domain, but user is logged in locally
>> 	lookup_account_localy()
>> 	return
>> }
>
> It is possible to have multiple domains on the network. Such a check
> would work for user.Current(), but would not return correct data for
> user.Lookup("OTHERDOMAIN\\username").
>
> Maybe we don't even have to compare any strings, but use a much simpler
> logic based on BufferType: always use lookup_account_in_domain  first if
> BufferType==NetSetupDomainName, and never otherwise. What do you think?
>
> https://codereview.appspot.com/8541047/
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

[brainman, 2013-04-18 04:43:52]

On 2013/04/18 04:13:41, snaury wrote:
> 
> Maybe we don't even have to compare any strings, but use a much simpler logic
> based on BufferType: always use lookup_account_in_domain  first if
> BufferType==NetSetupDomainName, and never otherwise. What do you think?

SGTM. But, I think we'll be chasing our tail again year from now :-)

Alex

[brainman, 2013-04-18 04:47:01]

On 2013/04/18 04:20:19, lucio wrote:
> ... what goes
> wrong if one does an unconditional "local lookup" and the host is part
> of a domain?

The api fails. Please, do not ask me why, because I do not know? :-)

Alex

[lucio, 2013-04-18 04:52:18]

On 4/18/13, alex.brainman@gmail.com <alex.brainman@gmail.com> wrote:
> On 2013/04/18 04:20:19, lucio wrote:
>> ... what goes
>> wrong if one does an unconditional "local lookup" and the host is part
>> of a domain?
>
> The api fails. Please, do not ask me why, because I do not know? :-)
>
If that is a quick response, is it not sufficient indication that a
subsequent lookup in the domain is going to be quick?  That would
address the original problem, would it not?

Or does everything fall apart instead?

Lucio.

[brainman, 2013-04-18 05:05:41]

On 2013/04/18 04:52:18, lucio wrote:
> >
> If that is a quick response, ...

It is slow on my computer.

Alex

[lucio, 2013-04-18 05:42:25]

The I guess it does precisely what snaury is trying to do and, in all
likelyhood, there isn't a better way.  But that's just speculation.

Lucio.

On 4/18/13, alex.brainman@gmail.com <alex.brainman@gmail.com> wrote:
> On 2013/04/18 04:52:18, lucio wrote:
>> >
>> If that is a quick response, ...
>
> It is slow on my computer.
>
> Alex
>
> https://codereview.appspot.com/8541047/
>

[snaury, 2013-04-22 19:48:21]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, alex.brainman@gmail.com,
lucio.dere@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[snaury, 2013-04-22 20:10:55]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, alex.brainman@gmail.com,
lucio.dere@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2013-04-23 00:59:25]

I think you made mistake with pointers. Your NetGetJoinInformation call always
fails with "The parameter is incorrect" error. I moved all windows api
prototypes into syscall http://code.google.com/p/go/issues/detail?id=5298#c5
(like we always do), and that helped to clear the issue. Please, accept my
changes.

Alex

https://codereview.appspot.com/8541047/diff/27001/src/pkg/os/user/lookup_wind...
File src/pkg/os/user/lookup_windows.go (right):

https://codereview.appspot.com/8541047/diff/27001/src/pkg/os/user/lookup_wind...
src/pkg/os/user/lookup_windows.go:61: // pretend username is fullname
This comment is misleading now. pc is not part of any domain, if we are here.

https://codereview.appspot.com/8541047/diff/27001/src/pkg/os/user/lookup_wind...
src/pkg/os/user/lookup_windows.go:116: defer t.Close()
Good catch.

[snaury, 2013-04-23 05:40:18]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, alex.brainman@gmail.com,
lucio.dere@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[snaury, 2013-04-23 05:43:41]

On 2013/04/23 00:59:25, brainman wrote:
> Please, accept my changes.

Done.

> pc is not part of any domain, if we are here.

Not necessarily. If computer is part of a domain this code is tried as well
(just like before). But comment was misplaced, so I fixed it this time.

> Good catch.

Was looking for leaks in my changes, found a token leak instead. %)

[brainman, 2013-04-24 07:00:58]

LGTM

Thank you very much.

We are not permitted to submit now, so leaving it for others. I suppose it can
wait until after go1.1, if we are too close.

Alex

[brainman, 2013-05-15 02:57:21]

Any more reviews? Can I submit now?

Alex

[bradfitz, 2013-05-15 03:20:56]

https://codereview.appspot.com/8541047/diff/33001/src/pkg/syscall/security_wi...
File src/pkg/syscall/security_windows.go (right):

https://codereview.appspot.com/8541047/diff/33001/src/pkg/syscall/security_wi...
src/pkg/syscall/security_windows.go:62: // do not reorder
If these are constants defined elsewhere (i.e. in Windows header files), then
iota is the wrong mechanism.  You should instead say = 0, = 1, = 2, = 3.

[bradfitz, 2013-05-15 03:21:26]

Submit away once you're happy with it.  I left one comment, which you can
fix later in a subsequent commit if you'd like.



On Tue, May 14, 2013 at 7:57 PM, <alex.brainman@gmail.com> wrote:

> Any more reviews? Can I submit now?
>
> Alex
>
>
https://codereview.appspot.**com/8541047/<https://codereview.appspot.com/8541...
>

[brainman, 2013-05-15 03:25:09]

*** Submitted as https://code.google.com/p/go/source/detail?r=2cf178b97603 ***

os/user: faster user lookup on Windows

Trying to lookup user's display name with directory services can
take several seconds when user's computer is not in a domain.
As a workaround, check if computer is joined in a domain first,
and don't use directory services if it is not.
Additionally, don't leak tokens in user.Current().
Fixes issue 5298.

R=golang-dev, bradfitz, alex.brainman, lucio.dere
CC=golang-dev
https://codereview.appspot.com/8541047

Committer: Alex Brainman <alex.brainman@gmail.com>

[brainman, 2013-05-15 03:29:58]

On 2013/05/15 03:20:56, bradfitz wrote:
> If these are constants defined elsewhere (i.e. in Windows header files), then
> iota is the wrong mechanism.  You should instead say = 0, = 1, = 2, = 3.

We have a few of them

syscall # grep iota *.go
security_windows.go:    NetSetupUnknownStatus = iota
security_windows.go:    SidTypeUser = 1 + iota
security_windows.go:    TOKEN_ASSIGN_PRIMARY = 1 << iota
security_windows.go:    TokenUser = 1 + iota
zerrors_windows.go:     E2BIG Errno = APPLICATION_ERROR + iota
ztypes_windows.go:      FILE_NOTIFY_CHANGE_FILE_NAME = 1 << iota
ztypes_windows.go:      FILE_ACTION_ADDED = iota + 1
ztypes_windows.go:      HKEY_CLASSES_ROOT = 0x80000000 + iota
ztypes_windows.go:      REG_NONE = iota

I like to leave them as they are. We have comments to warn future users.

But if you insist that it is wrong, I will change them all.

Alex

[bradfitz, 2013-05-15 03:36:09]

I would slightly prefer they all be changed.

To me (and I think to others?), "iota" generally means "I want some unique
numbers, but I don't really care what they are".

But I don't care enough, unless somebody else agrees.




On Tue, May 14, 2013 at 8:29 PM, <alex.brainman@gmail.com> wrote:

> On 2013/05/15 03:20:56, bradfitz wrote:
>
>> If these are constants defined elsewhere (i.e. in Windows header
>>
> files), then
>
>> iota is the wrong mechanism.  You should instead say = 0, = 1, = 2, =
>>
> 3.
>
> We have a few of them
>
> syscall # grep iota *.go
> security_windows.go:    NetSetupUnknownStatus = iota
> security_windows.go:    SidTypeUser = 1 + iota
> security_windows.go:    TOKEN_ASSIGN_PRIMARY = 1 << iota
> security_windows.go:    TokenUser = 1 + iota
> zerrors_windows.go:     E2BIG Errno = APPLICATION_ERROR + iota
> ztypes_windows.go:      FILE_NOTIFY_CHANGE_FILE_NAME = 1 << iota
> ztypes_windows.go:      FILE_ACTION_ADDED = iota + 1
> ztypes_windows.go:      HKEY_CLASSES_ROOT = 0x80000000 + iota
> ztypes_windows.go:      REG_NONE = iota
>
> I like to leave them as they are. We have comments to warn future users.
>
> But if you insist that it is wrong, I will change them all.
>
> Alex
>
>
https://codereview.appspot.**com/8541047/<https://codereview.appspot.com/8541...
>

[brainman, 2013-05-15 03:40:29]

On 2013/05/15 03:36:09, bradfitz wrote:
> 
> To me (and I think to others?), "iota" generally means "I want some unique
> numbers, but I don't really care what they are".
> 

I disagree. If "iota" means "... I don't really care what they are", why does Go
allows "1 + iota" and "1 << iota" and such?

Alex

[bradfitz, 2013-05-15 03:42:43]

On Tue, May 14, 2013 at 8:40 PM, <alex.brainman@gmail.com> wrote:

> On 2013/05/15 03:36:09, bradfitz wrote:
>
>  To me (and I think to others?), "iota" generally means "I want some
>>
> unique
>
>> numbers, but I don't really care what they are".
>>
>
>
> I disagree. If "iota" means "... I don't really care what they are", why
> does Go allows "1 + iota" and "1 << iota" and such?
>

I disagree but I would prefer this thread end and the code remain unchanged.