feat: implement provisioner auth middleware and proper org params #12330
Conversation
Step to enable org scoped provisioner daemons
coderd/httpmw/organizationparam.go
Outdated
| // If the name is exactly "default", then we fetch the default | ||
| // organization. This is a special case to make it easier | ||
| // for single org deployments. | ||
| // | ||
| // arg == uuid.Nil.String() should be a temporary workaround for | ||
| // legacy provisioners that don't provide an organization ID. | ||
| // This prevents a breaking change. | ||
| // TODO: This change was added March 2024, this should be removed | ||
| // some number of months after that date. | ||
| if arg == codersdk.DefaultOrganization || arg == uuid.Nil.String() { | ||
| organization, dbErr = db.GetDefaultOrganization(ctx) |
There was a problem hiding this comment.
This is the only quirky thing. A nil uuid has to be interpreted as the default org for backwards compatibility.
I've added a default string so we can use that placeholder in single org deployments and not have to use a uuid which is harder to remember.
| } | ||
|
|
||
| // Use the same subject for the userAuthKey | ||
| ctx = context.WithValue(ctx, userAuthKey{}, Authorization{ |
There was a problem hiding this comment.
-
why do we need this for provisioner daemons?
-
more generally, why do we store the
rbac.Subjectunder 2 different context keys (authContextKey{}anduserAuthKey{})? -
what is the difference between an "actor" and a "subject"? Are they interchangeable ideas and we're just inconsistent in how we refer to them in the code?
There was a problem hiding this comment.
These should be merged into 1 context value.
It exists because dbauthz needed a context, and it did not feel right to use the httpmw functions for it. So it created it's own.
We should standardize on the dbauthz context, or place one in like /rbac.
- I was just being consistent with the other auths. If I omit this, then the
HTTPAuthorizerwill not work:. This is used in the provisioner handlers.Line 66 in 0656d80
- Legacy reasons when
dbauthzused to be an experiment, and not always enabled. - Interchangeable. One affects http handlers, one affects dbauthz.
I just made an issue tracking it here: #12363
| serverURL, err := c.URL.Parse(fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons/serve", req.Organization)) | ||
| orgParam := req.Organization.String() | ||
| if req.Organization == uuid.Nil { | ||
| orgParam = DefaultOrganization |
There was a problem hiding this comment.
I don't get why we don't like uuid.Nil meaning the default on the API, but are fine with it on the SDK.
There was a problem hiding this comment.
I went back and forth on this. I think we are the only users of our sdk atm, but if I was to require the OrgID argument in the sdk to be set now, that would be a breaking change for the sdk.
I've been trying to maintain that single org deployments do not have to make any changes, and they continue to work as they do today.
On the sdk side, this is an omission of a field being set.
On the API, the nil uuid could be an explicit value being set, rather than an omission.
| }) | ||
| }, &provisionerd.Options{ | ||
| Logger: logger.Named("provisionerd"), | ||
| Logger: logger.Named(provPSK), |
There was a problem hiding this comment.
Why is the logger changed to the PSK? I'm assuming over-zealous replacement of strings? This is only a test, but yikes if we ever did that in production!
What this does
This is a prerequisite to org scoping provisioner daemons. To do so, the provisioner authentication has to properly support the orgs in the api route.
Implemented similar to our coder workspace agent tokens and workspace proxies. As a future follow up we might want to expand the security around how provisioners authenticate, and that can all be handled in this middleware. Eg assigning a unique token to each provisioner.
Changes
readto all organizations forsubjectProvisionerdRequireAPIKeyOrProvisionerDaemonAuthmiddleware to allow a user auth or provisioner auth in handlers.ExtractOrganizationParamallows settingdefaultas the organization ID. This is easier for single org deployments to not bother messing with org ids.ExtractOrganizationParamhandles theuuid.Nilas thedefaultorg. This is to prevent a breaking change, as prior to this the nil uuid was userdefaultas org id if non specified.Follow up
The follow up PR will add
organizationIDas a column on the provisioner daemons table.