infobloxopen / seal

Apache License 2.0
16 stars 11 forks source link

Compiled rego for negation (NOT operator) is incorrect #124

Closed rchowinfoblox closed 3 years ago

rchowinfoblox commented 3 years ago

The NOT operator (negation) is supposed to have higher precedence than AND operator. Suppose we have this (admittedly contrived) SEAL statement using NOT operator:

allow subject group everyone to manage petstore.pet
where not ctx.id == "shouldnotmatch" and ctx.id == "shouldmatch";

The front-end SEAL compiler correctly parses the where condition, according to this SEAL compiler debug log:

DEBU[0000] compileCondition                              condition="((not(ctx.id == \"shouldnotmatch\")) and (ctx.id == \"shouldmatch\"))" lvl=0 method=compileCondition type="&ast.InfixCondition{Token:token.Token{Type:\"and\", Literal:\"and\"}, Left:(*ast.PrefixCondition)(0xc000292b80), Operator:\"and\", Right:(*ast.InfixCondition)(0xc0002791d0)}"

But the back-end rego compiler generates incorrect rego:

allow {
    seal_list_contains(seal_subject.groups, `everyone`)
    seal_list_contains(base_verbs[input.type][`manage`], input.verb)
    re_match(`petstore.pet`, input.type)
    not line1_not1_cnd
}

line1_not1_cnd {
    some i
    input.ctx[i]["id"] == "shouldnotmatch"

    input.ctx[i]["id"] == "shouldmatch"
}

The generated incorrect rego is the equivalent of AND having higher precedence:

(not  ((ctx.id == "shouldnotmatch") and (ctx.id == "shouldmatch"))  )

which is not the same as desired NOT having higher precedence:

( (not(ctx.id == "shouldnotmatch"))  and  (ctx.id == "shouldmatch") )
wplu commented 3 years ago

As you said - NOT should have higher affinity than AND: https://github.com/infobloxopen/seal/blob/master/pkg/parser/condition.go#L160-L172

    PRECEDENCE_LOWEST
    PRECEDENCE_OR          // logical or
    PRECEDENCE_AND         // logical and
    PRECEDENCE_NOT         // logical not
    PRECEDENCE_EQUALS      // ==
    PRECEDENCE_LESSGREATER // > or <
    PRECEDENCE_SUM         // +
    PRECEDENCE_PRODUCT     // *
    PRECEDENCE_PREFIX      // -X or !X
    PRECEDENCE_CALL        // myFunction(X)
rchowinfoblox commented 3 years ago

Fix PR https://github.com/infobloxopen/seal/pull/125 Merged https://github.com/infobloxopen/seal/commit/543e4131cdfc11051d84821c4f86dcd74b3d0841