-
Notifications
You must be signed in to change notification settings - Fork 75
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
periklis
merged 1 commit into
observatorium:main
from
periklis:authz-matchers-api-proposal
Jul 23, 2021
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
--- | ||
weight: 1 | ||
toc: true | ||
title: Proposals | ||
lead: "" | ||
lastmod: "2021-04-30T10:40:00+00:00" | ||
images: [] | ||
draft: false | ||
description: Observatorium Proposals | ||
date: "2021-04-30T10:40:00+00:00" | ||
--- | ||
|
||
|
74 changes: 74 additions & 0 deletions
74
docs/proposals/drafts/20210709-opa-authorizer-labels-observatorium-api.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
--- | ||
toc: true | ||
title: 2021-07 Authz Matchers API | ||
--- | ||
|
||
## Implement Authz Matchers API for OPA authorizers | ||
|
||
* **Owners** | ||
* @periklis | ||
|
||
* **Related Tickets** | ||
* [LOG-1513: Provide an opa-openshift authorizer for OpenShift Cluster Logging using Loki](https://issues.redhat.com/browse/LOG-1513) | ||
|
||
* **Other Docs** | ||
* Cross-Tenancy proposal??? | ||
|
||
## TL;DR; | ||
|
||
We propose an API that allows OPA authorizers to return label matchers which get enforced read endpoints by the observatorium api. | ||
|
||
## Why | ||
|
||
The proposed API addresses the issue to use domain-specific authorization knowledge to read queries. This enables a much richer isolation of read queries for the observatroium api. | ||
|
||
## Goals | ||
|
||
* Users can configure OPA authorizers to pass a set of label matchers back to the observatorium api. | ||
* The observatorium api can extend the label sets automatically for queries against the read endpoints for all supported observability signals. | ||
|
||
## Non-Goals | ||
|
||
* Provide a generic label matching engine inside the observatorium api for query result sets. | ||
|
||
## How | ||
|
||
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). | ||
|
||
We propose to extend and further standardize the API between the observatorium-api and OPA authorizers. The current implementation includes only a boolean flag to signal success or failure for the authorization request against an OPA endpoint. This proposal introduces a second optional field to pass label matchers back to the API, e.g.: | ||
|
||
Current implementation payload: | ||
|
||
```json | ||
{ | ||
"result": true|false | ||
} | ||
``` | ||
|
||
Proposed implementation payload: | ||
|
||
```json | ||
{ | ||
"result": true|false, | ||
"matchers": [ | ||
{ | ||
"name": "label key", | ||
"type": "MatchEqual|MatchNotEqual|MatchRegex|MatchNotReqex", | ||
"value": "label value" | ||
} | ||
] | ||
} | ||
``` | ||
|
||
Furthermore the observatorium api will handle an empty or missing `matchers` field as obligatory. Thus a successful authorization request with an empty or missing `matchers` field is accepted as successful authorization. | ||
|
||
## Migration plan | ||
|
||
The present implementations of OPA authorizers (e.g. [opa-ams](https://github.com/observatorium/opa-ams)) do not require any migration. The observatorium-api will handle the response payload following the missing `matchers` approach. Thus these authorizers will not put further constraints on the read endpoints. | ||
|
||
## Action plan | ||
|
||
- [] Iterate and finalize this design document. | ||
- [] Ensure authz matchers API is applicable to other signals (e.g. tracing). | ||
- [] Implement a proof-of-concept Authz Matchers API [observatorium/api#151](https://github.com/observatorium/api/pull/151) for PromQL and LogQL. | ||
- [] Make this API GA. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
--- | ||
weight: 1 | ||
toc: true | ||
title: Drafts | ||
menu: | ||
docs: | ||
parent: proposals | ||
lead: "" | ||
lastmod: "2021-04-30T10:40:00+00:00" | ||
images: [] | ||
draft: false | ||
description: Observatorium Draft Proposals | ||
date: "2021-04-30T10:40:00+00:00" | ||
--- | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 likekafka_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
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.
Changing this payload right now is not required but we have to do this for cross-tenant queries in the future.
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.
Alright let's do this in a separate step. This doesn't hurt if we change this later.