-
Notifications
You must be signed in to change notification settings - Fork 67
Description
daemon.go: 310:
// Optimisation: avoid calling userFromRequest, which acquires the state
// lock, in case we don't need to (when endpoint is OpenAccess). This
// avoids holding the state lock for /v1/health in particular, which is
// not good: https://github.com/canonical/pebble/pull/369
var user *UserState
if _, isOpen := access.(OpenAccess); !isOpen {
user, err = userFromRequest(c.d.state, r, ucred)
if err != nil {
Forbidden("forbidden").ServeHTTP(w, r)
return
}
}
// If we don't have a named-identity user, use ucred UID to see if we have a default.
if user == nil && ucred != nil {
if ucred.Uid == 0 || ucred.Uid == uint32(os.Getuid()) {
// Admin if UID is 0 (root) or the UID the daemon is running as.
user = &UserState{Access: state.AdminAccess, UID: &ucred.Uid}
} else {
// Regular read access if any other local UID.
user = &UserState{Access: state.ReadAccess, UID: &ucred.Uid}
}
}Today, the userFromRequest call is made to resolve the current Identity. The identity is captured in a struct UserState.
// UserState represents the state of an authenticated API user.
type UserState struct {
Access state.IdentityAccess
UID *uint32
Username string
}When a Identity is resolved, Username is available. This is used in preference to UID (when no Identity was resolved) in the function:
func userString(u *UserState) string {
if u == nil {
return "<unknown>"
}
if u.Username != "" {
return u.Username
}
if u.UID != nil {
return strconv.Itoa(int(*u.UID))
}
return "<unknown>"
}The thing that bugs me here is that we only resolve Identities if the AccessChecker type is not OpenAccess.
This does not feel nice. It means that:
user, err = userFromRequest(c.d.state, r, ucred)
if err != nil {
Forbidden("forbidden").ServeHTTP(w, r)
return
}Identity lookup, which could previously have resulted in an outright refusal of API access, is being bypassed in this condition.
I understand why this was done, and I agree that right now there is no problem, as ultimately "Open Access" means open access.
However, for example, an UID lookup under identities could result in an Access value which is different from the default fallback. Since we do not do the lookup, that UID will for OpenAccess have a different Access value, compared to other cases. This feels really risky.
How about we implement a simple caching layer for state Identities, so that lookups can be instant without a state global lock ?
I am currently looking at userFromRequest to form part of mTLS client certificate lookup, and having OpenAccess effectively demote mTLS -> TLS, by ignoring the client certificate lookup / validation step, does not feel ideal. Of course I can work around this, but I want to see if my effort in this space can result in something slightly nicer overall ?
As the title suggests, skipping userFromRequest conditionally also means the value that userString can print for a UID with a named identity will different whether that print happens inside an Open or non-open accessed end-point.