Etcd: auth: strange compare condition in etcd/auth/store.go

[Sheph]

I've came across this check in etcd/auth/store.go:

func (as *authStore) isOpPermitted(userName string, revision uint64, key, rangeEnd []byte, permTyp authpb.Permission_Type) error {
        ...
    if revision < as.Revision() {
        return ErrAuthOldRevision
    }
        ...
}

Why does it check for revision < as.Revision() ? Shouldn't it be something like revision != as.Revision(), I mean, is it really ok if revision is > as.Revision() ?

[spzala]

I guess the return error is the answer - ErrAuthOldRevision an older revision is an error case. /cc @mitake for a better explanation. Thanks!

[Sheph]

Yes, but why is that, why is older revision an error, but newer revision is not an error ? I guess this has something to do with how raft works, but IMO all auth requests are serialized on raft node loop, so if a request comes in and makes an auth check it's guaranteed that all earlier raft requests have already succeeded, so 'revision' must be equal to auth store revision strictly, it should not be greater... One case I can think of when it can be greater is when I authorized on one node, took the token and then went with it to another node and issued a serializable (not linearizable) get request, in that case it doesn't go through raft, but rater does the check against local auth store right away. That local auth store might not have catched up with the cluster yet, so it might have older auth store revision, but auth check should pass, thus this comparison... So I've sort of answered my question myself :) But the next question is - is this the only case ? The short story is that I'm working on a patch for etcd for personal project and I really really need this condition to be !=, not <, even if that means that I need to cut down some etcd functionality...

[spzala]

:) @Sheph let's wait to hear from @mitake Thanks!

[mitake]

@Sheph @spzala Sorry for my late reply.

One case I can think of when it can be greater is when I authorized on one node, took the token and then went with it to another node and issued a serializable (not linearizable) get request, in that case it doesn't go through raft, but rater does the check against local auth store right away.

I think the scenario can be possible. Nice catch! Also I like your idea of defensive programming. I completely agree that requiring strict equal revision from token makes sense! Could you send your change as PR?

[Sheph]

Thanks for the reply, @mitake. BTW, when working on my feature I touched a lot of places with auth and found quite a number of security problems, some of them lead to privilege escalation, some of them allow unauthorized operation, etc. I've fixed them on my local branch, but the commits are mixed with some other stuff, I'm planning to make a series of PRs soon, it'll include a fix for this one too. I also have some concerns regarding jwt tokens and some ideas on how to fix them, I'll try to open a separate ticket for that soon too

[mitake]

@Sheph thanks a lot! Could you share the detail of the issues to [email protected] before opening PRs (the security related process is described here https://github.com/etcd-io/etcd/tree/master/security)?

[Sheph]

Sure! Thanks for the tip

[jingyih]

Yes, but why is that, why is older revision an error, but newer revision is not an error ? I guess this has something to do with how raft works, but IMO all auth requests are serialized on raft node loop, so if a request comes in and makes an auth check it's guaranteed that all earlier raft requests have already succeeded, so 'revision' must be equal to auth store revision strictly, it should not be greater... One case I can think of when it can be greater is when I authorized on one node, took the token and then went with it to another node and issued a serializable (not linearizable) get request, in that case it doesn't go through raft, but rater does the check against local auth store right away. That local auth store might not have catched up with the cluster yet, so it might have older auth store revision, but auth check should pass, thus this comparison... So I've sort of answered my question myself :) But the next question is - is this the only case ? The short story is that I'm working on a patch for etcd for personal project and I really really need this condition to be !=, not <, even if that means that I need to cut down some etcd functionality...

I found some relevant info here: https://github.com/etcd-io/etcd/blob/master/Documentation/learning/design-auth-v3.md#notes-on-the-implementation-of-authenticate-rpc

[Sheph]

Thanks, @jingyih. So:

If the numbers differ, it means someone else updated the auth metadata. So it retries the checking.

Is another reason to make it != instead of just <

[jingyih]

Agreed. "!=" sounds good to me.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.