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

Metrics: Differentiate between restricted and unrestricted certificates #13214

Merged
merged 15 commits into from
Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 22 additions & 15 deletions lxd/api_metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,15 @@ func allowMetrics(d *Daemon, r *http.Request) response.Response {
return response.EmptySyncResponse
}

return allowPermission(entity.TypeServer, auth.EntitlementCanViewMetrics)(d, r)
entityType := entity.TypeServer

// Check for individual permissions on project if set.
projectName := request.QueryParam(r, "project")
if projectName != "" {
entityType = entity.TypeProject
}

return allowPermission(entityType, auth.EntitlementCanViewMetrics)(d, r)
}

// swagger:operation GET /1.0/metrics metrics metrics_get
Expand Down Expand Up @@ -323,31 +331,30 @@ func metricsGet(d *Daemon, r *http.Request) response.Response {
}

func getFilteredMetrics(s *state.State, r *http.Request, compress bool, metricSet *metrics.MetricSet) response.Response {
// Ignore filtering in case the authentication for metrics is disabled.
if !s.GlobalConfig.MetricsAuthentication() {
return response.SyncResponsePlain(true, compress, metricSet.String())
}

// Get instances the user is allowed to view.
userHasPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeInstance)
if err != nil && !auth.IsDeniedError(err) {
userHasProjectPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanViewMetrics, entity.TypeProject)
if err != nil {
return response.SmartError(err)
} else if err != nil {
// This is counterintuitive. We are unauthorized to get a permission checker for viewing instances because a metric type certificate
// can't view instances. However, in order to get to this point we must already have auth.EntitlementCanViewMetrics. So we can view
// the metrics but we can't do any filtering, so just return the metrics.
return response.SyncResponsePlain(true, compress, metricSet.String())
}

// Filter by project name and instance name.
metricSet.FilterSamples(userHasPermission)

userHasPermission, err = s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanView, entity.TypeProject)
userHasServerPermission, err := s.Authorizer.GetPermissionChecker(r.Context(), r, auth.EntitlementCanViewMetrics, entity.TypeServer)
tomponline marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return response.SmartError(err)
}

// Filter by project only.
metricSet.FilterSamples(userHasPermission)
// Filter all metrics for view permissions.
// Internal server metrics without labels are compared against the server entity.
metricSet.FilterSamples(func(labels map[string]string) bool {
if labels["project"] != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct? What if the label does have a non-empty project, but the user doesn't have a project permission, but does have server permission, this would return false?

Doesn't having the server permission mean they can see all metrics (including the project ones)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay tricky one, thanks for rising this as I have found another potential security bug in my changes. The unrestricted metrics certificate got just too much permission.

But the way the checking is done here is right.
In case of the TLS driver if the certificate is unrestricted, the driver permits can_view_metrics by default (see latest push where I went back to the original changes).
In case of OIDC this is allowed as the can_view_metrics on the server type supersedes the can_view_metrics on the project type. This way the check still returns true even if there is no explicit can_view_metrics entitlement set for the project (see the changed OpenFGA model).

Copy link
Member

Choose a reason for hiding this comment

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

OK so its resolved internally in each driver that the user has implicit project level permissions if its assigned at the server level. Makes sense thanks

return userHasProjectPermission(entity.ProjectURL(labels["project"]))
}

return userHasServerPermission(entity.ServerURL())
})

return response.SyncResponsePlain(true, compress, metricSet.String())
}
Expand Down
2 changes: 1 addition & 1 deletion lxd/auth/authorization_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ const (
// EntitlementCanViewResources is the `can_view_resources` Entitlement. It applies to entity.TypeServer.
EntitlementCanViewResources Entitlement = "can_view_resources"

// EntitlementCanViewMetrics is the `can_view_metrics` Entitlement. It applies to entity.TypeServer.
// EntitlementCanViewMetrics is the `can_view_metrics` Entitlement. It applies to entity.TypeServer and entity.TypeProject.
tomponline marked this conversation as resolved.
Show resolved Hide resolved
EntitlementCanViewMetrics Entitlement = "can_view_metrics"

// EntitlementCanViewWarnings is the `can_view_warnings` Entitlement. It applies to entity.TypeServer.
Expand Down
1 change: 1 addition & 0 deletions lxd/auth/driver_openfga_model.openfga
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ type project
define can_delete_storage_buckets: [identity, service_account, group#member] or operator or storage_bucket_manager or can_edit_projects from server
define can_view_operations: [identity, service_account, group#member] or operator or viewer or can_view_projects from server
define can_view_events: [identity, service_account, group#member] or operator or viewer or can_view_projects from server
define can_view_metrics: [identity, service_account, group#member] or operator or viewer or can_view_metrics from server
type image
relations
define project: [project]
Expand Down
7 changes: 5 additions & 2 deletions lxd/auth/driver_tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func (t *tls) CheckPermission(ctx context.Context, r *http.Request, entityURL *a

if !isRestricted {
return nil
} else if id.IdentityType == api.IdentityTypeCertificateMetrics && entitlement == EntitlementCanViewMetrics {
} else if id.IdentityType == api.IdentityTypeCertificateMetricsUnrestricted && entitlement == EntitlementCanViewMetrics {
return nil
}

Expand Down Expand Up @@ -166,7 +166,7 @@ func (t *tls) GetPermissionChecker(ctx context.Context, r *http.Request, entitle

if !isRestricted {
return allowFunc(true), nil
} else if id.IdentityType == api.IdentityTypeCertificateMetrics && entitlement == EntitlementCanViewMetrics {
} else if id.IdentityType == api.IdentityTypeCertificateMetricsUnrestricted && entitlement == EntitlementCanViewMetrics {
return allowFunc(true), nil
}

Expand All @@ -178,6 +178,9 @@ func (t *tls) GetPermissionChecker(ctx context.Context, r *http.Request, entitle
// Check server level object types
switch entityType {
case entity.TypeServer:
// We have to keep EntitlementCanViewMetrics here for backwards compatibility with older versions of LXD.
// Historically when viewing the metrics endpoint for a specific project with a restricted certificate
// also the internal server metrics get returned.
if entitlement == EntitlementCanView || entitlement == EntitlementCanViewResources || entitlement == EntitlementCanViewMetrics {
return allowFunc(true), nil
}
Expand Down
1 change: 1 addition & 0 deletions lxd/auth/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ func EntitlementsByEntityType(entityType entity.Type) ([]Entitlement, error) {
EntitlementCanDeleteStorageBuckets,
EntitlementCanViewOperations,
EntitlementCanViewEvents,
EntitlementCanViewMetrics,
}, nil
case entity.TypeServer:
return []Entitlement{
Expand Down
2 changes: 1 addition & 1 deletion lxd/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ func (d *Daemon) Authenticate(w http.ResponseWriter, r *http.Request) (trusted b
// Validate metrics certificates.
if r.URL.Path == "/1.0/metrics" {
for _, i := range r.TLS.PeerCertificates {
trusted, username := util.CheckTrustState(*i, d.identityCache.X509Certificates(api.IdentityTypeCertificateMetrics), d.endpoints.NetworkCert(), trustCACertificates)
trusted, username := util.CheckTrustState(*i, d.identityCache.X509Certificates(api.IdentityTypeCertificateMetricsRestricted, api.IdentityTypeCertificateMetricsUnrestricted), d.endpoints.NetworkCert(), trustCACertificates)
if trusted {
return true, username, api.AuthenticationMethodTLS, nil, nil
}
Expand Down
17 changes: 14 additions & 3 deletions lxd/db/cluster/certificates.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,11 @@ func (cert *Certificate) ToIdentityType() (IdentityType, error) {
case certificate.TypeServer:
return api.IdentityTypeCertificateServer, nil
case certificate.TypeMetrics:
return api.IdentityTypeCertificateMetrics, nil
if cert.Restricted {
return api.IdentityTypeCertificateMetricsRestricted, nil
}

return api.IdentityTypeCertificateMetricsUnrestricted, nil
}

return "", fmt.Errorf("Unknown certificate type `%d`", cert.Type)
Expand Down Expand Up @@ -178,7 +182,7 @@ func GetCertificates(ctx context.Context, tx *sql.Tx, filters ...CertificateFilt
case certificate.TypeServer:
identityTypes = append(identityTypes, api.IdentityTypeCertificateServer)
case certificate.TypeMetrics:
identityTypes = append(identityTypes, api.IdentityTypeCertificateMetrics)
identityTypes = append(identityTypes, api.IdentityTypeCertificateMetricsRestricted, api.IdentityTypeCertificateMetricsUnrestricted)
}
}

Expand Down Expand Up @@ -278,9 +282,16 @@ func DeleteCertificates(ctx context.Context, tx *sql.Tx, name string, certificat
return DeleteIdentitys(ctx, tx, name, api.IdentityTypeCertificateClientUnrestricted)
} else if certificateType == certificate.TypeServer {
return DeleteIdentitys(ctx, tx, name, api.IdentityTypeCertificateServer)
} else if certificateType == certificate.TypeMetrics {
err := DeleteIdentitys(ctx, tx, name, api.IdentityTypeCertificateMetricsRestricted)
if err != nil {
return err
}

return DeleteIdentitys(ctx, tx, name, api.IdentityTypeCertificateMetricsUnrestricted)
}

return DeleteIdentitys(ctx, tx, name, api.IdentityTypeCertificateMetrics)
return nil
}

// UpdateCertificate updates the certificate matching the given key parameters.
Expand Down
34 changes: 24 additions & 10 deletions lxd/db/cluster/identities.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,12 @@ func (a AuthMethod) Value() (driver.Value, error) {
type IdentityType string

const (
identityTypeCertificateClientRestricted int64 = 1
identityTypeCertificateClientUnrestricted int64 = 2
identityTypeCertificateServer int64 = 3
identityTypeCertificateMetrics int64 = 4
identityTypeOIDCClient int64 = 5
identityTypeCertificateClientRestricted int64 = 1
identityTypeCertificateClientUnrestricted int64 = 2
identityTypeCertificateServer int64 = 3
identityTypeCertificateMetricsRestricted int64 = 4
identityTypeOIDCClient int64 = 5
identityTypeCertificateMetricsUnrestricted int64 = 6
)

// Scan implements sql.Scanner for IdentityType. This converts the integer value back into the correct API constant or
Expand All @@ -142,8 +143,10 @@ func (i *IdentityType) Scan(value any) error {
*i = api.IdentityTypeCertificateClientUnrestricted
case identityTypeCertificateServer:
*i = api.IdentityTypeCertificateServer
case identityTypeCertificateMetrics:
*i = api.IdentityTypeCertificateMetrics
case identityTypeCertificateMetricsRestricted:
*i = api.IdentityTypeCertificateMetricsRestricted
case identityTypeCertificateMetricsUnrestricted:
*i = api.IdentityTypeCertificateMetricsUnrestricted
case identityTypeOIDCClient:
*i = api.IdentityTypeOIDCClient
default:
Expand All @@ -162,8 +165,10 @@ func (i IdentityType) Value() (driver.Value, error) {
return identityTypeCertificateClientUnrestricted, nil
case api.IdentityTypeCertificateServer:
return identityTypeCertificateServer, nil
case api.IdentityTypeCertificateMetrics:
return identityTypeCertificateMetrics, nil
case api.IdentityTypeCertificateMetricsRestricted:
return identityTypeCertificateMetricsRestricted, nil
case api.IdentityTypeCertificateMetricsUnrestricted:
return identityTypeCertificateMetricsUnrestricted, nil
case api.IdentityTypeOIDCClient:
return identityTypeOIDCClient, nil
}
Expand All @@ -180,7 +185,9 @@ func (i IdentityType) toCertificateType() (certificate.Type, error) {
return certificate.TypeClient, nil
case api.IdentityTypeCertificateServer:
return certificate.TypeServer, nil
case api.IdentityTypeCertificateMetrics:
case api.IdentityTypeCertificateMetricsRestricted:
return certificate.TypeMetrics, nil
case api.IdentityTypeCertificateMetricsUnrestricted:
return certificate.TypeMetrics, nil
}

Expand Down Expand Up @@ -244,6 +251,13 @@ func (i Identity) ToCertificate() (*Certificate, error) {
return nil, fmt.Errorf("Failed to check restricted status of identity: %w", err)
}

// Metrics certificates can be both restricted and unrestricted.
// But an unrestricted metrics certificate has still less permissions as an unrestricted client certificate.
// So it does not have full access to LXD only the metrics endpoint.
if i.Type == api.IdentityTypeCertificateMetricsUnrestricted {
isRestricted = false
}

c := &Certificate{
ID: i.ID,
Fingerprint: i.Identifier,
Expand Down
3 changes: 2 additions & 1 deletion lxd/db/cluster/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,8 @@ CREATE TABLE identities_projects (
INSERT INTO identities (id, auth_method, type, identifier, name, metadata) SELECT id, 1, 1, fingerprint, name, json_object('cert', certificate) FROM certificates WHERE type = 1 AND restricted = 1;
INSERT INTO identities (id, auth_method, type, identifier, name, metadata) SELECT id, 1, 2, fingerprint, name, json_object('cert', certificate) FROM certificates WHERE type = 1 AND restricted = 0;
INSERT INTO identities (id, auth_method, type, identifier, name, metadata) SELECT id, 1, 3, fingerprint, name, json_object('cert', certificate) FROM certificates WHERE type = 2;
INSERT INTO identities (id, auth_method, type, identifier, name, metadata) SELECT id, 1, 4, fingerprint, name, json_object('cert', certificate) FROM certificates WHERE type = 3;
INSERT INTO identities (id, auth_method, type, identifier, name, metadata) SELECT id, 1, 4, fingerprint, name, json_object('cert', certificate) FROM certificates WHERE type = 3 AND restricted = 1;
INSERT INTO identities (id, auth_method, type, identifier, name, metadata) SELECT id, 1, 6, fingerprint, name, json_object('cert', certificate) FROM certificates WHERE type = 3 AND restricted = 0;
INSERT INTO identities_projects (identity_id, project_id) SELECT certificate_id, project_id FROM certificates_projects;

DROP TABLE certificates;
Expand Down
14 changes: 11 additions & 3 deletions lxd/db/cluster/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -777,14 +777,15 @@ func TestUpdateFromV69(t *testing.T) {
INSERT INTO certificates (fingerprint, type, name, certificate, restricted) VALUES ('eeef45f0570ce713864c86ec60c8d88f60b4844d3a8849b262c77cb18e88394d', 1, 'restricted-client', ?, 1);
INSERT INTO certificates (fingerprint, type, name, certificate, restricted) VALUES ('86ec60c8d88f60b4844d3a8849b262c77cb18e88394deeef45f0570ce713864c', 1, 'unrestricted-client', ?, 0);
INSERT INTO certificates (fingerprint, type, name, certificate, restricted) VALUES ('49b262c77cb18e88394d8e6ec60c8d8eef45f0570ce713864c8f60b4844d3a88', 2, 'server', ?, 0);
INSERT INTO certificates (fingerprint, type, name, certificate, restricted) VALUES ('60c8d8eef45f0570ce713864c8f60b4844d3a8849b262c77cb18e88394d8e6ec', 3, 'metrics', ?, 0);
INSERT INTO certificates (fingerprint, type, name, certificate, restricted) VALUES ('60c8d8eef45f0570ce713864c8f60b4844d3a8849b262c77cb18e88394d8e6ec', 3, 'metrics', ?, 1);
INSERT INTO certificates (fingerprint, type, name, certificate, restricted) VALUES ('47c88da8fd0cb9a8d44768a445e6c27aee44e078ce74cbaec0726de427bac056', 3, 'metrics', ?, 0);
INSERT INTO projects (name, description) VALUES ('p1', '');
INSERT INTO projects (name, description) VALUES ('p2', '');
INSERT INTO projects (name, description) VALUES ('p3', '');
INSERT INTO certificates_projects (certificate_id, project_id) VALUES (1, 2);
INSERT INTO certificates_projects (certificate_id, project_id) VALUES (1, 3);
INSERT INTO certificates_projects (certificate_id, project_id) VALUES (1, 4);
`, c1, c2, c1, c2)
`, c1, c2, c1, c2, c2)
require.NoError(t, err)
})
require.NoError(t, err)
Expand Down Expand Up @@ -833,7 +834,14 @@ INSERT INTO certificates_projects (certificate_id, project_id) VALUES (1, 4);
assert.Equal(t, c1, metadata.Certificate)

identity = getTLSIdentityByFingerprint("60c8d8eef45f0570ce713864c8f60b4844d3a8849b262c77cb18e88394d8e6ec")
assert.Equal(t, api.IdentityTypeCertificateMetrics, string(identity.Type))
assert.Equal(t, api.IdentityTypeCertificateMetricsRestricted, string(identity.Type))
assert.Equal(t, "metrics", identity.Name)
err = json.Unmarshal([]byte(identity.Metadata), &metadata)
require.NoError(t, err)
assert.Equal(t, c2, metadata.Certificate)

identity = getTLSIdentityByFingerprint("47c88da8fd0cb9a8d44768a445e6c27aee44e078ce74cbaec0726de427bac056")
assert.Equal(t, api.IdentityTypeCertificateMetricsUnrestricted, string(identity.Type))
assert.Equal(t, "metrics", identity.Name)
err = json.Unmarshal([]byte(identity.Metadata), &metadata)
require.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion lxd/identity/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func IsRestrictedIdentityType(identityType string) (bool, error) {
// identity types must correspond to an authentication method. An error is returned if the identity type is not recognised.
func AuthenticationMethodFromIdentityType(identityType string) (string, error) {
switch identityType {
case api.IdentityTypeCertificateClientRestricted, api.IdentityTypeCertificateClientUnrestricted, api.IdentityTypeCertificateServer, api.IdentityTypeCertificateMetrics:
case api.IdentityTypeCertificateClientRestricted, api.IdentityTypeCertificateClientUnrestricted, api.IdentityTypeCertificateServer, api.IdentityTypeCertificateMetricsRestricted, api.IdentityTypeCertificateMetricsUnrestricted:
return api.AuthenticationMethodTLS, nil
case api.IdentityTypeOIDCClient:
return api.AuthenticationMethodOIDC, nil
Expand Down
24 changes: 5 additions & 19 deletions lxd/metrics/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import (
"strings"

"github.com/canonical/lxd/shared"
"github.com/canonical/lxd/shared/api"
"github.com/canonical/lxd/shared/entity"
)

// NewMetricSet returns a new MetricSet.
Expand All @@ -24,26 +22,14 @@ func NewMetricSet(labels map[string]string) *MetricSet {
return &out
}

// FilterSamples filters the existing MetricSet using the given permission checker. Samples not containing the "project" label are skipped.
func (m *MetricSet) FilterSamples(permissionChecker func(entityURL *api.URL) bool) {
// FilterSamples filters the existing samples based on the provided check.
// When checking a sample for permission its labels get passed into the check function
// so that the samples relation to specific identity types can be used for verification.
func (m *MetricSet) FilterSamples(permissionCheck func(labels map[string]string) bool) {
for metricType, samples := range m.set {
allowedSamples := make([]Sample, 0, len(samples))
for _, s := range samples {
projectName := s.Labels["project"]
instanceName := s.Labels["name"]
if projectName == "" {
continue
}

var hasPermission bool

if instanceName != "" {
hasPermission = permissionChecker(entity.InstanceURL(projectName, instanceName))
} else {
hasPermission = permissionChecker(entity.ProjectURL(projectName))
}

if hasPermission {
if permissionCheck(s.Labels) {
allowedSamples = append(allowedSamples, s)
}
}
Expand Down
13 changes: 6 additions & 7 deletions lxd/metrics/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (

"github.com/stretchr/testify/require"

"github.com/canonical/lxd/shared/api"
"github.com/canonical/lxd/shared/entity"
)

Expand All @@ -20,21 +19,21 @@ func TestMetricSet_FilterSamples(t *testing.T) {
}

m := newMetricSet()
permissionChecker := func(u *api.URL) bool {
return u.String() == entity.InstanceURL("default", "jammy").String()
filter := func(labels map[string]string) bool {
return entity.InstanceURL(labels["project"], labels["name"]).String() == entity.InstanceURL("default", "jammy").String()
}

m.FilterSamples(permissionChecker)
m.FilterSamples(filter)

// Should still contain the sample
require.Equal(t, []Sample{{Value: 10, Labels: labels}}, m.set[CPUSecondsTotal])

m = newMetricSet()
permissionChecker = func(u *api.URL) bool {
return u.String() == entity.InstanceURL("not-default", "jammy").String()
filter = func(labels map[string]string) bool {
return entity.InstanceURL(labels["project"], labels["name"]).String() == entity.InstanceURL("not-default", "jammy").String()
}

m.FilterSamples(permissionChecker)
m.FilterSamples(filter)

// Should no longer contain the sample.
require.Equal(t, []Sample{}, m.set[CPUSecondsTotal])
Expand Down
9 changes: 6 additions & 3 deletions shared/api/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ const (
// IdentityTypeCertificateServer represents cluster member authentication.
IdentityTypeCertificateServer = "Server certificate"

// IdentityTypeCertificateMetrics represents identities that may only view metrics.
IdentityTypeCertificateMetrics = "Metrics certificate"
// IdentityTypeCertificateMetricsRestricted represents identities that may only view metrics and are not privileged.
IdentityTypeCertificateMetricsRestricted = "Metrics certificate (restricted)"

// IdentityTypeOIDCClient represents an identity that authenticates with OIDC..
// IdentityTypeCertificateMetricsUnrestricted represents identities that may only view metrics and are privileged.
IdentityTypeCertificateMetricsUnrestricted = "Metrics certificate (unrestricted)"

// IdentityTypeOIDCClient represents an identity that authenticates with OIDC.
IdentityTypeOIDCClient = "OIDC client"
)

Expand Down
Loading
Loading