Metrics certificates are not restricted by project

[markylaing]

This commit ae092f4 changed the permissions of GET /1.0/metrics from containers view on the project, to can_view_metrics on the server.

As a result, metrics certificates can now view metrics for all projects, and restricting them by project does not change this.

Subsequently in the schema change from certificates to identities the restricted field of metric type certificates was ignored, so it is now no longer possible to distinguish between metrics certificates that can view metrics for all instances in all projects, or metrics certificates that are restricted to a subset of projects.

Possible steps to fix:

• Rename the existing IdentityTypeCertificateMetrics to IdentityTypeCertificateMetricsUnrestricted.
• Add a new identity type IdentityTypeCertificateMetricsRestricted.
• Fix the schema update to separate metrics certificates by restricted status.
• Add new entitlements can_view_metrics on project, and can_view_metrics on instance.
• In the metrics handler, filter server metrics by can_view_metrics on server, filter project metrics by can_view_metrics on project, and filter instance metrics by can_view_metrics on instance.
• In the TLS authorizer, handle restricted and unrestricted metrics certificates. Unrestricted metrics certificates allow all when the entitlement is can_view_metrics (regardless of entity type). Restricted certificates allow all instances, filter projects, and disallow server.

[tomponline]

Also related lxc/incus#648

[tomponline]

We should add a test to https://github.com/canonical/lxd-ci/blob/main/tests/cluster to check for certificate functionality after upgrades here.

[roosterfish]

I am wondering if we should introduce a can_view_metrics on instances for the existing /1.0/metrics endpoint.

If a user queries /1.0/metrics?project=abc the access handler has to perform extra steps in order to validate if there is an instance in the project for which the user might have can_view_metrics.

Having another metrics endpoint /1.0/metrics/{instanceName} would fit better into the permission model.
This way if a somebody queries /1.0/metrics we can check can_view_metrics on TypeServer. For a query on /1.0/metrics?project=abc we can check can_view_metrics on TypeProject. And last but not least when querying /1.0/metrics/v1?project=abc we can check can_view_metrics on TypeInstance.

[tomponline]

I am wondering if we should introduce a can_view_metrics on instances for the existing /1.0/metrics endpoint.

If a user queries /1.0/metrics?project=abc the access handler has to perform extra steps in order to validate if there is an instance in the project for which the user might have can_view_metrics.

Having another metrics endpoint /1.0/metrics/{instanceName} would fit better into the permission model. This way if a somebody queries /1.0/metrics we can check can_view_metrics on TypeServer. For a query on /1.0/metrics?project=abc we can check can_view_metrics on TypeProject. And last but not least when querying /1.0/metrics/v1?project=abc we can check can_view_metrics on TypeInstance.

This would be an API breaking change would it not?

[roosterfish]

This would be an API breaking change would it not?

If you query /1.0/metrics with can_view_metrics granted on TypeServer it will return everything.
If you query /1.0/metrics?project=xyz with can_view_metrics granted on TypeProject it will return all the project instances metrics.
Is it safe to say that can_view_metrics on TypeProject supersedes an equal entitlement on TypeInstance?
Just having this would replicate what we have currently. I don't see where this would be a breaking change in the API.

Additionally to allow having a can_view_metrics on TypeInstance level a new endpoint will be added. Right now you cannot explicitly request metrics for a specific instance.

[tomponline]

Additionally to allow having a can_view_metrics on TypeInstance level a new endpoint will be added. Right now you cannot explicitly request metrics for a specific instance.

Lets log an issue for this separately as sounds like a feature/improvement rather than related to this bug fix.