Skip to content
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

Runtime: Apply access policies to alerts returned in ListResources #4464

Merged

Conversation

begelundmuller
Copy link
Contributor

@begelundmuller begelundmuller commented Mar 28, 2024

Makes sure only admins and alert owners/recipients can fetch alert details.

Closes https://github.com/rilldata/rill-private-issues/issues/250


// Extract admin attributes
var email string
admin := true // If no attributes are set, assume it's an admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems odd, usually when no attributes are available, minimum permissions should be assumed.
Why do we need this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The basic reason is that the JWT attributes are only empty if a) it's a service token, or b) it's on localhost. In both cases, admin-like behavior is required.

More high-level, it's an annoying mix of issues:

  • We don't support security policies for alerts/reports, so need to hard-code the access logic for UI-managed alerts/reports directly in the runtime.
  • The runtime JWT claims are not defined for service accounts (and in general, there's not a way to know what kind of claims it is – could be unauthenticated localhost, assumed user on localhost, cloud user, cloud service, or custom attributes).

Hence, the TODO in this PR:

// TODO: This implementation is very specific to properties currently set by the admin server. Consider refactoring to a more generic implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but doesn't it mean a some can send a request without these JWT attributes and get an unfiltered list back ?

Copy link
Contributor Author

@begelundmuller begelundmuller Mar 29, 2024

Choose a reason for hiding this comment

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

The JWTs are signed using the admin service's private key, so it can't be forged.

(And the JWT is verified in the runtime server's middleware. And there are checks in the API resolvers that ensure the JWT has permission to get/list resources, so unauthenticated requests will bounce.)

@nishantmonu51 nishantmonu51 added the blocker A release blocker issue that should be resolved before a new release label Mar 28, 2024
@nishantmonu51 nishantmonu51 merged commit 3854334 into main Apr 1, 2024
6 checks passed
@nishantmonu51 nishantmonu51 deleted the begelundmuller/fix-access-alerts-list-resources branch April 1, 2024 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker A release blocker issue that should be resolved before a new release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants