Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix(coderd): ensure correct RBAC when enqueueing notifications#15478

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
johnstcn merged 9 commits intomainfromcj/fix-autobuild-notifications
Nov 12, 2024

Conversation

johnstcn
Copy link
Member

@johnstcnjohnstcn commentedNov 11, 2024
edited by dannykopping
Loading

While investigating#15477 I noticed the following on my test workspace:

2024-11-11 16:10:01.076 [warn]  autobuild: failed to notify of workspace marked as dormant  workspace_id=3a5b3ab6-c0f6-496f-8efe-25e02a2a9885  workspace_name=apricot-starfish-90  workspace_id=3a5b3ab6-c0f6-496f-8efe-25e02a2a9885 ...    error= new message metadata:               github.com/coder/coder/v2/coderd/notifications.(*StoreEnqueuer).EnqueueWithData                   /home/runner/work/coder/coder/coderd/notifications/enqueuer.go:69             - unauthorized: rbac: forbidden

I expanded the search and found that this was afundamental problem with our test notifier implementation -- namely, that it made absolutely no RBAC assertions.

Logs (loki)

The prospect of creating a completely new test implementation backed by a realdatabase.Store was more than I could stomach right now, so I wired in some minimal RBAC assertions and fixed what tests ended up breaking.

Hoever, this is not the ideal fix. Changes to our RBAC policy on notifications will not be captured here and may result in similar sadness in future.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

review: moved to its own package to avoid import cycle

mtojek and dannykopping reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

review: no changes are needed tolifecycle_executor.go as it has its own actor to which I've added the required permissions

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

review: moved tonotificationstest

Copy link
MemberAuthor

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

self-review: Another option I'm considering is just moving the dbauthz.AsNotifier calls inside the notifications enqueuer. That way, external callers don't need to think about authz when they enqueue a notification. This is both a pro and a con IMO. If folks feel strongly enough about it being the other way, I can make the necessary modifications.

Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Blocking merge since you have an approval already; I think leaving these in was not intentional?

@dannykopping
Copy link
Contributor

self-review: Another option I'm considering is just moving the dbauthz.AsNotifier calls inside the notifications enqueuer. That way, external callers don't need to think about authz when they enqueue a notification. This is both a pro and a con IMO. If folks feel strongly enough about it being the other way, I can make the necessary modifications.

Why do you think it'd be a con? I think this would make sense and express the intent clearly.

@mtojek
Copy link
Member

@dannykopping Danielle made a good point on that inthis thread.

@johnstcn
Copy link
MemberAuthor

self-review: Another option I'm considering is just moving the dbauthz.AsNotifier calls inside the notifications enqueuer. That way, external callers don't need to think about authz when they enqueue a notification. This is both a pro and a con IMO. If folks feel strongly enough about it being the other way, I can make the necessary modifications.

Why do you think it'd be a con? I think this would make sense and express the intent clearly.

I could imagine later-on creating an endpoint/api/v2/notifications/send that does something similar to the below and does not perform any additional RBAC checks:

func (a *api) postNotification(rw http.ResponseWriter, r *http.Request) {  [...]  var req codersdk.NotificationRequest  if err := httpapi.Read(&ctx, rw, r, &req); err != nil { ... }  // Note: passing r.Context() here so that RBAC is enforced based on actor in context  notifID, err := api.NotificationsEnqueuer.Enqueue(r.Context(), req.TemplateID, req.Labels, req.CreatedBy, req.Targets...)  if err != nil { ... }  httpapi.Write(r.Context(), http.StatusCreated, codersdk.Response{...}}}

Ifdbauthz.AsNotifier is simply called within the notifier itself then all users can notify all users, and effectively no actor-level RBAC checking is done. If it's done at the API endpoint level then only users with the correct privileges are able to do so.

Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

Before you all start going deeper into discussions keep in mind that firstly we need to mitigate the issue.

Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

TYVM for addressing this! 🦸‍♂️

@johnstcn
Copy link
MemberAuthor

Local smoke-testing:

  • Runnc -kdl 8888 in a separate terminal
  • Run./scripts/develop.sh -- --notifications-method=webhook --notifications-webhook-endpoint=http://localhost:8888 --notifications-max-send-attempts=1
  • Users:
    • Create a user, suspend a user, resume a user
    • Observe notification message webhook body in netcat output
  • Workspaces:
    • Create a workspace, enable dormancy on template
    • In local database:UPDATE workspaces SET last_used_at = NOW() - INTERVAL '1 hours';
    • Wait for dormancy to kick in
    • Observe notification webhook message body in netcat output
mtojek, dannykopping, and stirby reacted with rocket emoji

@johnstcnjohnstcn merged commit30e6fbd intomainNov 12, 2024
26 checks passed
@johnstcnjohnstcn deleted the cj/fix-autobuild-notifications branchNovember 12, 2024 12:40
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@mtojekmtojekmtojek approved these changes

@dannykoppingdannykoppingdannykopping approved these changes

@mafredrimafredriAwaiting requested review from mafredri

@SasSwartSasSwartAwaiting requested review from SasSwart

@DanielleMaywoodDanielleMaywoodAwaiting requested review from DanielleMaywood

Assignees

@johnstcnjohnstcn

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@johnstcn@dannykopping@mtojek@DanielleMaywood

[8]ページ先頭

©2009-2025 Movatter.jp