Skip to content
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.

Sign up for GitHub

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

Jump to bottom

os/user: add illumos getgroups support #14709

Open
bradfitz opened this issue Mar 8, 2016 · 25 comments
Open

os/user: add illumos getgroups support #14709

bradfitz opened this issue Mar 8, 2016 · 25 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-illumos
Milestone
Unplanned

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Mar 8, 2016

Solaris is broken after the os/user groups change:

http://build.golang.org/log/26a14fbc0126ab1ed65eaeffe18362689b13a54f

...
net
os/user
# os/user
Undefined           first referenced
 symbol                 in file
getgrouplist                        $WORK/os/user/_obj/lookup_unix.cgo2.o
ld: fatal: symbol referencing errors. No output written to $WORK/os/user/_obj/_cgo_.o
collect2: error: ld returned 1 exit status
crypto/x509
net/textproto
log/syslog
...

Let me know if you need help testing on Solaris. It's not yet a trybot, due to lack of time.
@bradfitz bradfitz added this to the Go1.7 milestone Mar 8, 2016
@bradfitz
Copy link
Contributor Author

bradfitz commented Mar 8, 2016

Apparently Solaris didn't have getgrouplist until recently. I found elsewhere that "getgrouplist() is provided by the latest Solaris 11.2 SRU 11.2.10.5.0, released 15 May 2015. It will be available in all future updates/releases." But apparently our builders don't have that.

@4ad, advice?

@bradfitz
Copy link
Contributor Author

bradfitz commented Mar 8, 2016

Actually, we should just not try to build it & test it. I'll send out a CL. We can keep this bug open to add support later.

@bradfitz
Copy link
Contributor Author

bradfitz commented Mar 8, 2016

Or rather, I'll wait for @zombiezen to finish his pending CLs, since my change would necessarily split lookup_unix.go into two halves probably (user lookup vs group lookup, with different build tags), or maybe add a group_solaris.go file with an import "C" and a fake getgrouplist impl in it just for Solaris.

Ross, let me know if you prefer to tackle this.

@4ad
Copy link
Member

4ad commented Mar 8, 2016

We will have to implement getgrouplist for Solaris ourselves. In the meantime we can stub it.

@zombiezen
Copy link
Contributor

I can roll in the stub as part of https://golang.org/cl/20348, since I'm already splitting out the OS-specific functionality.

@bradfitz
Copy link
Contributor Author

bradfitz commented Mar 8, 2016

Great, thanks.

@gopherbot
Copy link

CL https://golang.org/cl/20348 mentions this issue.

@zombiezen
Copy link
Contributor

Just to point out, the other group-related functionality should still work on Solaris (let me know if it doesn't). getgrouplist is annoyingly non-standard. I'm contemplating whether the internal listGroups function should just parse /etc/groups itself in the future because it will ironically be more portable.

@bradfitz
Copy link
Contributor Author

bradfitz commented Mar 8, 2016

Parsing /etc/groups as a fallback seems fine too.

@bradfitz
Copy link
Contributor Author

bradfitz commented Mar 8, 2016

Repurposing this bug to add Solaris support, since this is the bug number references in the t.Skip text now.

@bradfitz bradfitz reopened this Mar 8, 2016
@bradfitz bradfitz changed the title os/user: solaris broken by groups change os/user: add solaris getgroups support Mar 8, 2016
@binarycrusader
Copy link
Contributor

Some versions of Solaris support this function, so ideally, if there is a sanctioned way to see if we can import the symbol from libc, and if it's nil, then fallback to the other functionality, that seems like the best option.

@minux
Copy link
Member

minux commented Mar 9, 2016 via email

@ianlancetaylor
Copy link
Contributor

We are talking about cgo here, and my understanding is that Solaris doesn't support internal linking anyhow. So I think we could use a weak reference in the C code.

@binarycrusader
Copy link
Contributor

The internal linker works just fine on Solaris. Not sure I understand?

@ianlancetaylor
Copy link
Contributor

Sorry, I expect it is me that does not understand.

@binarycrusader
Copy link
Contributor

I guess I was assuming that cgo import dynamic would simply assign nil for symbols it can't find:

//go:cgo_import_dynamic libc_Getgrouplist getgrouplist "libc.so"

My assumption was we could just check for nil in the wrapper function, if not nil, call the function, and if nil, fallback to other logic.

But I don't know if that's possible.

@minux
Copy link
Member

minux commented Mar 10, 2016 via email

@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016
@zombiezen zombiezen removed their assignment Sep 15, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@rsc rsc modified the milestones: Unplanned, Go1.8 Oct 19, 2016
@matejb
Copy link
Contributor

matejb commented Sep 27, 2017

Stumbled on this issue yesterday while working on the application for Illumnos based OS.
And made the in-app version of the function that parses /etc/group.

And I see there are existing private functions for /etc/group parsing inside user package.
I can use these to make User.GroupIds work for Solaris.
If that would be acceptable?

@4ad
Copy link
Member

4ad commented Sep 27, 2017

No, we need to do what @minux said in his last reply.

@andy-js
Copy link
Contributor

andy-js commented Aug 6, 2020

I'm not sure why nobody suggested implementing this using getgrent. There's no actual need to have getgrouplist.

@andy-js
Copy link
Contributor

andy-js commented Aug 10, 2020

See: #40667

@gopherbot
Copy link

Change https://golang.org/cl/315278 mentions this issue: os/user: implement (*User).GroupIds on solaris

@tklauser tklauser changed the title os/user: add solaris getgroups support os/user: add illumos getgroups support May 2, 2021
gopherbot pushed a commit that referenced this issue May 2, 2021
@tklauser
os/user: implement (*User).GroupIds on solaris
abb110b 
It seems like getgrouplist is supported since Solaris 11.3 (released in
2016):
https://docs.oracle.com/cd/E86824_01/html/E54766/getgrouplist-3c.html

Use it to implement (*User).GroupIds on solaris, like on other Unix
platforms.

Unfortunately it looks like getgrouplist was added to illumos only
fairly recently, see
illumos/illumos-gate@f2c438c

Thus, don't use it on GOOS=illumos for now.

Updates #14709

Change-Id: Ibfcdbfca6b7d1af96630512d08921e5637ca76d4
Reviewed-on: https://go-review.googlesource.com/c/go/+/315278
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@tklauser
Copy link
Member

tklauser commented May 2, 2021

https://golang.org/cl/315278 added support for solaris, repurposing this issue to add support on illumos.

@kolyshkin
Copy link
Contributor

https://go-review.googlesource.com/c/go/+/330753 implements /etc/passwd parsing, and unconditionally enables that for illumos. As far as I understand this lands in Go 1.18.

Ideally, what's needed (on top of the above, for Illumos) is a dynamic check if getgrouplist libc function is available, and using it.

@gopherbot
Copy link

Change https://golang.org/cl/330753 mentions this issue: os/user: implement go native GroupIds

gopherbot pushed a commit that referenced this issue Sep 21, 2021
@kolyshkin @ianlancetaylor
os/user: implement go native GroupIds
30faf96 
Currently, GroupIds (a method that returns supplementary group IDs
for a user) is not implemented when cgo is not available, or osusergo
build tag is set, or the underlying OS lacks getgrouplist(3).

This adds a native Go implementation of GroupIds (which parses
/etc/group) for such cases, together with some tests.

This implementation is used:
 - when cgo is not available;
 - when osusergo build tag is set;
 - on AIX (which lacks getgrouplist(3));
 - on Illumos (which only recently added getgrouplist(3)).

This commit moves listgroups_unix.go to cgo_listgroups_unix.go, and adds
listgroups_unix.go which implements the feature.

NOTE the +build equivalent of go:build expression in listgroups_unix.go
is not provided as it is going to be bulky. Go 1.17 already prefers
go:build over +build, and no longer fail if a file contains go:build
without +build, so the absence of +build is not a problem even with Go
1.17, and this code is targeted for Go 1.18.

Updates #14709
Updates #30563

Change-Id: Icc95cda97ee3bcb03ef028b16eab7d3faba9ffab
Reviewed-on: https://go-review.googlesource.com/c/go/+/330753
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-illumos
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests