Skip to content

Commit

Permalink
Auth: Add storage volume and bucket location to URL in access check (#…
Browse files Browse the repository at this point in the history 
…13517)

This PR fixes fine grained authorization for storage volumes and storage
buckets in clustered LXD.

When listing volumes, we can use the `Location` field of the storage
volume or bucket to perform an access check.

When performing an action on a single storage volume, we need to perform
the following checks:
1. Is the pool containing the volume remote? If so, the `target` query
parameter may have been used to target a particular cluster member, but
should not be used in the URL for the volume (volumes in remote pools do
not have a location).
2. Is the volume located on another cluster member? If so, and the
target parameter is set, use the target parameter as the location. If
so, and the target parameter is unset, use the cluster member name as
the location. If not, use the server name as the location.

To prevent extra queries from being performed when a request is
forwarded to another node. Each node will assume that forwarded requests
have already reached their intended destination. This is done by
checking for `request.CtxForwardedProtocol` in the request context (I
tried this originally by setting the `target` parameter but this broke
storage volume migrations).

Closes #13365
  • Loading branch information
@tomponline
tomponline authored Jul 29, 2024
2 parents 3153b53 + f0ac51f commit 8964692
Show file tree
Hide file tree
Showing 7 changed files with 589 additions and 906 deletions.
81 changes: 0 additions & 81 deletions lxd/cluster/connect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"github.com/canonical/lxd/lxd/instance/instancetype"
"github.com/canonical/lxd/lxd/request"
"github.com/canonical/lxd/lxd/state"
storagePools "github.com/canonical/lxd/lxd/storage"
"github.com/canonical/lxd/shared"
"github.com/canonical/lxd/shared/api"
"github.com/canonical/lxd/shared/version"
Expand Down Expand Up @@ -121,86 +120,6 @@ func ConnectIfInstanceIsRemote(s *state.State, projectName string, instName stri
return client, nil
}

// ConnectIfVolumeIsRemote figures out the address of the cluster member on which the volume with the given name is
// defined. If it's not the local cluster member it will connect to it and return the connected client, otherwise
// it just returns nil. If there is more than one cluster member with a matching volume name, an error is returned.
func ConnectIfVolumeIsRemote(s *state.State, poolName string, projectName string, volumeName string, volumeType int, networkCert *shared.CertInfo, serverCert *shared.CertInfo, r *http.Request) (lxd.InstanceServer, error) {
localNodeID := s.DB.Cluster.GetNodeID()
var err error
var nodes []db.NodeInfo
var poolID int64
err = s.DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error {
poolID, err = tx.GetStoragePoolID(ctx, poolName)
if err != nil {
return err
}

nodes, err = tx.GetStorageVolumeNodes(ctx, poolID, projectName, volumeName, volumeType)
if err != nil {
return err
}

return nil
})
if err != nil && err != db.ErrNoClusterMember {
return nil, err
}

// If volume uses a remote storage driver and so has no explicit cluster member, then we need to check
// whether it is exclusively attached to remote instance, and if so then we need to forward the request to
// the node whereit is currently used. This avoids conflicting with another member when using it locally.
if err == db.ErrNoClusterMember {
// GetStoragePoolVolume returns a volume with an empty Location field for remote drivers.
var dbVolume *db.StorageVolume
err = s.DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error {
dbVolume, err = tx.GetStoragePoolVolume(ctx, poolID, projectName, volumeType, volumeName, true)
return err
})
if err != nil {
return nil, err
}

remoteInstance, err := storagePools.VolumeUsedByExclusiveRemoteInstancesWithProfiles(s, poolName, projectName, &dbVolume.StorageVolume)
if err != nil {
return nil, fmt.Errorf("Failed checking if volume %q is available: %w", volumeName, err)
}

if remoteInstance == nil {
// Volume isn't exclusively attached to an instance. Use local cluster member.
return nil, nil
}

var instNode db.NodeInfo
err = s.DB.Cluster.Transaction(s.ShutdownCtx, func(ctx context.Context, tx *db.ClusterTx) error {
instNode, err = tx.GetNodeByName(ctx, remoteInstance.Node)
return err
})
if err != nil {
return nil, fmt.Errorf("Failed getting cluster member info for %q: %w", remoteInstance.Node, err)
}

// Replace node list with instance's cluster member node (which might be local member).
nodes = []db.NodeInfo{instNode}
}

nodeCount := len(nodes)
if nodeCount > 1 {
return nil, fmt.Errorf("More than one cluster member has a volume named %q. Please target a specific member", volumeName)
} else if nodeCount < 1 {
// Should never get here.
return nil, fmt.Errorf("Volume %q has empty cluster member list", volumeName)
}

node := nodes[0]
if node.ID == localNodeID {
// Use local cluster member if volume belongs to this local member.
return nil, nil
}

// Connect to remote cluster member.
return Connect(node.Address, networkCert, serverCert, r, false)
}

// SetupTrust is a convenience around InstanceServer.CreateCertificate that adds the given server certificate to
// the trusted pool of the cluster at the given address, using the given token. The certificate is added as
// type CertificateTypeServer to allow intra-member communication. If a certificate with the same fingerprint
Expand Down
23 changes: 9 additions & 14 deletions lxd/response.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,26 +57,21 @@ func forwardedResponseIfInstanceIsRemote(s *state.State, r *http.Request, projec
return response.ForwardedResponse(client, r), nil
}

// forwardedResponseIfVolumeIsRemote redirects a request to the node hosting
// the volume with the given pool ID, name and type. If the container is local,
// nothing gets done and nil is returned. If more than one node has a matching
// volume, an error is returned.
//
// This is used when no targetNode is specified, and saves users some typing
// when the volume name/type is unique to a node.
func forwardedResponseIfVolumeIsRemote(s *state.State, r *http.Request, poolName string, projectName string, volumeName string, volumeType int) response.Response {
if request.QueryParam(r, "target") != "" {
// forwardedResponseIfVolumeIsRemote checks for the presence of the ctxStorageVolumeRemoteNodeInfo key in the request context.
// If it is present, the db.NodeInfo value for this key is used to set up a client for the indicated member and forward the request.
// Otherwise, a nil response is returned to indicate that the request was not forwarded, and should continue within this member.
func forwardedResponseIfVolumeIsRemote(s *state.State, r *http.Request) response.Response {
storageVolumeDetails, err := request.GetCtxValue[storageVolumeDetails](r.Context(), ctxStorageVolumeDetails)
if err != nil {
return nil
} else if storageVolumeDetails.forwardingNodeInfo == nil {
return nil
}

client, err := cluster.ConnectIfVolumeIsRemote(s, poolName, projectName, volumeName, volumeType, s.Endpoints.NetworkCert(), s.ServerCert(), r)
client, err := cluster.Connect(storageVolumeDetails.forwardingNodeInfo.Address, s.Endpoints.NetworkCert(), s.ServerCert(), r, false)
if err != nil {
return response.SmartError(err)
}

if client == nil {
return nil
}

return response.ForwardedResponse(client, r)
}
Loading

0 comments on commit 8964692

Please sign in to comment.