server/auth: disallow creating empty permission ranges #15294
Conversation
|
Sorry for missing the issue. As a retrospective, I'm considering to improve the interval tree library for denying inputs like begin >= end in NewInterval() functions e.g. NewBytesAffineInterval . I also think updating other functions like Insert() might be possible. But they will update signature types for having a new error. I'd like to have other people's opinions. |
|
Thanks for checking @ahrtr , I updated this PR based on your comments. Could you take a look? |
Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com> Co-authored-by: Benjamin Wang <wachao@vmware.com>
|
@ahrtr Resolved the latest comments, thanks for reviewing! |
|
cc @ptabor @serathius @spzala the PR has more than 300 lines of code change, but actually it's just a minor change. It has only around 30 lines of production code change, all others are unit test cases. PTAL. |
|
@ptabor @serathius @spzala it’s great if you can cross check |
| @@ -155,3 +157,51 @@ type unifiedRangePermissions struct { | |||
| readPerms adt.IntervalTree | |||
| writePerms adt.IntervalTree | |||
| } | |||
|
|
|||
| // Constraints related to key range | |||
There was a problem hiding this comment.
Does same rules apply for making a range request?
There was a problem hiding this comment.
Yes, the same rules are used by range, watch and delete.
There was a problem hiding this comment.
Could they use the same function to validate the range?
There was a problem hiding this comment.
I think that's possible. It seems that etcdctl doesn't perform this kind of check for now e.g. etcdctl get k2 k1 or etcdctl del k2 k1. They perform nothing so are not harmful but printing errors might be friendly. If you think it's valuable I'll open PRs for it.
There was a problem hiding this comment.
I'm more worried about consistency, feel free to create an issue with the suggestion.
Current
RoleGrantPermissionallows to create permission for ranges like (a, a) or (b, a). These are treated as empty ranges so not harmful. But they are quite confusing and etcd should disallow it.cc @ahrtr @serathius @spzala @ptabor