-
Notifications
You must be signed in to change notification settings - Fork 904
feat: implement provisioner auth middleware and proper org params #12330
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.
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
Changes from all commits
913600f
df54d89
3e982cb
0cbd149
e543e57
c6fef46
9372502
c855447
321a145
0656d80
03d6f74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
package httpmw | ||
|
||
import ( | ||
"context" | ||
"crypto/subtle" | ||
"net/http" | ||
|
||
"golang.org/x/xerrors" | ||
|
||
"github.com/coder/coder/v2/coderd/database" | ||
"github.com/coder/coder/v2/coderd/database/dbauthz" | ||
"github.com/coder/coder/v2/coderd/httpapi" | ||
"github.com/coder/coder/v2/codersdk" | ||
) | ||
|
||
type provisionerDaemonContextKey struct{} | ||
|
||
func ProvisionerDaemonAuthenticated(r *http.Request) bool { | ||
proxy, ok := r.Context().Value(provisionerDaemonContextKey{}).(bool) | ||
return ok && proxy | ||
} | ||
|
||
type ExtractProvisionerAuthConfig struct { | ||
DB database.Store | ||
Optional bool | ||
} | ||
|
||
func ExtractProvisionerDaemonAuthenticated(opts ExtractProvisionerAuthConfig, psk string) func(next http.Handler) http.Handler { | ||
return func(next http.Handler) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
ctx := r.Context() | ||
|
||
handleOptional := func(code int, response codersdk.Response) { | ||
if opts.Optional { | ||
next.ServeHTTP(w, r) | ||
return | ||
} | ||
httpapi.Write(ctx, w, code, response) | ||
} | ||
|
||
if psk == "" { | ||
// No psk means external provisioner daemons are not allowed. | ||
// So their auth is not valid. | ||
handleOptional(http.StatusBadRequest, codersdk.Response{ | ||
Message: "External provisioner daemons not enabled", | ||
}) | ||
return | ||
} | ||
|
||
token := r.Header.Get(codersdk.ProvisionerDaemonPSK) | ||
if token == "" { | ||
handleOptional(http.StatusUnauthorized, codersdk.Response{ | ||
Message: "provisioner daemon auth token required", | ||
}) | ||
return | ||
} | ||
|
||
if subtle.ConstantTimeCompare([]byte(token), []byte(psk)) != 1 { | ||
handleOptional(http.StatusUnauthorized, codersdk.Response{ | ||
Message: "provisioner daemon auth token invalid", | ||
}) | ||
return | ||
} | ||
|
||
// The PSK does not indicate a specific provisioner daemon. So just | ||
// store a boolean so the caller can check if the request is from an | ||
// authenticated provisioner daemon. | ||
ctx = context.WithValue(ctx, provisionerDaemonContextKey{}, true) | ||
// nolint:gocritic // Authenticating as a provisioner daemon. | ||
ctx = dbauthz.AsProvisionerd(ctx) | ||
subj, ok := dbauthz.ActorFromContext(ctx) | ||
if !ok { | ||
// This should never happen | ||
httpapi.InternalServerError(w, xerrors.New("developer error: ExtractProvisionerDaemonAuth missing rbac actor")) | ||
} | ||
|
||
// Use the same subject for the userAuthKey | ||
ctx = context.WithValue(ctx, userAuthKey{}, Authorization{ | ||
Actor: subj, | ||
ActorName: "provisioner_daemon", | ||
}) | ||
|
||
next.ServeHTTP(w, r.WithContext(ctx)) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,8 +179,8 @@ type ServeProvisionerDaemonRequest struct { | |
ID uuid.UUID `json:"id" format:"uuid"` | ||
// Name is the human-readable unique identifier for the daemon. | ||
Name string `json:"name" example:"my-cool-provisioner-daemon"` | ||
// Organization is the organization for the URL. At present provisioner daemons ARE NOT scoped to organizations | ||
// and so the organization ID is optional. | ||
// Organization is the organization for the URL. If no orgID is provided, | ||
// then it is assumed to use the default organization. | ||
Organization uuid.UUID `json:"organization" format:"uuid"` | ||
// Provisioners is a list of provisioner types hosted by the provisioner daemon | ||
Provisioners []ProvisionerType `json:"provisioners"` | ||
|
@@ -194,7 +194,12 @@ type ServeProvisionerDaemonRequest struct { | |
// implementation. The context is during dial, not during the lifetime of the | ||
// client. Client should be closed after use. | ||
func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisionerDaemonRequest) (proto.DRPCProvisionerDaemonClient, error) { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get why we don't like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
serverURL, err := c.URL.Parse(fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons/serve", orgParam)) | ||
if err != nil { | ||
return nil, xerrors.Errorf("parse url: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this for provisioner daemons?
more generally, why do we store the
rbac.Subject
under 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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be merged into 1 context value.
It exists because
dbauthz
needed a context, and it did not feel right to use thehttpmw
functions for it. So it created it's own.We should standardize on the
dbauthz
context, or place one in like/rbac
.HTTPAuthorizer
will not work:coder/coderd/authorize.go
Line 66 in 0656d80
dbauthz
used to be an experiment, and not always enabled.I just made an issue tracking it here: #12363