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

Rework users.GetUID #4262

Merged
merged 8 commits into from
Sep 6, 2024
Merged

Rework users.GetUID #4262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b4ae166
Explicit usage of users.GetUID
twz123 Apr 8, 2024
b3064ce
Use testify in GetUID test
twz123 Apr 9, 2024
6320960
More context for errors in GetUID
twz123 Apr 9, 2024
2ce5800
Use id program from PATH
twz123 Apr 9, 2024
92b8316
Rename GetUID to LookupUID
twz123 Apr 9, 2024
b39e6c7
Use -1 as UID return value in case of errors
twz123 Apr 10, 2024
038aa8f
Lookup etcd user ID only once
twz123 Apr 10, 2024
64dd416
Introduce users.ErrNotFound
twz123 Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 41 additions & 16 deletions cmd/controller/certificates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"path/filepath"

"github.com/k0sproject/k0s/internal/pkg/file"
"github.com/k0sproject/k0s/internal/pkg/users"
"github.com/k0sproject/k0s/pkg/apis/k0s/v1beta1"
"github.com/k0sproject/k0s/pkg/certificate"
"github.com/k0sproject/k0s/pkg/config"
Expand Down Expand Up @@ -65,6 +66,14 @@ func (c *Certificates) Init(ctx context.Context) error {
c.CACert = string(cert)
// Changing the URL here also requires changes in the "k0s kubeconfig admin" subcommand.
kubeConfigAPIUrl := fmt.Sprintf("https://localhost:%d", c.ClusterSpec.API.Port)

apiServerUID, err := users.LookupUID(constant.ApiserverUser)
if err != nil {
err = fmt.Errorf("failed to lookup UID for %q: %w", constant.ApiserverUser, err)
apiServerUID = users.RootUID
logrus.WithError(err).Warn("Files with key material for kube-apiserver user will be owned by root")
}

eg.Go(func() error {
// Front proxy CA
if err := c.CertManager.EnsureCA("front-proxy-ca", "kubernetes-front-proxy-ca"); err != nil {
Expand All @@ -80,7 +89,7 @@ func (c *Certificates) Init(ctx context.Context) error {
CACert: proxyCertPath,
CAKey: proxyCertKey,
}
_, err := c.CertManager.EnsureCertificate(proxyClientReq, constant.ApiserverUser)
_, err := c.CertManager.EnsureCertificate(proxyClientReq, apiServerUID)

return err
})
Expand All @@ -94,16 +103,16 @@ func (c *Certificates) Init(ctx context.Context) error {
CACert: caCertPath,
CAKey: caCertKey,
}
adminCert, err := c.CertManager.EnsureCertificate(adminReq, "root")
adminCert, err := c.CertManager.EnsureCertificate(adminReq, users.RootUID)
if err != nil {
return err
}

if err := kubeConfig(c.K0sVars.AdminKubeConfigPath, kubeConfigAPIUrl, c.CACert, adminCert.Cert, adminCert.Key, "root"); err != nil {
if err := kubeConfig(c.K0sVars.AdminKubeConfigPath, kubeConfigAPIUrl, c.CACert, adminCert.Cert, adminCert.Key, users.RootUID); err != nil {
return err
}

return c.CertManager.CreateKeyPair("sa", c.K0sVars, constant.ApiserverUser)
return c.CertManager.CreateKeyPair("sa", c.K0sVars, apiServerUID)
})

eg.Go(func() error {
Expand All @@ -115,11 +124,20 @@ func (c *Certificates) Init(ctx context.Context) error {
CACert: caCertPath,
CAKey: caCertKey,
}
konnectivityCert, err := c.CertManager.EnsureCertificate(konnectivityReq, constant.KonnectivityServerUser)

uid, err := users.LookupUID(constant.KonnectivityServerUser)
if err != nil {
err = fmt.Errorf("failed to lookup UID for %q: %w", constant.KonnectivityServerUser, err)
uid = users.RootUID
logrus.WithError(err).Warn("Files with key material for konnectivity-server user will be owned by root")
}

konnectivityCert, err := c.CertManager.EnsureCertificate(konnectivityReq, uid)
if err != nil {
return err
}
return kubeConfig(c.K0sVars.KonnectivityKubeConfigPath, kubeConfigAPIUrl, c.CACert, konnectivityCert.Cert, konnectivityCert.Key, constant.KonnectivityServerUser)

return kubeConfig(c.K0sVars.KonnectivityKubeConfigPath, kubeConfigAPIUrl, c.CACert, konnectivityCert.Cert, konnectivityCert.Key, uid)
})

eg.Go(func() error {
Expand All @@ -130,12 +148,12 @@ func (c *Certificates) Init(ctx context.Context) error {
CACert: caCertPath,
CAKey: caCertKey,
}
ccmCert, err := c.CertManager.EnsureCertificate(ccmReq, constant.ApiserverUser)
ccmCert, err := c.CertManager.EnsureCertificate(ccmReq, apiServerUID)
if err != nil {
return err
}

return kubeConfig(filepath.Join(c.K0sVars.CertRootDir, "ccm.conf"), kubeConfigAPIUrl, c.CACert, ccmCert.Cert, ccmCert.Key, constant.ApiserverUser)
return kubeConfig(filepath.Join(c.K0sVars.CertRootDir, "ccm.conf"), kubeConfigAPIUrl, c.CACert, ccmCert.Cert, ccmCert.Key, apiServerUID)
})

eg.Go(func() error {
Expand All @@ -146,12 +164,20 @@ func (c *Certificates) Init(ctx context.Context) error {
CACert: caCertPath,
CAKey: caCertKey,
}
schedulerCert, err := c.CertManager.EnsureCertificate(schedulerReq, constant.SchedulerUser)

uid, err := users.LookupUID(constant.SchedulerUser)
if err != nil {
err = fmt.Errorf("failed to lookup UID for %q: %w", constant.SchedulerUser, err)
uid = users.RootUID
logrus.WithError(err).Warn("Files with key material for kube-scheduler user will be owned by root")
}

schedulerCert, err := c.CertManager.EnsureCertificate(schedulerReq, uid)
if err != nil {
return err
}

return kubeConfig(filepath.Join(c.K0sVars.CertRootDir, "scheduler.conf"), kubeConfigAPIUrl, c.CACert, schedulerCert.Cert, schedulerCert.Key, constant.SchedulerUser)
return kubeConfig(filepath.Join(c.K0sVars.CertRootDir, "scheduler.conf"), kubeConfigAPIUrl, c.CACert, schedulerCert.Cert, schedulerCert.Key, uid)
})

eg.Go(func() error {
Expand All @@ -162,7 +188,7 @@ func (c *Certificates) Init(ctx context.Context) error {
CACert: caCertPath,
CAKey: caCertKey,
}
_, err := c.CertManager.EnsureCertificate(kubeletClientReq, constant.ApiserverUser)
_, err := c.CertManager.EnsureCertificate(kubeletClientReq, apiServerUID)
return err
})

Expand Down Expand Up @@ -212,8 +238,7 @@ func (c *Certificates) Init(ctx context.Context) error {
CAKey: caCertKey,
Hostnames: hostnames,
}
_, err = c.CertManager.EnsureCertificate(serverReq, constant.ApiserverUser)

_, err = c.CertManager.EnsureCertificate(serverReq, apiServerUID)
return err
})

Expand All @@ -227,7 +252,7 @@ func (c *Certificates) Init(ctx context.Context) error {
Hostnames: hostnames,
}
// TODO Not sure about the user...
_, err := c.CertManager.EnsureCertificate(apiReq, constant.ApiserverUser)
_, err := c.CertManager.EnsureCertificate(apiReq, apiServerUID)
return err
})

Expand Down Expand Up @@ -262,7 +287,7 @@ func detectLocalIPs(ctx context.Context) ([]string, error) {
return localIPs, nil
}

func kubeConfig(dest, url, caCert, clientCert, clientKey, owner string) error {
func kubeConfig(dest, url, caCert, clientCert, clientKey string, ownerID int) error {
// We always overwrite the kubeconfigs as the certs might be regenerated at startup
const (
clusterName = "local"
Expand Down Expand Up @@ -295,5 +320,5 @@ func kubeConfig(dest, url, caCert, clientCert, clientKey, owner string) error {
return err
}

return file.Chown(dest, owner, constant.CertSecureMode)
return file.Chown(dest, ownerID, constant.CertSecureMode)
}
3 changes: 2 additions & 1 deletion cmd/kubeconfig/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"os"
"path"

"github.com/k0sproject/k0s/internal/pkg/users"
"github.com/k0sproject/k0s/pkg/certificate"
"github.com/k0sproject/k0s/pkg/config"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -104,7 +105,7 @@ Note: A certificate once signed cannot be revoked for a particular user`,
certManager := certificate.Manager{
K0sVars: opts.K0sVars,
}
userCert, err := certManager.EnsureCertificate(userReq, "root")
userCert, err := certManager.EnsureCertificate(userReq, users.RootUID)
if err != nil {
return err
}
Expand Down
3 changes: 2 additions & 1 deletion cmd/kubeconfig/kubeconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"path/filepath"
"testing"

"github.com/k0sproject/k0s/internal/pkg/users"
"github.com/k0sproject/k0s/pkg/apis/k0s/v1beta1"
"github.com/k0sproject/k0s/pkg/certificate"
"github.com/k0sproject/k0s/pkg/config"
Expand Down Expand Up @@ -118,7 +119,7 @@ yJm2KSue0toWmkBFK8WMTjAvmAw3Z/qUhJRKoqCu3k6Mf8DNl6t+Uw==

s.Require().NoError(os.MkdirAll(k0sVars.CertRootDir, 0755))

userCert, err := certManager.EnsureCertificate(userReq, "root")
userCert, err := certManager.EnsureCertificate(userReq, users.RootUID)
s.Require().NoError(err)
clusterAPIURL := cfg.Spec.API.APIAddressURL()

Expand Down
4 changes: 2 additions & 2 deletions docs/external-runtime-deps.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@ are not detected.

#### id

External `/usr/bin/id` will be executed as a fallback if local user lookup
fails, in case NSS is used.
External `id` will be executed as a fallback if local user lookup fails, in case
NSS is used.

## Windows specific
<!--
Expand Down
5 changes: 1 addition & 4 deletions internal/pkg/file/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ import (
"fmt"
"io"
"os"

"github.com/k0sproject/k0s/internal/pkg/users"
)

// Exists checks if a file exists and is not a directory before we
Expand All @@ -36,9 +34,8 @@ func Exists(fileName string) bool {
}

// Chown changes file/dir mode
func Chown(file, owner string, permissions os.FileMode) error {
func Chown(file string, uid int, permissions os.FileMode) error {
// Chown the file properly for the owner
uid, _ := users.GetUID(owner)
err := os.Chown(file, uid, -1)
if err != nil && os.Geteuid() == 0 {
return err
Expand Down
63 changes: 51 additions & 12 deletions internal/pkg/users/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,64 @@ limitations under the License.
package users

import (
"bytes"
"errors"
"fmt"
"os/exec"
"os/user"
"strconv"
"strings"
)

// GetUID returns uid of given username and logs a warning if its missing
func GetUID(name string) (int, error) {
entry, err := user.Lookup(name)
if err == nil {
return strconv.Atoi(entry.Uid)
}
if errors.Is(err, user.UnknownUserError(name)) {
const (
// An unknown (i.e. invalid) user ID. It's distinct from a UID's zero value,
// which happens to be [RootUID]. Assuming root may or may not be a good
// default, depending on the use case. Setting file ownership to root is a
// restrictive and safe default, running programs as root is not. Therefore,
// this is the preferred return value for UIDs in case of error; callers
// must then explicitly decide on the fallback instead of accidentally
// assuming root.
UnknownUID = -1

RootUID = 0 // User ID of the root user
)

var ErrNotExist = errors.New("user does not exist")

// Lookup looks up a user's UID by username. If the user cannot be found, the
// returned error is [ErrNotExist]. If an error is returned, the returned UID
// will be [UnknownUID].
func LookupUID(name string) (int, error) {
var uid string

if entry, err := user.Lookup(name); err != nil {
if !errors.Is(err, user.UnknownUserError(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors.Is(err, user.UnknownUserError(name)) looks weird but seems to work.

Maybe returning a locally defined UnknownUserError = errors.New("unknown user") would make the function more pleasant to use.

https://go.dev/play/p/EsG_Q7Uagbw

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also pondering about this. The original code re-used this standard library error in this way. Changing the returned error would trickle down to callers, which are relying on this as well.

What about having a function for this instead, e.g.

func IsNotExist(err error) bool {
	var UnknownUserError user.UnknownUserError
	return errors.As(err, &UnknownUserError)
}

Callers could then use if users.IsNotExist(err) { ... } ... Which would mimic the design of the standard library's os.Is... functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. The os.Is... function docs now do say new code should use errors.Is(err, fs.ErrNotExist) but they're not going anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Right. I'll have a look at your initial proposal first, then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

return UnknownUID, err
}

err = ErrNotExist

// fallback to call external `id` in case NSS is used
out, err := exec.Command("/usr/bin/id", "-u", name).CombinedOutput()
if err == nil {
return strconv.Atoi(strings.TrimSuffix(string(out), "\n"))
out, idErr := exec.Command("id", "-u", name).Output()
kke marked this conversation as resolved.
Show resolved Hide resolved
if idErr != nil {
var exitErr *exec.ExitError
if errors.As(idErr, &exitErr) {
return UnknownUID, fmt.Errorf("%w (%w: %s)", err, idErr, bytes.TrimSpace(exitErr.Stderr))
}
return UnknownUID, fmt.Errorf("%w (%w)", err, idErr)
}

uid = string(bytes.TrimSpace(out))
} else {
uid = entry.Uid
}
return 0, err

parsedUID, err := strconv.Atoi(uid)
if err != nil {
return UnknownUID, fmt.Errorf("UID %q is not a decimal integer: %w", uid, err)
}
if parsedUID < 0 {
return UnknownUID, fmt.Errorf("UID is negative: %d", parsedUID)
}

return parsedUID, nil
}
21 changes: 12 additions & 9 deletions internal/pkg/users/users_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,28 @@ limitations under the License.
package users

import (
"os/exec"
"runtime"
"testing"

"github.com/stretchr/testify/assert"
)

func TestGetUID(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("No numeric user IDs on Windows")
}

uid, err := GetUID("root")
if err != nil {
t.Errorf("failed to find uid for root: %v", err)
}
if uid != 0 {
t.Errorf("root uid is not 0. It is %d", uid)
uid, err := LookupUID("root")
if assert.NoError(t, err, "Failed to get UID for root user") {
assert.Equal(t, 0, uid, "root's UID is not 0?")
}

uid, err = GetUID("some-non-existing-user")
if err == nil {
t.Errorf("unexpectedly got uid for some-non-existing-user: %d", uid)
uid, err = LookupUID("some-non-existing-user")
if assert.Error(t, err, "Got a UID for some-non-existing-user?") {
assert.ErrorIs(t, err, ErrNotExist)
var exitErr *exec.ExitError
assert.ErrorAs(t, err, &exitErr, "expected external `id` to return an error")
assert.Equal(t, UnknownUID, uid)
}
}
17 changes: 7 additions & 10 deletions pkg/certificate/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (

"github.com/k0sproject/k0s/internal/pkg/file"
"github.com/k0sproject/k0s/internal/pkg/stringslice"
"github.com/k0sproject/k0s/internal/pkg/users"
"github.com/k0sproject/k0s/pkg/config"
"github.com/k0sproject/k0s/pkg/constant"
)
Expand Down Expand Up @@ -100,13 +99,11 @@ func (m *Manager) EnsureCA(name, cn string) error {
}

// EnsureCertificate creates the specified certificate if it does not already exist
func (m *Manager) EnsureCertificate(certReq Request, ownerName string) (Certificate, error) {
func (m *Manager) EnsureCertificate(certReq Request, ownerID int) (Certificate, error) {

keyFile := filepath.Join(m.K0sVars.CertRootDir, fmt.Sprintf("%s.key", certReq.Name))
certFile := filepath.Join(m.K0sVars.CertRootDir, fmt.Sprintf("%s.crt", certReq.Name))

uid, _ := users.GetUID(ownerName)

// if regenerateCert returns true, it means we need to create the certs
if m.regenerateCert(certReq, keyFile, certFile) {
logrus.Debugf("creating certificate %s", certFile)
Expand Down Expand Up @@ -160,11 +157,11 @@ func (m *Manager) EnsureCertificate(certReq Request, ownerName string) (Certific
return Certificate{}, err
}

err = os.Chown(keyFile, uid, -1)
err = os.Chown(keyFile, ownerID, -1)
if err != nil && os.Geteuid() == 0 {
return Certificate{}, err
}
err = os.Chown(certFile, uid, -1)
err = os.Chown(certFile, ownerID, -1)
if err != nil && os.Geteuid() == 0 {
return Certificate{}, err
}
Expand All @@ -173,8 +170,8 @@ func (m *Manager) EnsureCertificate(certReq Request, ownerName string) (Certific
}

// certs exist, let's just verify their permissions
_ = os.Chown(keyFile, uid, -1)
_ = os.Chown(certFile, uid, -1)
_ = os.Chown(keyFile, ownerID, -1)
_ = os.Chown(certFile, ownerID, -1)

cert, err := os.ReadFile(certFile)
if err != nil {
Expand Down Expand Up @@ -230,12 +227,12 @@ func isManagedByK0s(cert *certinfo.Certificate) bool {
return false
}

func (m *Manager) CreateKeyPair(name string, k0sVars *config.CfgVars, owner string) error {
func (m *Manager) CreateKeyPair(name string, k0sVars *config.CfgVars, ownerID int) error {
keyFile := filepath.Join(k0sVars.CertRootDir, fmt.Sprintf("%s.key", name))
pubFile := filepath.Join(k0sVars.CertRootDir, fmt.Sprintf("%s.pub", name))

if file.Exists(keyFile) && file.Exists(pubFile) {
return file.Chown(keyFile, owner, constant.CertSecureMode)
return file.Chown(keyFile, ownerID, constant.CertSecureMode)
}

reader := rand.Reader
Expand Down
Loading
Loading