Stable UNIX users: storage and auth API #51102
Conversation
espadolini
added
the
no-changelog
espadolini
force-pushed
the
espadolini/stable-unix-user-api
branch
from
0fb9265 to
d0943ca
Compare
espadolini
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
This comment was marked as off-topic.
This comment was marked as off-topic.
espadolini
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
espadolini
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
espadolini
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
espadolini
force-pushed
the
espadolini/stable-unix-user-api
branch
from
39de628 to
b7c7571
Compare
There was a problem hiding this comment.
I think this is still not quite right? If following the existing pattern the package should be lib/auth/stableunixusersv1.
There was a problem hiding this comment.
I don't think I'd ever version the implementation like that unless I really had to - I'm almost sure there's going to be overlap in functionality, etc. - and all you'd need is a newtype to implement both services with "the same" object, which would strongly benefit from being in the same go package.
| // Validate implements [AuthPreference]. | ||
| func (c *AuthPreferenceV2) Validate() error { |
There was a problem hiding this comment.
Should CASD defer to Validate?
There was a problem hiding this comment.
If you mean that CheckAndSetDefaults should call Validate, no - Validate is specifically intended to not get called after UnmarshalAuthPreference.
If you mean the opposite I'm not sure - I like the guarantee that Validate is read-only, and the intended usecase is to validate after getting the AuthPreferenceV2 out of storage which would've already done a CASD.
| ap, err := s.clusterConfiguration.GetAuthPreference(ctx) | ||
| if err != nil { | ||
| return 0, trace.Wrap(err) | ||
| } | ||
| authPref = ap |
There was a problem hiding this comment.
Why doesn't the readonly cache sufice on subsequent attempts?
There was a problem hiding this comment.
The readonly cache has a TTL, and we don't have a way to distinguish the specific condition that failed in Backend.AtomicWrite (dynamodb doesn't return that, iirc) so on our subsequent attempts we should fetch the freshest possible AuthPreference with the freshest possible revision.
| func (s *server) createNewStableUNIXUser(ctx context.Context, username string, authPref readonly.AuthPreference) (int32, error) { | ||
| cfg := authPref.GetStableUNIXUserConfig() | ||
| if cfg == nil || !cfg.Enabled { | ||
| return 0, trace.LimitExceeded("stable UNIX users are not enabled") |
There was a problem hiding this comment.
Is this a limit exceeded because when the feature is disabled the UID range is 0? Would a concrete error be a better marker for this condition?
There was a problem hiding this comment.
I changed the logic to rely on returned boolean flags instead in 02cac25
| @@ -48,6 +48,8 @@ type ClusterConfigurationService struct { | |||
| backend.Backend | |||
| } | |||
|
|
|||
| var _ services.ClusterConfigurationInternal = (*ClusterConfigurationService)(nil) | |||
There was a problem hiding this comment.
The appearance of the blank identifier in this construct indicates that the declaration exists only for the type checking, not to create a variable. Don't do this for every type that satisfies an interface, though. By convention, such declarations are only used when there are no static conversions already present in the code, which is a rare event.
There was a problem hiding this comment.
There's no such conversion in the file or package, AFAICT, and I personally really like that the assertion shows which interface I intended to implement and if it's by pointer or value.
| Backend backend.Backend | ||
| } | ||
|
|
||
| var _ services.StableUNIXUsersInternal = (*StableUNIXUsersService)(nil) |
There was a problem hiding this comment.
The appearance of the blank identifier in this construct indicates that the declaration exists only for the type checking, not to create a variable. Don't do this for every type that satisfies an interface, though. By convention, such declarations are only used when there are no static conversions already present in the code, which is a rare event.
There was a problem hiding this comment.
There's no such conversion in the file or package, AFAICT.
espadolini
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
This RFD implements the storage and storage manipulation in the auth server, and defines and implements the auth API to store and fetch stable UIDs for automatically provisioned UNIX users.
Related RFD: #50414
Part of #50292