Add device ownership #249
Add device ownership #249
Conversation
nsklikas
force-pushed
the
main
branch
4 times, most recently
from
708c09d to
8a2d87d
Compare
|
Hi @nsklikas! There are some failing tests (https://github.com/ubuntu/authd-oidc-brokers/actions/runs/12256799287/job/34192844158?pr=249) and some linter errors (https://github.com/ubuntu/authd-oidc-brokers/actions/runs/12256799287?pr=249). Please resolve those before marking the PR ready for review. Thanks! |
nsklikas
force-pushed
the
main
branch
2 times, most recently
from
72835a9 to
90573fc
Compare
|
Hello @adombeck, thank you for the very quick response. Sorry for this, all checks are passing now. BTW there are a couple of |
cmd/authd-oidc/daemon/daemon.go
Outdated
| if err := ensureDirWithPerms(brokerConfigDir, 0700, os.Geteuid()); err != nil { | ||
| if err := ensureDirWithPerms(brokerConfigDir, 0755, os.Geteuid()); err != nil { | ||
| close(a.ready) | ||
| return fmt.Errorf("error initializing broker configuration directory %q: %v", brokerConfigDir, err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
issue: if we cannot create a folder with 0700 rights, then we cannot do it with additional priviledges of 0755, so you can remove the second function call. Right now the first err is not actually handled so make sure you add handling code for that
There was a problem hiding this comment.
If this is true, then shouldn't this be changed as well?
There was a problem hiding this comment.
If this is true, then shouldn't this be changed as well?
No. The reason we call ensureDirWithPerms there twice is not that it requires different permissions to create a directory with 0700 than with 0755 (it does not), but that we want to create the data directory with 0700 by default but also support it having permissions 0755 (which is the case when it's the SNAP_DATA directory, because that's created with 0755 by snap). We don't need to support that for the broker.conf.d directory.
cmd/authd-oidc/daemon/daemon.go
Outdated
| @@ -125,6 +125,16 @@ func (a *App) serve(config daemonConfig) error { | |||
| } | |||
| } | |||
|
|
|||
| brokerConfigDir := config.Paths.BrokerConf + ".d" | |||
| // Should these dirs be created by the server or the snap? | |||
There was a problem hiding this comment.
After talking to Adrian I can confirm that permission for this folder must be 0700
If the folder is not present, we create it with those permission
internal/broker/broker.go
Outdated
| // The user is allowed to login if: | ||
| // - ALL users are allowed | ||
| // - the user's name is in the list of allowed_users | ||
| // - the user is the owner of the machine |
There was a problem hiding this comment.
| // - the user is the owner of the machine | |
| // - the user is the owner of the machine and OWNER is in the allowed_users list |
There was a problem hiding this comment.
suggestion: honestly, I would remove these comments before the merging, since they're only helpful in dev and review time
There was a problem hiding this comment.
I disagree, explaining the intend of a function in a comment doesn't only make it easier for reviewers but also for whoever else reads the code in the future.
There was a problem hiding this comment.
I personally don't care much of comments when commit messages explain the rationale well enough (really, I comment-less code is more than fine when git blame can be your friend), but given this is not the case (that implies: please be more elaborated in them), I feel it's better to keep it too.
There was a problem hiding this comment.
note: meta discussion which is not relevant to this PR
I personally don't care much of comments when commit messages explain the rationale well enough (really, I comment-less code is more than fine when git blame can be your friend)
I think commit messages should explain the rationale in more detail than code comments, but IMO they don't replace code comments, because it requires significantly more work to view the commit message which originally introduced a change when the code has been modified many times since.
internal/broker/config.go
Outdated
| OwnerUserKey = "OWNER" | ||
|
|
||
| // ownerAutoregistrationConfigPath is the name of the file that will be auto-generated to register the owner. | ||
| ownerRegistrationConfigPath = "20-owner-autoregistration.conf" |
There was a problem hiding this comment.
| ownerRegistrationConfigPath = "20-owner-autoregistration.conf" | |
| ownerAutoRegistrationConfigPath = "20-owner-autoregistration.conf" |
internal/broker/config.go
Outdated
|
|
||
| // ownerAutoregistrationConfigPath is the name of the file that will be auto-generated to register the owner. | ||
| ownerRegistrationConfigPath = "20-owner-autoregistration.conf" | ||
| ownerRegistrationConfigTemplate = "templates/20-owner-autoregistration.conf.tmpl" |
There was a problem hiding this comment.
| ownerRegistrationConfigTemplate = "templates/20-owner-autoregistration.conf.tmpl" | |
| ownerAutoRegistrationConfigTemplate = "templates/20-owner-autoregistration.conf.tmpl" |
internal/broker/config.go
Outdated
| @@ -53,6 +94,38 @@ func getDropInFiles(cfgPath string) ([]any, error) { | |||
| return dropInFiles, nil | |||
| } | |||
|
|
|||
| func parseUsersSection(cfg *userConfig, users *ini.Section) { | |||
There was a problem hiding this comment.
I would expect a parse* function to return a config object and an error, instead of applying side effects. Since you use it to enrich an existing object I strongly suggest a different name, like for example
| func parseUsersSection(cfg *userConfig, users *ini.Section) { | |
| func populateUsersConfig(cfg *userConfig, users *ini.Section) { |
internal/broker/config.go
Outdated
| r, ok := uc.allowedUsers[user] | ||
| if !ok { | ||
| return false | ||
| } | ||
| return r |
There was a problem hiding this comment.
question: why returning r when you can just return ok without the if?
This would allow for explicitly blocking users, but the spec doesn't have that feature
internal/broker/config.go
Outdated
| return err | ||
| } | ||
|
|
||
| f, err := os.Create(p) |
There was a problem hiding this comment.
issue: this creates a file with default 0666 permission.
After talking to Adrian I can confirm files inside the broker.d folder need to have 0600
internal/broker/broker.go
Outdated
| allowed = true | ||
| } else if b.cfg.userConfig.OwnerUserAllowed() { | ||
| // If owner is undefined, then the first user to login is considered the owner | ||
| if b.cfg.userConfig.OwnerIsUnset() || *b.cfg.userConfig.Owner() == userName { |
There was a problem hiding this comment.
*b.cfg.userConfig.Owner() shouldn't we check for nil ptr too?
internal/broker/broker.go
Outdated
| if b.cfg.userConfig.AllUsersAllowed() { | ||
| allowed = true | ||
| } else if b.cfg.userConfig.IsUserAllowed(userName) { | ||
| allowed = true | ||
| } else if b.cfg.userConfig.OwnerUserAllowed() { | ||
| // If owner is undefined, then the first user to login is considered the owner | ||
| if b.cfg.userConfig.OwnerIsUnset() || *b.cfg.userConfig.Owner() == userName { | ||
| allowed = true | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel this code can be put in a single function where you follow the happy-path better with early returns.
internal/broker/broker.go
Outdated
| // user to login as the owner. | ||
| if b.cfg.userConfig.OwnerUserAllowed() && b.cfg.userConfig.OwnerIsUnset() { | ||
| if b.cfg.ConfigFile != "" { | ||
| // TODO(nsklikas): What should we do in case of error? |
There was a problem hiding this comment.
Opening a discussion here, since this should be handled.
There was a problem hiding this comment.
After our sync today, it was decided that authn should fail in this case. I will update it.
internal/broker/broker_test.go
Outdated
| func pointerTo[T ~string](s T) *T { | ||
| return &s | ||
| } |
There was a problem hiding this comment.
In authd itself we use this instead:
func ptrValue[T any](value T) *T {
return &value
}So for sake of consistency between our projects, please reuse the same.
nsklikas
force-pushed
the
main
branch
2 times, most recently
from
769e6ff to
84b43a6
Compare
|
I tried to apply all the changes proposed, I did the following manual tests:
I went with @adombeck's suggestion, but I am not adding the owner in the allowedUsers list because I think that in the future we may want to differentiate between regular users and the owner. Also tried to add more info the commit messages (cc @3v1n0) |
internal/broker/config.go
Outdated
| slog.Error(fmt.Sprintf("Failed to open autoregistration template: %v", err)) | ||
| return err |
There was a problem hiding this comment.
I feel it would be better to just:
| slog.Error(fmt.Sprintf("Failed to open autoregistration template: %v", err)) | |
| return err | |
| return fmt.Errorf("failed to open autoregistration template: %w", err) |
Then in the caller function to log in case of error.
internal/broker/config.go
Outdated
|
|
||
| f, err := os.OpenFile(p, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600) | ||
| if err != nil { | ||
| slog.Error(fmt.Sprintf("Failed to create owner registration file: %v", err)) |
There was a problem hiding this comment.
Ditto, as before, wrap the error then log in the caller.
internal/broker/config_test.go
Outdated
| err := os.WriteFile(confPath, []byte(configTypes["valid"]), 0600) | ||
| require.NoError(t, err, "Setup: Failed to write config file") | ||
|
|
||
| dropInDir := confPath + ".d" |
There was a problem hiding this comment.
So I just want to offer my logic behind this, in tests IMO in tests it's better not to use helper functions or constants (e.g. a couple of lines we use "20-owner-autoregistration.conf" to generate the path of the file, instead of using the const ownerAutoRegistrationConfigPath). The reason for this is to act as a safe guard for typos in the code and to make changing this behavior harder. Also this function is used in the function that we are trying to test, so from a higher level I don't like this.
Of course, this does not always make sense and there are cases where it makes changes to the code harder (but I think that behavior like this is unlikely to change, so this is not the case).
Anyway, this is a nit-pick I would say, I don't think there is any real difference between the 2 approaches and I understand the reason, so I will update it.
internal/broker/config_test.go
Outdated
| err := os.WriteFile(confPath, []byte(testParseUserConfigTypes[name]), 0600) | ||
| require.NoError(t, err, "Setup: Failed to write config file") | ||
|
|
||
| dropInDir := confPath + ".d" |
There was a problem hiding this comment.
Same here, can reuse GetDropInDir?
internal/broker/helper_test.go
Outdated
| if cfg.allowedUsers == nil { | ||
| cfg.SetAllowedUsers(map[string]struct{}{"OWNER": {}}) | ||
| } else { | ||
| cfg.SetAllowedUsers(cfg.allowedUsers) |
There was a problem hiding this comment.
| if cfg.allowedUsers == nil { | |
| cfg.SetAllowedUsers(map[string]struct{}{"OWNER": {}}) | |
| } else { | |
| cfg.SetAllowedUsers(cfg.allowedUsers) | |
| cfg.SetAllowedUsers(cfg.allowedUsers) | |
| if cfg.allowedUsers == nil { | |
| cfg.SetAllowedUsers(cfg.SetAllowedUsers(map[string]struct{}{"OWNER": {}})) |
There was a problem hiding this comment.
Can likely ignore as per https://github.com/ubuntu/authd-oidc-brokers/pull/249/files#r1886878243
| p := msentraid.New() | ||
|
|
||
| ret := p.NormalizeUsername(tc.username) | ||
|
|
||
| require.Equal(t, tc.normalizedUsername, ret) |
There was a problem hiding this comment.
nit:
| p := msentraid.New() | |
| ret := p.NormalizeUsername(tc.username) | |
| require.Equal(t, tc.normalizedUsername, ret) | |
| p := msentraid.New() | |
| normalized := p.NormalizeUsername(tc.username) | |
| require.Equal(t, tc.normalizedUsername, normalized) |
There was a problem hiding this comment.
The newlines were there to separate the 3 steps of the test (setup, execute, assert). I will update it.
| tests := map[string]struct { | ||
| username string | ||
|
|
||
| normalizedUsername string |
There was a problem hiding this comment.
| normalizedUsername string | |
| wantNormalized string |
If OWNER is in the `allowed_users` list and no owner is registered, a configuration file with the first user to login is generated.
Add the logic for filtering users based on the allowed_users list. A user is allowed to login if - ALL users are allowed - the user's name is in the list of allowed_users - OWNER is in the allowed_users and the user is the owner of the machine
nsklikas
force-pushed
the
main
branch
2 times, most recently
from
6d98975 to
1fef256
Compare
Each provider should implement a NormalizeUsername method that will be used when comparing usernames.
|
I tried to apply all the changes requested:
|
|
@3v1n0 are you happy as well? |
| b.setOwnerMutex.Lock() | ||
| defer b.setOwnerMutex.Unlock() |
There was a problem hiding this comment.
Wasn't this better as part of the configuration?
There was a problem hiding this comment.
I mean the mutex being part of userConfig and so moving the ops dealing with it there?
There was a problem hiding this comment.
I considered that, but I decided to keep @adombeck's suggestion because i wanted to keep the config object as light as possible.
On one hand it makes sense to keep the mutex as close as possible to the race condition (the writing of the file), so we could move it to the config object. On the other hand, it also fine to say that registerOwner is not thread safe and the caller is responsible for implementing that.
I am fine with either approach (slightly leaning on keeping the current one).
There was a problem hiding this comment.
Ah okay. I don't care if we keep it here or move it to userConfig.
There was a problem hiding this comment.
I meant something more like:
diff --git a/internal/broker/broker.go b/internal/broker/broker.go
index 4cf8198..6426843 100644
--- a/internal/broker/broker.go
+++ b/internal/broker/broker.go
@@ -49,8 +49,6 @@ type Config struct {
type Broker struct {
cfg Config
- setOwnerMutex sync.Mutex
-
provider providers.Provider
oidcCfg oidc.Config
@@ -615,7 +613,7 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, session *session, au
}
}
- err = b.maybeRegisterOwner(b.cfg.ConfigFile, authInfo.UserInfo.Name)
+ b.cfg.registerOwner(b.cfg.ConfigFile, authInfo.UserInfo.Name)
if err != nil {
// The user is not allowed if we fail to create the owner-autoregistration file.
// Otherwise the owner might change if the broker is restarted.
@@ -643,19 +641,6 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, session *session, au
return AuthGranted, userInfoMessage{UserInfo: authInfo.UserInfo}
}
-func (b *Broker) maybeRegisterOwner(cfgPath string, userName string) error {
- // We need to lock here to avoid a race condition where two users log in at the same time, causing both to be
- // considered the owner.
- b.setOwnerMutex.Lock()
- defer b.setOwnerMutex.Unlock()
-
- if !b.cfg.userConfig.shouldRegisterOwner() {
- return nil
- }
-
- return b.cfg.registerOwner(cfgPath, userName)
-}
-
// userNameIsAllowed checks whether the user's username is allowed to access the machine.
func (b *Broker) userNameIsAllowed(userName string) bool {
normalizedUsername := b.provider.NormalizeUsername(userName)
@@ -669,7 +654,8 @@ func (b *Broker) userNameIsAllowed(userName string) bool {
if _, ok := b.cfg.userConfig.allowedUsers[normalizedUsername]; ok {
return true
}
- return b.cfg.userConfig.ownerAllowed && b.cfg.userConfig.owner == normalizedUsername
+
+ return b.cfg.isOwnerAllowed(normalizedUsername)
}
func (b *Broker) startAuthenticate(sessionID string) (context.Context, error) {
diff --git a/internal/broker/config.go b/internal/broker/config.go
index 7070921..c084069 100644
--- a/internal/broker/config.go
+++ b/internal/broker/config.go
@@ -69,11 +69,10 @@ type userConfig struct {
ownerAllowed bool
firstUserBecomesOwner bool
owner string
+ ownerMutex *sync.RWMutex
homeBaseDir string
allowedSSHSuffixes []string
- setOwnerMutex *sync.Mutex
-
provider provider
}
@@ -144,12 +143,14 @@ func populateUsersConfig(cfg *userConfig, users *ini.Section) {
// We need to read the owner key after we call HasKey, because the key is created
// when we call the "Key" function and we can't distinguish between empty and unset.
+ cfg.ownerMutex.Lock()
+ defer cfg.ownerMutex.Unlock()
cfg.owner = cfg.provider.NormalizeUsername(users.Key(ownerKey).String())
}
// parseConfigFile parses the config file and returns a map with the configuration keys and values.
func parseConfigFile(cfgPath string, p provider) (userConfig, error) {
- cfg := userConfig{provider: p, setOwnerMutex: &sync.Mutex{}}
+ cfg := userConfig{provider: p, ownerMutex: &sync.RWMutex{}}
dropInFiles, err := getDropInFiles(cfgPath)
if err != nil {
@@ -189,11 +190,22 @@ func (uc *userConfig) shouldRegisterOwner() bool {
return uc.ownerAllowed && uc.firstUserBecomesOwner && uc.owner == ""
}
+func (uc *userConfig) isOwnerAllowed(userName string) bool {
+ uc.ownerMutex.RLock()
+ defer uc.ownerMutex.RUnlock()
+
+ return uc.ownerAllowed && uc.owner == userName
+}
+
func (uc *userConfig) registerOwner(cfgPath, userName string) error {
// We need to lock here to avoid a race condition where two users log in at the same time, causing both to be
// considered the owner.
- uc.setOwnerMutex.Lock()
- defer uc.setOwnerMutex.Unlock()
+ uc.ownerMutex.Lock()
+ defer uc.ownerMutex.Unlock()
+
+ if !uc.shouldRegisterOwner() {
+ return nil
+ }
if cfgPath == "" {
uc.owner = uc.provider.NormalizeUsername(userName)
diff --git a/internal/broker/config_test.go b/internal/broker/config_test.go
index 90f33b1..f20e9d5 100644
--- a/internal/broker/config_test.go
+++ b/internal/broker/config_test.go
@@ -59,7 +59,7 @@ issuer = https://higher-precedence-issuer.url.com
func TestParseConfig(t *testing.T) {
t.Parallel()
p := &testutils.MockProvider{}
- ignoredFields := map[string]struct{}{"provider": {}, "setOwnerMutex": {}}
+ ignoredFields := map[string]struct{}{"provider": {}, "ownerMutex": {}}
tests := map[string]struct {
configType string
@@ -319,7 +319,7 @@ func TestRegisterOwner(t *testing.T) {
err = os.Mkdir(dropInDir, 0700)
require.NoError(t, err, "Setup: Failed to create drop-in directory")
- cfg := userConfig{firstUserBecomesOwner: true, ownerAllowed: true, provider: p, setOwnerMutex: &sync.Mutex{}}
+ cfg := userConfig{firstUserBecomesOwner: true, ownerAllowed: true, provider: p, ownerMutex: &sync.RWMutex{}}
err = cfg.registerOwner(confPath, userName)
require.NoError(t, err)
diff --git a/internal/broker/export_test.go b/internal/broker/export_test.go
index 1723c93..769c093 100644
--- a/internal/broker/export_test.go
+++ b/internal/broker/export_test.go
@@ -9,7 +9,7 @@ import (
)
func (cfg *Config) Init() {
- cfg.setOwnerMutex = &sync.Mutex{}
+ cfg.ownerMutex = &sync.RWMutex{}
}
func (cfg *Config) SetClientID(clientID string) {
@@ -29,10 +29,14 @@ func (cfg *Config) SetAllowedUsers(allowedUsers map[string]struct{}) {
}
func (cfg *Config) SetOwner(owner string) {
+ cfg.ownerMutex.Lock()
+ defer cfg.ownerMutex.Unlock()
cfg.owner = owner
}
func (cfg *Config) SetFirstUserBecomesOwner(firstUserBecomesOwner bool) {
+ cfg.ownerMutex.Lock()
+ defer cfg.ownerMutex.Unlock()
cfg.firstUserBecomesOwner = firstUserBecomesOwner
}
Now the usage of the RWMutex vs Mutex is on whether you want to b able to read an unset value while writing on it.
| } | ||
| // If owner is undefined, then the first user to log in is considered the owner | ||
| return b.cfg.userConfig.firstUserBecomesOwner || b.cfg.userConfig.owner == normalizedUsername | ||
| return b.cfg.userConfig.ownerAllowed && b.cfg.userConfig.owner == normalizedUsername |
There was a problem hiding this comment.
Shouldn't all the owner accesses now being protected too?
At least with a RWMutex instead?
There was a problem hiding this comment.
No, this part of the code will be reached only after the first user logs in (owner is registered) so we don't need to lock it (unless I'm missing something).
owner will be written only once and that is protected by the mutex. All other accesses are reads and they will happen after the owner is written.
IAM-1248
Implement device ownership logic as described in
UD084. This PR:Some things that I am not sure about are:
.dconfiguration dir or should that be created by the snap? If the server should create it, are the permissions correct?20-owner-autoregistration.confbe? (right now they are0666)