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

[1.10.x] acl: fix txn_endpoint to properly authorize service registrations #10798

Merged
merged 2 commits into from
Aug 5, 2021

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Aug 5, 2021

Backport of the bugfix commit from #10793

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.
@dnephin dnephin added theme/acls ACL and token generation backport/1.8 labels Aug 5, 2021
@dnephin dnephin requested a review from a team August 5, 2021 19:45
@github-actions github-actions bot removed the theme/acls ACL and token generation label Aug 5, 2021
@vercel vercel bot temporarily deployed to Preview – consul August 5, 2021 19:46 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging August 5, 2021 19:46 Inactive
Copy link
Contributor

@freddygv freddygv left a 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.

@dnephin dnephin merged commit 7720275 into release/1.10.x Aug 5, 2021
@dnephin dnephin deleted the dnephin/backport-txn-authz-fix branch August 5, 2021 21:53
@hc-github-team-consul-core
Copy link
Contributor

🍒 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.

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit 7720275 onto release/1.9.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Aug 5, 2021
[1.10.x] acl: fix txn_endpoint to properly authorize service registrations
@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit 7720275 onto release/1.8.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Aug 5, 2021
[1.10.x] acl: fix txn_endpoint to properly authorize service registrations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants