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

Add Authz Matchers API proposal #421

Merged
merged 1 commit into from Jul 23, 2021

Conversation

periklis
Copy link
Contributor

@periklis periklis commented Jul 9, 2021

The following design proposal provide an extension for the observatorium api to OPA authorizers. In particular this extension addresses the issue to refine queries to read endpoints with label matchers provided by the OPA authorizer. It is inspired by the work on tenant isolation. Although it goes one step further to handle dynamic label matchers provided by domain specific OPA authorizers in form of PromQL's labels.Matcher (See prometheus/pkg/labels/matcher.go).

@periklis periklis self-assigned this Jul 9, 2021
@periklis periklis force-pushed the authz-matchers-api-proposal branch 7 times, most recently from 915da43 to d98c0c5 Compare July 9, 2021 08:51

## Why

OPA authorizers are the single source of through to check if a given subject (e.g. identified by a bearer token) has access to a requested resource (e.g. read/write on metrics and/or logs). Querying either the in-process via rego or any REST OPA authorizer relays this task from the observatorium-api to a separate isolated component. In addition an OPA authorizer can relay the request further to a third-party system (e.g. RedHat AMS, OpenShift/Kubernetes API server). Upon successful authorization the OPA authorizer can request further information (e.g. list of sub-resources the subject has access) and pass it back to the observatorium api. Such further information can be used to further refine the read endpoints (e.g. return logs for kubernetes namespaces the subject has access to).
Copy link
Member

Choose a reason for hiding this comment

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

Here you are describing the "what" or "how". This section should be about "why we need something like this" independently of the implementation.

@periklis periklis force-pushed the authz-matchers-api-proposal branch from d98c0c5 to cbf0d50 Compare July 9, 2021 10:26
Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

This looks good to me! Just a comment about the cross-tenancy use case.

Comment on lines +51 to +60
{
"result": true|false,
"matchers": [
{
"name": "label key",
"type": "MatchEqual|MatchNotEqual|MatchRegex|MatchNotReqex",
"value": "label value"
}
]
}
Copy link
Member

Choose a reason for hiding this comment

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

For cross-tenant use case, we would need to enforce different matchers depending on the tenant.

For example, Kafka people want to access CPU usage metric from OSD for their clusters, so we need to enforce {customer="kafka"} only for OSD metrics accessed by someone from Kafka. A query like

kafka_price_per_cpu * cpu_usage_total{tenant="osd"}

needs to be converted to

kafka_price_per_cpu{tenant="kafka"} * cpu_usage_total{tenant="osd", customer="kafka"}.

I propose to change this payload to something like

Suggested change
{
"result": true|false,
"matchers": [
{
"name": "label key",
"type": "MatchEqual|MatchNotEqual|MatchRegex|MatchNotReqex",
"value": "label value"
}
]
}
{
"result": true|false,
"matchers": {
"tenant-a": [
{
"name": "label key",
"type": "MatchEqual|MatchNotEqual|MatchRegex|MatchNotReqex",
"value": "label value"
}
],
"tenant-b": []
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Changing this payload right now is not required but we have to do this for cross-tenant queries in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright let's do this in a separate step. This doesn't hurt if we change this later.

@periklis periklis merged commit edcc594 into observatorium:main Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants