-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
[1.10.x] acl: fix txn_endpoint to properly authorize service registrations #10798
Conversation
Previously we were passing an Authorizer that would always allow the operation, then later checking the authorization using vetServiceTxnOp. On the surface this seemed strange, but I think it was actually masking a bug as well. Over time `servicePreApply` was changed to add additional authorization for `service.Proxy.DestinationServiceName`, but because we were passing a nil Authorizer, that authorization was not handled on the txn_endpoint. `TxnServiceOp.FillAuthzContext` has some special handling in enterprise, so we need to make sure to continue to use that from the Txn endpoint. This commit removes the `vetServiceTxnOp` function, and passes in the `FillAuthzContext` function so that `servicePreApply` can be used by both the catalog and txn endpoints. This should be much less error prone and prevent bugs like this in the future.
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.
LGTM, I think it would be worth backporting this to 1.9 and 1.8 as well.
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/423200. |
🍒✅ Cherry pick of commit 7720275 onto |
[1.10.x] acl: fix txn_endpoint to properly authorize service registrations
🍒✅ Cherry pick of commit 7720275 onto |
[1.10.x] acl: fix txn_endpoint to properly authorize service registrations
Backport of the bugfix commit from #10793