Add FreeBSD support to libs/go/sia/util #3093
Conversation
Add os_util_freebsd.go to provide FreeBSD-specific OS utility functions for the SIA (Service Identity Agent) library. This change enables AthenZ's SIA library to build and run on FreeBSD systems by providing the platform-specific implementations previously only available for Linux, Darwin, and Windows. The FreeBSD implementation is based on the Linux version, as both systems share similar Unix semantics for: - File ownership management (os.Chown) - User/group ID lookup (id command) - Syslog functionality - Syscall operations (Setuid/Setgid) This was identified while building recent github.com/redpanda-data/connect, which is part of FreeBSD ports. Tested on: FreeBSD 14.2 amd64 Signed-off-by: Giacomo Olgeni <[email protected]>
Summary of ChangesHello @olgeni, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request extends the AthenZ SIA library's platform support to include FreeBSD. By introducing a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for FreeBSD by introducing os_util_freebsd.go, which is a good step towards broader platform compatibility. My review focuses on improving the robustness and security of the new file. I've identified a couple of high-severity issues related to shelling out for user and group lookups, which could lead to security vulnerabilities or denial of service. I've provided suggestions to replace these with safer, pure Go implementations.
Additionally, I've noticed that os_util_freebsd.go is identical to os_util_linux.go and very similar to os_util_darwin.go. To improve long-term maintainability and avoid code duplication, I recommend consolidating these files into a single os_util_unix.go using Go build tags (e.g., //go:build linux,darwin,freebsd). The suggested improvements should ideally be applied across all these platform-specific files.
| func gidForGroup(groupname string) int { | ||
| //shelling out to id is used here because the os/user package | ||
| //requires cgo, which doesn't cross-compile. we can use getent group | ||
| //command but instead we opted for a simple grep for /etc/group | ||
| cmdStr := fmt.Sprintf("^%s:", groupname) | ||
| out, err := exec.Command(GetUtilPath("grep"), cmdStr, "/etc/group").Output() | ||
| if err != nil { | ||
| log.Printf("Cannot exec '%s %s /etc/group': %v\n", GetUtilPath("grep"), groupname, err) | ||
| return -1 | ||
| } | ||
| s := strings.Trim(string(out), "\n\r ") | ||
| comps := strings.Split(string(out), ":") | ||
| if len(comps) < 3 { | ||
| log.Printf("Invalid response from grep group command: %s\n", s) | ||
| return -1 | ||
| } | ||
| //the group id should be the third value: 'group_name:password:group_id:group_list' | ||
| id, err := strconv.Atoi(comps[2]) | ||
| if err != nil { | ||
| log.Printf("Invalid response from getent group command: %s\n", s) | ||
| return -1 | ||
| } | ||
| return id | ||
| } |
There was a problem hiding this comment.
The current implementation of gidForGroup shells out to grep, which is not robust and can be insecure. The groupname is used in a regex without sanitization, which could lead to unexpected behavior if it contains special regex characters. Also, parsing command output is more fragile than reading the file directly.
A better approach would be to read and parse /etc/group in pure Go. This avoids shelling out, is more secure, and removes the dependency on the grep command being available. This change would also be beneficial for the other OS-specific files (os_util_linux.go, os_util_darwin.go) which have similar implementations.
Please also add "bufio" to the import list.
func gidForGroup(groupname string) int {
// Reading /etc/group directly is a cross-compile friendly way to get gid without cgo.
file, err := os.Open("/etc/group")
if err != nil {
log.Printf("Cannot open /etc/group: %v\n", err)
return -1
}
defer file.Close()
scanner := bufio.NewScanner(file)
for scanner.Scan() {
line := scanner.Text()
// format is group_name:password:group_id:user_list
parts := strings.Split(line, ":")
if len(parts) >= 3 && parts[0] == groupname {
gid, err := strconv.Atoi(parts[2])
if err != nil {
log.Printf("Invalid GID '%s' in /etc/group for group %s: %v\n", parts[2], groupname, err)
return -1
}
return gid
}
}
if err := scanner.Err(); err != nil {
log.Printf("Error reading /etc/group: %v\n", err)
return -1
}
log.Printf("Group '%s' not found in /etc/group\n", groupname)
return -1
}| func idCommand(username, arg string) int { | ||
| //shelling out to id is used here because the os/user package | ||
| //requires cgo, which doesn't cross-compile | ||
| var out []byte | ||
| var err error | ||
| if username == "" { | ||
| out, err = exec.Command(GetUtilPath("id"), arg).Output() | ||
| } else { | ||
| out, err = exec.Command(GetUtilPath("id"), arg, username).Output() | ||
| } | ||
| if err != nil { | ||
| log.Fatalf("Cannot exec 'id %s %s': %v\n", arg, username, err) | ||
| } | ||
| s := strings.Trim(string(out), "\n\r ") | ||
| id, err := strconv.Atoi(s) | ||
| if err != nil { | ||
| log.Fatalf("Unexpected UID/GID format in user record: %s\n", string(out)) | ||
| } | ||
| return id | ||
| } |
There was a problem hiding this comment.
The function idCommand uses log.Fatalf on error. This will cause the entire application to terminate if the id command fails (e.g., if an invalid username is provided). This can be a potential denial-of-service vulnerability.
It would be more robust to handle the error gracefully by logging it and returning a sentinel value (like -1), similar to how errors are handled in gidForGroup. This would allow the calling functions to decide how to proceed instead of crashing. This change should also be applied to the other OS-specific files (os_util_linux.go, os_util_darwin.go).
func idCommand(username, arg string) int {
//shelling out to id is used here because the os/user package
//requires cgo, which doesn't cross-compile
var out []byte
var err error
if username == "" {
out, err = exec.Command(GetUtilPath("id"), arg).Output()
} else {
out, err = exec.Command(GetUtilPath("id"), arg, username).Output()
}
if err != nil {
log.Printf("Cannot exec 'id %s %s': %v\n", arg, username, err)
return -1
}
s := strings.Trim(string(out), "\n\r ")
id, err := strconv.Atoi(s)
if err != nil {
log.Printf("Unexpected UID/GID format in user record: %s\n", string(out))
return -1
}
return id
}
2 participants
Description
AthenZ's SIA (Service Identity Agent) library currently only supports Linux, Darwin, and Windows platforms. When building projects that vendor this library (like Redpanda Connect) on FreeBSD, the build fails with:
undefined: setupDirOwnershipThis occurs because Go's build system selects platform-specific files using filename suffixes (
_linux.go,_darwin.go, etc.), and there was no_freebsd.goimplementation.
This PR adds
os_util_freebsd.gobased on the Linux implementation.Testing
github.com/redpanda-data/[email protected]Contribution Checklist: