-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
lock down KV endpoints #2089
Comments
we should also port the bank example to the acceptance tests first. and we need to work on the usability of testing the system using the acceptance tests. getting at the logs is always a hassle so far (ok, different issue). |
yes, the points I mentioned above were just the end results. The side affects are fairly sizable: tests, client, bank example, etc.. |
When this is done, we should also move the http sender code from |
@petermattis: do we plan to do sql over rpc at some point soon? Without it, rpc enforcement is a lot simpler. If we do, there'll be a teensie bit more trickery involved. |
Given that rpc has benchmarked as significantly faster than http, I would plan for it happening at some point. |
Work towards #2089 This is a proposed implementation. The reason it's convoluted is that we need per method restrictions (we'll eventually be doing sql over rpc). This rearranges the authentication hook usage (moves from rpc/codec/ to rpc/server.go) so that it can access a 'restricted' bool passed at method registration time. For now, all RPC handlers are passing true. The http handlers already call the authentication hook manually, so they just specify restricted (kv) or not (sql) at the proper time. If there are no objections to this, I'll go ahead and remove all the multiuser tests and add plain unauthorized tests.
Work towards #2089 * restrict all RPC endpoints to root and node users only. * restrict http kv endpoint to root and node users only. * sql http endpoint allows all users Nothing makes use of the rpc.RegisterPublic method yet, but sql will.
what's the plan about porting all the tests? I'm assuming we won't have to rewrite every single test to use certain key ranges or something like that. |
there's not all that much to change. In addition to |
KV endpoint now requires
This would effectively make the KV endpoint unusable without |
Nice. There are some follow-on tasks mentioned in this issue, such as removing support for the KV http endpoint and moving the http sender code from |
Yup. I'm seeing "restrict to |
Fixes #2089. This removes permissions for root on the kv endpoints by hard-coding the requested user to security.NodeUser. In insecure mode, no permissions required (previously, we defaulted to 'root' so same thing). In secure mode, you need node client certs to do anything.
filed #2271 for follow-up refactoring and cleanup. |
KV endpoints should be internal only.
Some or all of the following should be done:
node
user (possiblyroot
as well for now)The text was updated successfully, but these errors were encountered: