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

lock down KV endpoints #2089

Closed
mberhault opened this issue Aug 13, 2015 · 11 comments
Closed

lock down KV endpoints #2089

mberhault opened this issue Aug 13, 2015 · 11 comments
Assignees
@mberhault
Milestone
Beta

Comments

@mberhault
Copy link
Contributor

KV endpoints should be internal only.
Some or all of the following should be done:

  • restrict KV api to the node user (possibly root as well for now)
  • remove HTTP endpoint
@mberhault mberhault self-assigned this Aug 13, 2015
@tbg
Copy link
Member

tbg commented Aug 13, 2015

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

@mberhault
Copy link
Contributor Author

yes, the points I mentioned above were just the end results. The side affects are fairly sizable: tests, client, bank example, etc..

@petermattis
Copy link
Collaborator

When this is done, we should also move the http sender code from client to sql/driver.

@jess-edwards jess-edwards mentioned this issue Aug 17, 2015
Closed
78 tasks
@jess-edwards jess-edwards added this to the Beta milestone Aug 17, 2015
@mberhault
Copy link
Contributor Author

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

@petermattis
Copy link
Collaborator

Given that rpc has benchmarked as significantly faster than http, I would plan for it happening at some point.

mberhault pushed a commit that referenced this issue Aug 20, 2015
Do not merge: restrict KV endpoint to root and node users.
987dd69 
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.
This was referenced Aug 20, 2015
Merged
Closed
mberhault pushed a commit that referenced this issue Aug 21, 2015
Restrict KV endpoint to root and node users.
83dfb1b 
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.
@tbg
Copy link
Member

tbg commented Aug 21, 2015

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.

@mberhault
Copy link
Contributor Author

there's not all that much to change. In addition to node which will always be needed, I'm still allowing root for now. I never finished porting all the tests to using testuser, so the change was fairly minimal. See #2193

@mberhault
Copy link
Contributor Author

KV endpoint now requires root or node. Permissions have been removed entirely.
Eventually, I think we want to require node only. This would emply:

  • remove Request.Header.User field and hard-code to node (see GetUser in proto/*.go for examples).
  • remove root as an accepted user
  • cleanup all client code to no longer specify Header.User

This would effectively make the KV endpoint unusable without node certificates, meaning anyone messing around with it would have to do so intentionally.

@petermattis
Copy link
Collaborator

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 client to sql/driver. Are you going to tackle those?

@mberhault
Copy link
Contributor Author

Yup. I'm seeing "restrict to node" as part of this, and all others as cleanup/refactor for which I'll probably file other issues. For this week though, I'll be focusing on the structured configs.

@mberhault mberhault mentioned this issue Aug 25, 2015
Merged
mberhault pushed a commit that referenced this issue Aug 26, 2015
Restrict KV endpoint to NodeUser only.
dd2f042 
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.
@mberhault
Copy link
Contributor Author

filed #2271 for follow-up refactoring and cleanup.

@knz knz mentioned this issue Mar 11, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mberhault mberhault

Labels
None yet
Projects
None yet
Milestone
Beta
Development

No branches or pull requests
4 participants
@tbg @mberhault @jess-edwards @petermattis