From 7baa71d12c86671b296a09f7ef4c0aaa4aeae7be Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Fri, 1 Aug 2025 19:42:18 +0100 Subject: [PATCH] Clean up some utils (#57423) - Remove some unused code - Move some code that is only used once to the calling package - Use testify assertions for checking the presence of files/directories --- integration/hostuser_test.go | 10 +- lib/client/sso/redirector.go | 27 +++- lib/client/sso/redirector_internal_test.go | 49 +++++++ lib/config/configuration.go | 10 +- lib/events/filesessions/filestream_test.go | 5 +- lib/release/release.go | 4 +- lib/service/service.go | 14 +- lib/utils/fs_test.go | 9 +- lib/utils/slices/slices.go | 17 +++ lib/utils/utils.go | 145 ++------------------- lib/utils/utils_test.go | 74 ----------- tool/tbot/db.go | 5 +- tool/tsh/common/db_exec_test.go | 2 +- 13 files changed, 133 insertions(+), 238 deletions(-) create mode 100644 lib/client/sso/redirector_internal_test.go diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go index 396c9588c4d..2eec82a9369 100644 --- a/integration/hostuser_test.go +++ b/integration/hostuser_test.go @@ -173,9 +173,9 @@ func TestRootHostUsersBackend(t *testing.T) { require.NoError(t, sudoersbk.WriteSudoersFile("user.name", validSudoersEntry)) _, err = os.Stat(filepath.Join(sudoersTestDir, "teleport-hostuuid-user_name")) require.NoError(t, err) + require.NoError(t, sudoersbk.RemoveSudoersFile("user.name")) - _, err = os.Stat(filepath.Join(sudoersTestDir, "teleport-hostuuid-user_name")) - require.True(t, os.IsNotExist(err)) + require.NoFileExists(t, filepath.Join(sudoersTestDir, "teleport-hostuuid-user_name")) }) t.Run("Test CreateHomeDirectory does not follow symlinks", func(t *testing.T) { @@ -344,16 +344,14 @@ func TestRootHostUsers(t *testing.T) { // delete the user and ensure the sudoers file got deleted require.NoError(t, closer.Close()) require.NoError(t, sudoers.RemoveSudoers(testuser)) - _, err = os.Stat(sudoersPath(testuser, uuid)) - require.True(t, os.IsNotExist(err)) + require.NoFileExists(t, sudoersPath(testuser, uuid)) // ensure invalid sudoers entries dont get written err = sudoers.WriteSudoers(testuser, []string{"badsudoers entry!!!"}, ) require.Error(t, err) - _, err = os.Stat(sudoersPath(testuser, uuid)) - require.True(t, os.IsNotExist(err)) + require.NoFileExists(t, sudoersPath(testuser, uuid)) }) t.Run("test delete all users in teleport service group", func(t *testing.T) { diff --git a/lib/client/sso/redirector.go b/lib/client/sso/redirector.go index 94f54e05098..40669f725df 100644 --- a/lib/client/sso/redirector.go +++ b/lib/client/sso/redirector.go @@ -305,7 +305,32 @@ func (rd *Redirector) clickableURL(redirectURL, postForm string) string { } }) - return utils.ClickableURL(rd.baseURL() + shortPath) + return clickableURL(rd.baseURL() + shortPath) +} + +// clickableURL fixes the address in a URL to make sure +// it's clickable, e.g. it replaces "undefined" addresses like +// 0.0.0.0 used in network listeners with the loopback address. +func clickableURL(in string) string { + out, err := url.Parse(in) + if err != nil { + return in + } + host, port, err := net.SplitHostPort(out.Host) + if err != nil { + return in + } + ip := net.ParseIP(host) + // If address is not an IP address, return it unchanged. + if ip == nil && out.Host != "" { + return out.String() + } + // if address is unspecified, e.g. all interfaces 0.0.0.0 or multicast, + // replace with localhost that is clickable + if len(ip) == 0 || ip.IsUnspecified() || ip.IsMulticast() { + out.Host = fmt.Sprintf("127.0.0.1:%v", port) + } + return out.String() } func (rd *Redirector) baseURL() string { diff --git a/lib/client/sso/redirector_internal_test.go b/lib/client/sso/redirector_internal_test.go new file mode 100644 index 00000000000..56d782b2d2e --- /dev/null +++ b/lib/client/sso/redirector_internal_test.go @@ -0,0 +1,49 @@ +/* + * Teleport + * Copyright (C) 2025 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package sso + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestClickableURL tests clickable URL conversions +func TestClickableURL(t *testing.T) { + t.Parallel() + + testCases := []struct { + info string + in string + out string + }{ + {info: "original URL is OK", in: "http://127.0.0.1:3000/hello", out: "http://127.0.0.1:3000/hello"}, + {info: "unspecified IPV6", in: "http://[::]:5050/howdy", out: "http://127.0.0.1:5050/howdy"}, + {info: "unspecified IPV4", in: "http://0.0.0.0:5050/howdy", out: "http://127.0.0.1:5050/howdy"}, + {info: "specified IPV4", in: "http://192.168.1.1:5050/howdy", out: "http://192.168.1.1:5050/howdy"}, + {info: "specified IPV6", in: "http://[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:5050/howdy", out: "http://[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:5050/howdy"}, + {info: "hostname", in: "http://example.com:3000/howdy", out: "http://example.com:3000/howdy"}, + } + for _, testCase := range testCases { + t.Run(testCase.info, func(t *testing.T) { + out := clickableURL(testCase.in) + require.Equal(t, testCase.out, out) + }) + } +} diff --git a/lib/config/configuration.go b/lib/config/configuration.go index 3991c93fb9a..d7f980724c0 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -69,6 +69,7 @@ import ( "github.com/gravitational/teleport/lib/utils" awsregion "github.com/gravitational/teleport/lib/utils/aws/region" logutils "github.com/gravitational/teleport/lib/utils/log" + libslices "github.com/gravitational/teleport/lib/utils/slices" ) // CommandLineFlags stores command line flag values, it's a much simplified subset @@ -2550,19 +2551,20 @@ func Configure(clf *CommandLineFlags, cfg *servicecfg.Config, legacyAppFlags boo // 140-2 compliant. if clf.FIPS { // Make sure all cryptographic primitives are FIPS compliant. - err = utils.UintSliceSubset(defaults.FIPSCipherSuites, cfg.CipherSuites) + // + err = libslices.ContainsAll(defaults.FIPSCipherSuites, cfg.CipherSuites) if err != nil { return trace.BadParameter("non-FIPS compliant TLS cipher suite selected: %v", err) } - err = utils.StringSliceSubset(defaults.FIPSCiphers, cfg.Ciphers) + err = libslices.ContainsAll(defaults.FIPSCiphers, cfg.Ciphers) if err != nil { return trace.BadParameter("non-FIPS compliant SSH cipher selected: %v", err) } - err = utils.StringSliceSubset(defaults.FIPSKEXAlgorithms, cfg.KEXAlgorithms) + err = libslices.ContainsAll(defaults.FIPSKEXAlgorithms, cfg.KEXAlgorithms) if err != nil { return trace.BadParameter("non-FIPS compliant SSH kex algorithm selected: %v", err) } - err = utils.StringSliceSubset(defaults.FIPSMACAlgorithms, cfg.MACAlgorithms) + err = libslices.ContainsAll(defaults.FIPSMACAlgorithms, cfg.MACAlgorithms) if err != nil { return trace.BadParameter("non-FIPS compliant SSH mac algorithm selected: %v", err) } diff --git a/lib/events/filesessions/filestream_test.go b/lib/events/filesessions/filestream_test.go index 386dd56b116..0994e80a9e7 100644 --- a/lib/events/filesessions/filestream_test.go +++ b/lib/events/filesessions/filestream_test.go @@ -165,10 +165,7 @@ func TestCompleteUpload(t *testing.T) { require.NoError(t, err) require.Equal(t, test.expectedContent, contents) - // Part files directory should no longer exists. - _, err = os.ReadDir(handler.uploadRootPath(*upload)) - require.Error(t, err) - require.True(t, os.IsNotExist(err)) + require.NoDirExists(t, handler.uploadRootPath(*upload)) }) } } diff --git a/lib/release/release.go b/lib/release/release.go index 4a442daf70b..ac08ef7f327 100644 --- a/lib/release/release.go +++ b/lib/release/release.go @@ -27,11 +27,11 @@ import ( "net/http" "os" + "github.com/dustin/go-humanize" "github.com/gravitational/roundtrip" "github.com/gravitational/trace" "github.com/gravitational/teleport/api/types" - "github.com/gravitational/teleport/lib/utils" ) // ClientConfig contains configuration for the release client @@ -124,7 +124,7 @@ func (c *Client) ListReleases(ctx context.Context) ([]*types.Release, error) { OS: a.OS, SHA256: a.SHA256, AssetSize: a.Size, - DisplaySize: utils.ByteCount(a.Size), + DisplaySize: humanize.Bytes(uint64(a.Size)), ReleaseIDs: a.ReleaseIDs, PublicURL: a.PublicURL, }) diff --git a/lib/service/service.go b/lib/service/service.go index 1ba52de94de..d8a08003520 100644 --- a/lib/service/service.go +++ b/lib/service/service.go @@ -1823,6 +1823,18 @@ func (process *TeleportProcess) exportUpgradeWindows(ctx context.Context, req pr return clt.ExportUpgradeWindows(ctx, req) } +// isGroupMember returns whether currently logged user is a member of a group +func isGroupMember(gid int) (bool, error) { + groups, err := os.Getgroups() + if err != nil { + return false, trace.ConvertSystemError(err) + } + if slices.Contains(groups, gid) { + return true, nil + } + return false, nil +} + // adminCreds returns admin UID and GID settings based on the OS func adminCreds() (*int, *int, error) { if runtime.GOOS != constants.LinuxOS { @@ -1830,7 +1842,7 @@ func adminCreds() (*int, *int, error) { } // if the user member of adm linux group, // make audit log folder readable by admins - isAdmin, err := utils.IsGroupMember(teleport.LinuxAdminGID) + isAdmin, err := isGroupMember(teleport.LinuxAdminGID) if err != nil { return nil, nil, trace.Wrap(err) } diff --git a/lib/utils/fs_test.go b/lib/utils/fs_test.go index c2dde216e2f..3045d785f44 100644 --- a/lib/utils/fs_test.go +++ b/lib/utils/fs_test.go @@ -408,8 +408,7 @@ func TestRemoveAllSecure(t *testing.T) { require.NoError(t, RemoveAllSecure("")) require.NoError(t, RemoveAllSecure(tempDir)) - _, err = os.Stat(tempDir) - require.True(t, os.IsNotExist(err), "Directory should be removed: %v", err) + require.NoDirExists(t, tempDir) } func TestRemoveSecure(t *testing.T) { @@ -419,8 +418,7 @@ func TestRemoveSecure(t *testing.T) { require.NoError(t, f.Close()) require.NoError(t, RemoveSecure(f.Name())) - _, err = os.Stat(tempFile) - require.True(t, os.IsNotExist(err), "File should be removed: %v", err) + require.NoFileExists(t, tempFile) } func TestRemoveSecure_symlink(t *testing.T) { @@ -428,6 +426,5 @@ func TestRemoveSecure_symlink(t *testing.T) { require.NoError(t, os.Symlink("/tmp", symlink)) require.NoError(t, RemoveSecure(symlink)) - _, err := os.Stat(symlink) - require.True(t, os.IsNotExist(err), "Symlink should be removed: %v", err) + require.NoFileExists(t, symlink) } diff --git a/lib/utils/slices/slices.go b/lib/utils/slices/slices.go index 5e524b7f1ed..7b99d1a14ad 100644 --- a/lib/utils/slices/slices.go +++ b/lib/utils/slices/slices.go @@ -18,6 +18,12 @@ package slices +import ( + "slices" + + "github.com/gravitational/trace" +) + // FilterMapUnique applies a function to all elements of a slice and collects them. // The function returns the value to collect and whether the current element should be included. // Returned values are deduplicated. @@ -54,6 +60,17 @@ func ToPointers[T any](in []T) []*T { return out } +// ContainsAll checks whether haystack contains all needles, reporting +// error indicating the first unmatched value. +func ContainsAll[S ~[]E, E comparable](haystack S, needles S) error { + for _, needle := range needles { + if !slices.Contains(haystack, needle) { + return trace.BadParameter("%v not in set", needle) + } + } + return nil +} + // FromPointers converts a slice of pointers to values to a slice of values. // Nil pointers are converted to zero-values. func FromPointers[T any](in []*T) []T { diff --git a/lib/utils/utils.go b/lib/utils/utils.go index 2a5143927bd..a48e5d93399 100644 --- a/lib/utils/utils.go +++ b/lib/utils/utils.go @@ -21,13 +21,11 @@ package utils import ( "context" "errors" - "fmt" "io" "io/fs" "log/slog" "math/rand/v2" "net" - "net/url" "os" "path/filepath" "runtime" @@ -146,52 +144,6 @@ func ThisFunction() string { return runtime.FuncForPC(pc[0]).Name() } -// SyncString is a string value -// that can be concurrently accessed -type SyncString struct { - sync.Mutex - string -} - -// Value returns value of the string -func (s *SyncString) Value() string { - s.Lock() - defer s.Unlock() - return s.string -} - -// Set sets the value of the string -func (s *SyncString) Set(v string) { - s.Lock() - defer s.Unlock() - s.string = v -} - -// ClickableURL fixes address in url to make sure -// it's clickable, e.g. it replaces "undefined" address like -// 0.0.0.0 used in network listeners format with loopback 127.0.0.1 -func ClickableURL(in string) string { - out, err := url.Parse(in) - if err != nil { - return in - } - host, port, err := net.SplitHostPort(out.Host) - if err != nil { - return in - } - ip := net.ParseIP(host) - // If address is not an IP address, return it unchanged. - if ip == nil && out.Host != "" { - return out.String() - } - // if address is unspecified, e.g. all interfaces 0.0.0.0 or multicast, - // replace with localhost that is clickable - if len(ip) == 0 || ip.IsUnspecified() || ip.IsMulticast() { - out.Host = fmt.Sprintf("127.0.0.1:%v", port) - } - return out.String() -} - // AsBool converts string to bool, in case of the value is empty // or unknown, defaults to false func AsBool(v string) bool { @@ -258,19 +210,8 @@ func StringsSet(in []string) map[string]struct{} { return out } -// IsGroupMember returns whether currently logged user is a member of a group -func IsGroupMember(gid int) (bool, error) { - groups, err := os.Getgroups() - if err != nil { - return false, trace.ConvertSystemError(err) - } - if slices.Contains(groups, gid) { - return true, nil - } - return false, nil -} - -// DNSName extracts DNS name from host:port string. +// DNSName extracts DNS name from host:port string, +// returning an error if the hostname is an IP address. func DNSName(hostport string) (string, error) { host, err := Host(hostport) if err != nil { @@ -485,58 +426,17 @@ func GetFreeTCPPorts(n int, offset ...int) (PortList, error) { return PortList{ports: list}, nil } -// StringSliceSubset returns true if b is a subset of a. -func StringSliceSubset(a []string, b []string) error { - aset := make(map[string]bool) - for _, v := range a { - aset[v] = true - } - - for _, v := range b { - _, ok := aset[v] - if !ok { - return trace.BadParameter("%v not in set", v) - } - - } - return nil -} - -// UintSliceSubset returns true if b is a subset of a. -func UintSliceSubset(a []uint16, b []uint16) error { - aset := make(map[uint16]bool) - for _, v := range a { - aset[v] = true - } - - for _, v := range b { - _, ok := aset[v] - if !ok { - return trace.BadParameter("%v not in set", v) - } - - } - return nil -} - // RemoveFromSlice makes a copy of the slice and removes the passed in values from the copy. func RemoveFromSlice(slice []string, values ...string) []string { - output := make([]string, 0, len(slice)) - - remove := make(map[string]bool) + remove := make(map[string]struct{}) for _, value := range values { - remove[value] = true + remove[value] = struct{}{} } - for _, s := range slice { + return slices.DeleteFunc(slice, func(s string) bool { _, ok := remove[s] - if ok { - continue - } - output = append(output, s) - } - - return output + return ok + }) } // ChooseRandomString returns a random string from the given slice. @@ -577,10 +477,7 @@ func AddrsFromStrings(s apiutils.Strings, defaultPort int) ([]NetAddr, error) { // FileExists checks whether a file exists at a given path func FileExists(fp string) bool { _, err := os.Stat(fp) - if err != nil && os.IsNotExist(err) { - return false - } - return true + return !errors.Is(err, fs.ErrNotExist) } // StoreErrorOf stores the error returned by f within *err. @@ -618,32 +515,6 @@ func ReadAtMost(r io.Reader, limit int64) ([]byte, error) { return data, err } -// HasPrefixAny determines if any of the string values have the given prefix. -func HasPrefixAny(prefix string, values []string) bool { - for _, val := range values { - if strings.HasPrefix(val, prefix) { - return true - } - } - - return false -} - -// ByteCount converts a size in bytes to a human-readable string. -func ByteCount(b int64) string { - const unit = 1000 - if b < unit { - return fmt.Sprintf("%d B", b) - } - div, exp := int64(unit), 0 - for n := b / unit; n >= unit; n /= unit { - div *= unit - exp++ - } - return fmt.Sprintf("%.1f %cB", - float64(b)/float64(div), "kMGTPE"[exp]) -} - // ErrLimitReached means that the read limit is reached. // // TODO(gavin): this should be converted to a 413 StatusRequestEntityTooLarge diff --git a/lib/utils/utils_test.go b/lib/utils/utils_test.go index 346c9eb792d..53739986742 100644 --- a/lib/utils/utils_test.go +++ b/lib/utils/utils_test.go @@ -152,30 +152,6 @@ func TestMaxVersions(t *testing.T) { } } -// TestClickableURL tests clickable URL conversions -func TestClickableURL(t *testing.T) { - t.Parallel() - - testCases := []struct { - info string - in string - out string - }{ - {info: "original URL is OK", in: "http://127.0.0.1:3000/hello", out: "http://127.0.0.1:3000/hello"}, - {info: "unspecified IPV6", in: "http://[::]:5050/howdy", out: "http://127.0.0.1:5050/howdy"}, - {info: "unspecified IPV4", in: "http://0.0.0.0:5050/howdy", out: "http://127.0.0.1:5050/howdy"}, - {info: "specified IPV4", in: "http://192.168.1.1:5050/howdy", out: "http://192.168.1.1:5050/howdy"}, - {info: "specified IPV6", in: "http://[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:5050/howdy", out: "http://[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:5050/howdy"}, - {info: "hostname", in: "http://example.com:3000/howdy", out: "http://example.com:3000/howdy"}, - } - for _, testCase := range testCases { - t.Run(testCase.info, func(t *testing.T) { - out := ClickableURL(testCase.in) - require.Equal(t, testCase.out, out) - }) - } -} - // TestParseAdvertiseAddr tests parsing of advertise address func TestParseAdvertiseAddr(t *testing.T) { t.Parallel() @@ -580,53 +556,3 @@ func TestReadAtMost(t *testing.T) { }) } } - -func TestByteCount(t *testing.T) { - tt := []struct { - name string - size int64 - expected string - }{ - { - name: "1 byte", - size: 1, - expected: "1 B", - }, - { - name: "2 byte2", - size: 2, - expected: "2 B", - }, - { - name: "1kb", - size: 1000, - expected: "1.0 kB", - }, - { - name: "1mb", - size: 1000_000, - expected: "1.0 MB", - }, - { - name: "1gb", - size: 1000_000_000, - expected: "1.0 GB", - }, - { - name: "1tb", - size: 1000_000_000_000, - expected: "1.0 TB", - }, - { - name: "1.6 kb", - size: 1600, - expected: "1.6 kB", - }, - } - - for _, tc := range tt { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expected, ByteCount(tc.size)) - }) - } -} diff --git a/tool/tbot/db.go b/tool/tbot/db.go index 59d67c14856..88e7a37282f 100644 --- a/tool/tbot/db.go +++ b/tool/tbot/db.go @@ -20,13 +20,14 @@ package main import ( "path/filepath" + "slices" + "strings" "github.com/gravitational/trace" "github.com/gravitational/teleport/lib/tbot/cli" "github.com/gravitational/teleport/lib/tbot/config" "github.com/gravitational/teleport/lib/tbot/tshwrap" - "github.com/gravitational/teleport/lib/utils" ) func onDBCommand(globalCfg *cli.GlobalArgs, dbCmd *cli.DBCommand) error { @@ -60,7 +61,7 @@ func onDBCommand(globalCfg *cli.GlobalArgs, dbCmd *cli.DBCommand) error { if dbCmd.Cluster != "" { // If we caught --cluster in our args, pass it through. args = append(args, "--cluster="+dbCmd.Cluster) - } else if !utils.HasPrefixAny("--cluster", *dbCmd.RemainingArgs) { + } else if !slices.ContainsFunc(*dbCmd.RemainingArgs, func(s string) bool { return strings.HasPrefix(s, "--cluster") }) { // If no `--cluster` was provided after a `--`, pass along the cluster // name in the identity. args = append(args, "--cluster="+identity.RouteToCluster) diff --git a/tool/tsh/common/db_exec_test.go b/tool/tsh/common/db_exec_test.go index 27b9850364f..8e4edaf57b8 100644 --- a/tool/tsh/common/db_exec_test.go +++ b/tool/tsh/common/db_exec_test.go @@ -250,7 +250,7 @@ func TestDatabaseExec(t *testing.T) { read, err = utils.ReadPath(filepath.Join(dir, "test-output", "mysql.output")) require.NoError(t, err) require.Equal(t, "db-query executed", strings.TrimSpace(string(read))) - require.True(t, utils.FileExists(filepath.Join(dir, "test-output", "summary.json"))) + require.FileExists(t, filepath.Join(dir, "test-output", "summary.json")) }, }, }