protect LeaseTimeToLive with RBAC #15656
Conversation
mitake
force-pushed
the
lease-timetolive-auth
branch
from
5e6f889
to
092566f
Compare
mitake
changed the title
mitake
force-pushed
the
lease-timetolive-auth
branch
3 times, most recently
from
67ba663
to
009ba5d
Compare
mitake
changed the title
mitake
force-pushed
the
lease-timetolive-auth
branch
4 times, most recently
from
ce757b1
to
f18d257
Compare
|
@mitake Overall looks good to me, and sorry for the late response. Could you rebase this PR although there is no conflict? |
mitake
force-pushed
the
lease-timetolive-auth
branch
from
f18d257
to
bed84ca
Compare
|
@ahrtr thanks, rebased on the latest main, could you check? |
|
Can we measure the performance impact when someone has enabled auth? I think that all other etcd methods authorization check can be done in constant time. Here we have linear to number of keys under lease. Would be good to find impact for higher number of leases. For example K8s (doesn't use auth, but just an example) has up to 1000 keys per lease. |
| l := s.lessor.Lookup(leaseID) | ||
| if l != nil { | ||
| for _, key := range l.Keys() { | ||
| if err := s.AuthStore().IsRangePermitted(authInfo, []byte(key), []byte{}); err != nil { | ||
| return err, 0 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Before iterating over all the keys, I think we also need to check LeaseTimeToLiveRequest.Keys; if it's false, it means users don't want to query al the keys at all, then no need to check the permission either on the server side.
We also need to add test to cover such case, such as the client has no range permission on a key, but it intentionally set LeaseTimeToLiveRequest.Keys to false, then it should not get an error response.
There was a problem hiding this comment.
Thanks for your comment, I'll update tomorrow.
There was a problem hiding this comment.
Updated, could you check @ahrtr ?
|
@serathius I measured overhead with this simple benchmark which emulates 1000 keys/lease on my local laptop (just for comparing the difference w/ and w/o the checking overhead). Result was like below (unit is microsecond): auth enabled:
auth disabled:
It's a noisy environment so 99 percentile wouldn't be so informative. The workload is quite artificial (e.g. cache hit rate should be quite high) but I think overhead is low enough. What do you think? |
|
Looks good, for the next time I recommend looking into benchmarking https://about.sourcegraph.com/blog/go/gophercon-2019-optimizing-go-code-without-a-blindfold and using https://pkg.go.dev/golang.org/x/perf/cmd/benchstat to eliminate noisy environment and confirming that results are statistically significant. |
One observation, shouldn't we expect auth enabled take more as it one that does checks of keys? |
|
@serathius Sorry I noticed that the benchmark isn't attaching lease correctly :( I did it again and could observe overhead correctly: auth enabled:
auth disabled:
(checking 1000 permission in a few microseconds is clearly impossible even for high cache hit condition...) What do you think?
Thanks for sharing, I'll check these materials. |
Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> Co-authored-by: Benjamin Wang <wachao@vmware.com>
Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> Co-authored-by: Benjamin Wang <wachao@vmware.com>
mitake
force-pushed
the
lease-timetolive-auth
branch
from
bed84ca
to
c9b3681
Compare
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.
Currently,
LeaseTimeToLive()API oretcdctl lease timetolivecommand don't require RBAC permission. However, their responses can have key names attached to the target lease. Value of the keys cannot be accessed through the API, but protecting the key name is better. So this PR let the API require read permission of keys attached to a lease (if a lease isn't attached to specific keys, behavior won't be changed).cc @ahrtr @serathius @spzala @ptabor