-
Notifications
You must be signed in to change notification settings - Fork 925
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
Avoid race between operation and events listener #12885
Closed
Closed
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f26353c
client/lxd: Add supportsAuthentication field to ProtocolLXD
masnax 55de107
client/lxd/events: Only use events API if client supports auth
masnax 1085085
client: Create events listeners before making the op request
masnax 72bd241
client/operations: Don't create events listener after starting an ope…
masnax 5017a40
client/operations: Remove skipListener field
masnax 861f757
client/lxd: Expose SupportsAuthentication field
masnax c3a3f44
lxc: Ensure the client supports event handler authentication
masnax File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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
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
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.
@markylaing @masnax will this work with OIDC authenticated clients?
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.
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?
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.
There are two indicators from
ConnectionArgs
:AuthType
andOIDCTokens
. 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), andOIDCTokens
will not be set if the client has not already authenticated.ProtocolLXD
has anoidcClient
field that will be non-nil if the client has an access token for that remote, so you could potentially use that.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.
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?
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.
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.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.
@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 havingcan_view_events
andcan_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.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.
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 havecan_view_events
for the whole project in order to perform the exec.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.
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 usinglxc monitor
for example.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.
Please keep in mind the client and lxc command still need to be able to work with older servers
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.
I'm not sure I follow, that would still behave the same if we infer authentication:
can_exec
, so the request fails with 403 anyway, so it wouldn't matter that we set up a bad websocket connection.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?