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.

Sign up for GitHub

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

Jump to bottom

Avoid race between operation and events listener #12885

Closed
wants to merge 7 commits into from
Closed

Avoid race between operation and events listener #12885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f26353c
client/lxd: Add supportsAuthentication field to ProtocolLXD
masnax Mar 1, 2024
55de107
client/lxd/events: Only use events API if client supports auth
masnax Mar 1, 2024
1085085
client: Create events listeners before making the op request
masnax Feb 13, 2024
72bd241
client/operations: Don't create events listener after starting an ope…
masnax Feb 13, 2024
5017a40
client/operations: Remove skipListener field
masnax Mar 1, 2024
861f757
client/lxd: Expose SupportsAuthentication field
masnax Mar 18, 2024
c3a3f44
lxc: Ensure the client supports event handler authentication
masnax Mar 18, 2024
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
49 changes: 32 additions & 17 deletions client/connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,18 +112,24 @@ func ConnectLXDHTTPWithContext(ctx context.Context, args *ConnectionArgs, client
return nil, err
}

t, ok := client.Transport.(*http.Transport)
if !ok {
return nil, fmt.Errorf("Invalid HTTP Transport type")
}

ctxConnected, ctxConnectedCancel := context.WithCancel(context.Background())

// Initialize the client struct
server := ProtocolLXD{
ctx: ctx,
httpBaseURL: *httpBaseURL,
httpProtocol: "custom",
httpUserAgent: args.UserAgent,
ctxConnected: ctxConnected,
ctxConnectedCancel: ctxConnectedCancel,
eventConns: make(map[string]*websocket.Conn),
eventListeners: make(map[string][]*EventListener),
ctx: ctx,
httpBaseURL: *httpBaseURL,
httpProtocol: "custom",
httpUserAgent: args.UserAgent,
ctxConnected: ctxConnected,
ctxConnectedCancel: ctxConnectedCancel,
eventConns: make(map[string]*websocket.Conn),
eventListeners: make(map[string][]*EventListener),
supportsAuthentication: t.TLSClientConfig != nil && len(t.TLSClientConfig.Certificates) > 0,
Copy link
Member

Choose a reason for hiding this comment

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

@markylaing @masnax will this work with OIDC authenticated clients?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, I'm guessing no since they won't have TLS certs. @markylaing is there something we can check to see if the client intends to use OIDC auth?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two indicators from ConnectionArgs: AuthType and OIDCTokens. I don't think this will work reliably though, AuthType may or may not be set (we have a default preference for OIDC over TLS), and OIDCTokens will not be set if the client has not already authenticated.

ProtocolLXD has an oidcClient field that will be non-nil if the client has an access token for that remote, so you could potentially use that.

Copy link
Member

Choose a reason for hiding this comment

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

The tests are passing suggesting that the tests arent covering this scenario with the mini-oidc service, can they be updated to check for the regression now and then we can see the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know much about the mini-oidc tests, but the related change from this PR just determines whether we use long polling or a websocket to track an operation. So we would need a test that explicitly tries to connect to the events websocket to track an operation, and fail if we hit the /1.0/operations/wait API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markylaing Is there a case where the client should be able to create and then track long-lived operations without having can_view_events & can_view_projects? Would it be valid and expected to work if a user tries to create/edit/manage instances/images/storage without also having can_view_events and can_view_projects?

My expectation was no, that you would need a set of entitlements inclusive of those two for those types of actions, and in that case checking for an intent to authenticate should be enough, we don't necessarily need to know if the connection will be authorized for /1.0/events. We already don't for TLS auth, since we just check if certs exist, not if they're actually going to be accepted by the server. So we could let the endpoint just handle the error and correctly return 403 if a client is authorized to create or manage images/instances/storage, but not authorized to use the events API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, ideally we would like to decouple this and have the entitlements work in isolation. For example, a client with can_exec on an instance shouldn't be required to have can_view_events for the whole project in order to perform the exec.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been able to dive into decoupling these yet but my thinking is that adding websocket functionality to /1.0/operations/{uuid}/wait OR adding a /1.0/operations/{uuid}/events would be the way to go. Both can be untrusted and require the websocket secret to function. We would then stop using events in the client except for when specifically using lxc monitor for example.

Copy link
Member

Choose a reason for hiding this comment

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

Please keep in mind the client and lxc command still need to be able to work with older servers

Copy link
Contributor Author

@masnax masnax Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, ideally we would like to decouple this and have the entitlements work in isolation. For example, a client with can_exec on an instance shouldn't be required to have can_view_events for the whole project in order to perform the exec.

I'm not sure I follow, that would still behave the same if we infer authentication:

  • The client intends to use OIDC auth, but does not have can_exec, so the request fails with 403 anyway, so it wouldn't matter that we set up a bad websocket connection.
  • The client does not intend to use OIDC auth, but tries to exec anyway, so the request fails as it's untrusted. In this case we would have skipped the websocket too, but it's ineffectual.
  • The client intends to use OIDC auth, so we set up the websocket listener. The client has all the necessary entitlements so the request succeeds.

Seems to me that's true especially if we want to have entitlements work in isolation. That would mean that whenever a client is authorized for a particular action that can involve a websocket, it must necessarily be authorized to start a websocket on that action by the fact that its authorized for the action itself. As you said, we don't need can_view_events to view events in that case.

Can you give an example of a situation where we might incorrectly infer that we should start a websocket for a particular action, that action is in fact authorized, but the websocket connection is not?

}

// Setup the HTTP client
Expand Down Expand Up @@ -174,15 +180,16 @@ func ConnectLXDUnixWithContext(ctx context.Context, path string, args *Connectio

// Initialize the client struct
server := ProtocolLXD{
ctx: ctx,
httpBaseURL: *httpBaseURL,
httpUnixPath: path,
httpProtocol: "unix",
httpUserAgent: args.UserAgent,
ctxConnected: ctxConnected,
ctxConnectedCancel: ctxConnectedCancel,
eventConns: make(map[string]*websocket.Conn),
eventListeners: make(map[string][]*EventListener),
ctx: ctx,
httpBaseURL: *httpBaseURL,
httpUnixPath: path,
httpProtocol: "unix",
httpUserAgent: args.UserAgent,
supportsAuthentication: true,
ctxConnected: ctxConnected,
ctxConnectedCancel: ctxConnectedCancel,
eventConns: make(map[string]*websocket.Conn),
eventListeners: make(map[string][]*EventListener),
}

// Determine the socket path
Expand Down Expand Up @@ -354,5 +361,13 @@ func httpsLXD(ctx context.Context, requestURL string, args *ConnectionArgs) (Ins
return nil, err
}
}

t, ok := httpClient.Transport.(*http.Transport)
if !ok {
return nil, fmt.Errorf("Invalid HTTP Transport type")
}

server.supportsAuthentication = t.TLSClientConfig != nil && len(t.TLSClientConfig.Certificates) > 0

return &server, nil
}
1 change: 1 addition & 0 deletions client/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@ type InstanceServer interface {
DeleteWarning(UUID string) (err error)

// Authorization functions
SupportsAuthentication() bool
GetAuthGroupNames() (groupNames []string, err error)
GetAuthGroups() (groups []api.AuthGroup, err error)
GetAuthGroup(groupName string) (group *api.AuthGroup, ETag string, err error)
Expand Down
19 changes: 14 additions & 5 deletions client/lxd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ type ProtocolLXD struct {
httpProtocol string
httpUserAgent string

// supportsAuthentication returns whether the client can attempt to make trusted connections to its target server.
// A unix client, or an http client with TLS certificates will be considered to support authentication.
supportsAuthentication bool

requireAuthenticated bool

clusterTarget string
Expand Down Expand Up @@ -172,6 +176,12 @@ func (r *ProtocolLXD) addClientHeaders(req *http.Request) {
}
}

// SupportsAuthentication returns whether the client can attempt to make trusted connections to its target server.
// A unix client, or an http client with TLS certificates will be considered to support authentication.
func (r *ProtocolLXD) SupportsAuthentication() bool {
return r.supportsAuthentication
}

// RequireAuthenticated sets whether we expect to be authenticated with the server.
func (r *ProtocolLXD) RequireAuthenticated(authenticated bool) {
r.requireAuthenticated = authenticated
Expand Down Expand Up @@ -395,11 +405,10 @@ func (r *ProtocolLXD) queryOperation(method string, path string, data any, ETag

// Setup an Operation wrapper
op := operation{
Operation: *respOperation,
r: r,
listener: listener,
chActive: make(chan bool),
skipListener: !useEventListener,
Operation: *respOperation,
r: r,
listener: listener,
chActive: make(chan bool),
}

// Log the data
Expand Down
29 changes: 23 additions & 6 deletions client/lxd_containers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,16 @@ func (r *ProtocolLXD) CreateContainerFromBackup(args ContainerBackupArgs) (Opera
req.Header.Set("Content-Type", "application/octet-stream")
req.Header.Set("X-LXD-pool", args.PoolName)

// Set up the events listener before making the request so that
// the operation doesn't complete and emit the event before we've begun listening.
var listener *EventListener
if r.supportsAuthentication {
listener, err = r.getEvents(false)
if err != nil {
return nil, err
}
}

// Send the request
resp, err := r.DoHTTP(req)
if err != nil {
Expand All @@ -141,6 +151,7 @@ func (r *ProtocolLXD) CreateContainerFromBackup(args ContainerBackupArgs) (Opera
// Setup an Operation wrapper
op := operation{
Operation: *respOperation,
listener: listener,
r: r,
chActive: make(chan bool),
}
Expand Down Expand Up @@ -196,8 +207,10 @@ func (r *ProtocolLXD) tryCreateContainer(req api.ContainersPost, urls []string)

rop.targetOp = op

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if r.supportsAuthentication {
Copy link
Member

Choose a reason for hiding this comment

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

Can you use SupportsAuthentication everywhere in client so that when we search for usage of this function we get a clear picture of the usage.

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
Copy link
Member

Choose a reason for hiding this comment

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

Should we be ignoring the error returned from AddHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen this sort of thing all over the place wherever we try to set up an events listener with getEvents or AddHandler and I didn't understand the reason.

}
}

err = rop.targetOp.Wait()
Expand Down Expand Up @@ -559,8 +572,10 @@ func (r *ProtocolLXD) tryMigrateContainer(source InstanceServer, name string, re

rop.targetOp = op

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if source.SupportsAuthentication() {
for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
}
}

err = rop.targetOp.Wait()
Expand Down Expand Up @@ -1251,8 +1266,10 @@ func (r *ProtocolLXD) tryMigrateContainerSnapshot(source InstanceServer, contain

rop.targetOp = op

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if source.SupportsAuthentication() {
for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
}
}

err = rop.targetOp.Wait()
Expand Down
4 changes: 4 additions & 0 deletions client/lxd_events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ func (r *ProtocolLXD) getEvents(allProjects bool) (*EventListener, error) {
ctxCancel: cancel,
}

if !r.supportsAuthentication {
return nil, fmt.Errorf("Failed to connect to the Events interface, the client does not support authentication")
}

connInfo, _ := r.GetConnectionInfo()
if connInfo.Project == "" {
return nil, fmt.Errorf("Unexpected empty project in connection info")
Expand Down
23 changes: 19 additions & 4 deletions client/lxd_images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,16 @@ func (r *ProtocolLXD) CreateImage(image api.ImagesPost, args *ImageCreateArgs) (
return nil, err
}

// Set up the events listener before making the request so that
// the operation doesn't complete and emit the event before we've begun listening.
var listener *EventListener
if r.supportsAuthentication {
listener, err = r.getEvents(false)
if err != nil {
return nil, err
}
}

req, err := http.NewRequest("POST", reqURL, body)
if err != nil {
return nil, err
Expand Down Expand Up @@ -567,6 +577,7 @@ func (r *ProtocolLXD) CreateImage(image api.ImagesPost, args *ImageCreateArgs) (
op := operation{
Operation: *respOperation,
r: r,
listener: listener,
chActive: make(chan bool),
}

Expand Down Expand Up @@ -642,8 +653,10 @@ func (r *ProtocolLXD) tryCopyImage(req api.ImagesPost, urls []string) (RemoteOpe
rop.targetOp = op
rop.handlerLock.Unlock()

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if r.supportsAuthentication {
for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
}
}

err = rop.targetOp.Wait()
Expand Down Expand Up @@ -843,8 +856,10 @@ func (r *ProtocolLXD) CopyImage(source ImageServer, image api.Image, args *Image
rop.targetOp = op
rop.handlerLock.Unlock()

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if r.supportsAuthentication {
for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
}
}

err = rop.targetOp.Wait()
Expand Down
35 changes: 27 additions & 8 deletions client/lxd_instances.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,10 @@ func (r *ProtocolLXD) tryRebuildInstance(instanceName string, req api.InstanceRe
rop.targetOp = op
rop.handlerLock.Unlock()

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if r.supportsAuthentication {
for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
}
}

err = rop.targetOp.Wait()
Expand Down Expand Up @@ -656,6 +658,16 @@ func (r *ProtocolLXD) CreateInstanceFromBackup(args InstanceBackupArgs) (Operati
req.Header.Set("X-LXD-devices", devProps.Encode())
}

// Set up the events listener before making the request so that
// the operation doesn't complete and emit the event before we've begun listening.
var listener *EventListener
if r.supportsAuthentication {
listener, err = r.getEvents(false)
if err != nil {
return nil, err
}
}

// Send the request
resp, err := r.DoHTTP(req)
if err != nil {
Expand All @@ -680,6 +692,7 @@ func (r *ProtocolLXD) CreateInstanceFromBackup(args InstanceBackupArgs) (Operati
op := operation{
Operation: *respOperation,
r: r,
listener: listener,
chActive: make(chan bool),
}

Expand Down Expand Up @@ -743,8 +756,10 @@ func (r *ProtocolLXD) tryCreateInstance(req api.InstancesPost, urls []string, op
rop.targetOp = op
rop.handlerLock.Unlock()

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if r.supportsAuthentication {
for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
}
}

err = rop.targetOp.Wait()
Expand Down Expand Up @@ -1100,8 +1115,10 @@ func (r *ProtocolLXD) tryMigrateInstance(source InstanceServer, name string, req

rop.targetOp = op

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if source.SupportsAuthentication() {
for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
}
}

err = rop.targetOp.Wait()
Expand Down Expand Up @@ -2099,8 +2116,10 @@ func (r *ProtocolLXD) tryMigrateInstanceSnapshot(source InstanceServer, instance

rop.targetOp = op

for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
if source.SupportsAuthentication() {
for _, handler := range rop.handlers {
_, _ = rop.targetOp.AddHandler(handler)
}
}

err = rop.targetOp.Wait()
Expand Down
62 changes: 32 additions & 30 deletions client/lxd_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,21 +101,22 @@ func (r *ProtocolLXD) GetServerResources() (*api.Resources, error) {
// UseProject returns a client that will use a specific project.
func (r *ProtocolLXD) UseProject(name string) InstanceServer {
return &ProtocolLXD{
ctx: r.ctx,
ctxConnected: r.ctxConnected,
ctxConnectedCancel: r.ctxConnectedCancel,
server: r.server,
http: r.http,
httpCertificate: r.httpCertificate,
httpBaseURL: r.httpBaseURL,
httpProtocol: r.httpProtocol,
httpUserAgent: r.httpUserAgent,
requireAuthenticated: r.requireAuthenticated,
clusterTarget: r.clusterTarget,
project: name,
eventConns: make(map[string]*websocket.Conn), // New project specific listener conns.
eventListeners: make(map[string][]*EventListener), // New project specific listeners.
oidcClient: r.oidcClient,
ctx: r.ctx,
ctxConnected: r.ctxConnected,
ctxConnectedCancel: r.ctxConnectedCancel,
server: r.server,
http: r.http,
httpCertificate: r.httpCertificate,
httpBaseURL: r.httpBaseURL,
httpProtocol: r.httpProtocol,
httpUserAgent: r.httpUserAgent,
supportsAuthentication: r.supportsAuthentication,
requireAuthenticated: r.requireAuthenticated,
clusterTarget: r.clusterTarget,
project: name,
eventConns: make(map[string]*websocket.Conn), // New project specific listener conns.
eventListeners: make(map[string][]*EventListener), // New project specific listeners.
oidcClient: r.oidcClient,
}
}

Expand All @@ -124,21 +125,22 @@ func (r *ProtocolLXD) UseProject(name string) InstanceServer {
// placement, preparing a new storage pool or network, ...
func (r *ProtocolLXD) UseTarget(name string) InstanceServer {
return &ProtocolLXD{
ctx: r.ctx,
ctxConnected: r.ctxConnected,
ctxConnectedCancel: r.ctxConnectedCancel,
server: r.server,
http: r.http,
httpCertificate: r.httpCertificate,
httpBaseURL: r.httpBaseURL,
httpProtocol: r.httpProtocol,
httpUserAgent: r.httpUserAgent,
requireAuthenticated: r.requireAuthenticated,
project: r.project,
eventConns: make(map[string]*websocket.Conn), // New target specific listener conns.
eventListeners: make(map[string][]*EventListener), // New target specific listeners.
oidcClient: r.oidcClient,
clusterTarget: name,
ctx: r.ctx,
ctxConnected: r.ctxConnected,
ctxConnectedCancel: r.ctxConnectedCancel,
server: r.server,
http: r.http,
httpCertificate: r.httpCertificate,
httpBaseURL: r.httpBaseURL,
httpProtocol: r.httpProtocol,
httpUserAgent: r.httpUserAgent,
supportsAuthentication: r.supportsAuthentication,
requireAuthenticated: r.requireAuthenticated,
project: r.project,
eventConns: make(map[string]*websocket.Conn), // New target specific listener conns.
eventListeners: make(map[string][]*EventListener), // New target specific listeners.
oidcClient: r.oidcClient,
clusterTarget: name,
}
}

Expand Down
Loading
Loading