feat: Added authz-casbin plugin and doc and tests for it

[rushitote]

What this PR does / why we need it:

fix: #4674

This PR adds an authorization plugin: authz-casbin to APISIX. This is based on Lua Casbin which is the Lua implementation of Casbin library. The plugin supports enforcement of powerful authorization scenarios based on various access control models supported by Casbin.

The plugin works on the basis of a model file and a policy file. The model acts as a configuration for the policies and policy enforcement. The plugin currently also supports direct model/policy text in absence of files through plugin metadata.

An example of authz model is:

[request_definition]
r = sub, obj, act
[policy_definition]
p = sub, obj, act
[role_definition]
g = _, _
[policy_effect]
e = some(where (p.eft == allow))
[matchers]
m = (g(r.sub, p.sub) || keyMatch(r.sub, p.sub)) && keyMatch(r.obj, p.obj) && keyMatch(r.act, p.act)

And example of authz policy is:

p, *, /, GET
p, admin, *, *
g, alice, admin

This means that any user (subject) can access the homepage which is at / via GET request but only admins can a access other pages and use other request methods. And here, as defined in policy alice has a role as admin and hence she has admin access.

Pre-submission checklist:

• Did you explain what problem does this PR solve? Or what new features have been added?
• Have you added corresponding test cases?
• Have you modified the corresponding document?
• Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

[spacewander]

The plugin_metadata should be only set on the CP side. It's forbidden to let configuration flow from the DP to the CP.

[spacewander]

Since we already use metadata to store the configuration, there is no need to provide APIs on the DP side. Everything should be done with the plugin metadata's API on the CP side.

[rushitote]

@spacewander
Thanks for your review, so we will remove all the API functions and if the model/policy text in plugin metadata is updated (from the CP side), we will update the casbin_enforcer through that model/policy too. Is this correct?

[spacewander]

Yes, you can use metadata.modifiedIndex to ensure casbin_enforcer is created with the latest model/policy, like this one:

apisix/apisix/http/router/radixtree_uri.lua

Lines 31 to 36 in fd8f875

	if not cached_version or cached_version ~= user_routes.conf_version then
	uri_router = base_router.create_radixtree_uri_router(user_routes.values,
	uri_routes, false)
	cached_version = user_routes.conf_version
	end

[spacewander]

And I think you can submit a minimum viable pull request so that we can check & merge it early.

[rushitote]

Sure I will do that.

[rushitote]

I added a condition that if the configuration changes, the casbin_enforcer will be recreated.

[spacewander]

Multiple routes will use the same casbin enforcer? I think we should store different casbin via lrucache, like this:

apisix/apisix/plugins/error-log-logger.lua

Line 144 in 9db2dd2

config, err = lrucache(plugin_name, metadata.modifiedIndex, update_filter, metadata.value)

[rushitote]

Yes, you are right. I have changed it now.

[spacewander]

Sorry, I rethought about it afternoon, and found it isn't a good idea.
It is more suitable to bind the casbin with the conf like:

apisix/apisix/plugins/uri-blocker.lua

Line 83 in 38561dc

conf.block_rules_concat = core.table.concat(block_rules, "|")

We also need to ensure the global casbin is update-to-date with the metadata via #4710 (comment)

[rushitote]

@spacewander Sorry, I found this now. But is there any reason for us to not use lrucache? It worked well for two different routes with different casbin enforcers when I tested it.

[rushitote]

I have removed lrucache and replaced it by binding the casbin_enforcer to conf. Also we will use the modifiedIndex to keep the casbin from metadata updated.

[spacewander]

@spacewander Sorry, I found this now. But is there any reason for us to not use lrucache? It worked well for two different routes with different casbin enforcers when I tested it.

It works fine, but makes the life cycle complex.

[tokers]

We don't need to check the existence for these three variables if we use them in the HTTP sub-system.

[rushitote]

Yes, right - this is redundant here.

[tokers]

Please specify the precedence between this one and the path way.

[rushitote]

Sure, I will make it more clear there.

[spacewander]

Need a similar test for the policy_path configuration.

[rushitote]

Added the tests for enforcer from files.

[spacewander]

IMHO, it surprises me that the enforcer from metadata can override the one from the route. Normally, the route level configuration can override the global one.

[spacewander]

We don't need to bind the enforcer from metadata with the route configuration as it is global. Recreating the global enforcer when the request hits another route is not a good idea.

[rushitote]

@spacewander so, should we create a new casbin_enforcer local variable specifically for global config? Then we can also have (if any) route enforcer override the global one.

[spacewander]

@spacewander so, should we create a new casbin_enforcer local variable specifically for global config? Then we can also have (if any) route enforcer override the global one.

Do you mean a new global variable?

[rushitote]

@spacewander so, should we create a new casbin_enforcer local variable specifically for global config? Then we can also have (if any) route enforcer override the global one.

Do you mean a new global variable?

local, similar as that of the first commit (here)

[rushitote]

@spacewander I was thinking of something like this:

local casbin_enforcer

local function new_enforcer(conf, modifiedIndex)
    local model_path = conf.model_path
    local policy_path = conf.policy_path

    if model_path and policy_path then
        conf.type = "file"
        conf.casbin_enforcer = casbin:new(model_path, policy_path)
        return
    end

    local metadata = plugin.plugin_metadata(plugin_name)
    if metadata and metadata.value.model and metadata.value.policy then
        conf.type = "metadata"
        if not casbin_enforcer or casbin_enforcer.modifiedIndex ~= modifiedIndex then
            local model = metadata.value.model
            local policy = metadata.value.policy
            casbin_enforcer = casbin:newEnforcerFromText(model, policy)
            casbin_enforcer.modifiedIndex = modifiedIndex
        end
    end
end


function _M.rewrite(conf, ctx)
    -- creates an enforcer when request sent for the first time
    local metadata = plugin.plugin_metadata(plugin_name)

    if (not conf.casbin_enforcer and conf.type ~= "metadata") or
    (conf.type == "metadata" and casbin_enforcer.modifiedIndex ~= metadata.modifiedIndex) then
        new_enforcer(conf, metadata.modifiedIndex)
    end

    local path = ctx.var.uri
    local method = ctx.var.method
    local username = get_headers()[conf.username]
    if not username then username = "anonymous" end

    if conf.casbin_enforcer then
        if not conf.casbin_enforcer:enforce(username, path, method) then
            return 403, {message = "Access Denied"}
        end
    else
        if not casbin_enforcer:enforce(username, path, method) then
            return 403, {message = "Access Denied"}
        end
    end
end

So, we divide routes into two types - either file or metadata. The file one has their own enforcer binded to the conf while all the metadata based routes use the same casbin_enforcer. In case of the metadata based route, enforcer is created only when either there isn't one or when the modifiedIndex has changed. And since this separates any route into one of the two types, one type will not override the other. Will this solve the issue?

[spacewander]

LGTM. The only issue is that both new_enforcer and rewrite check if we need to create a new enforcer. I think we can just leave the check in one method.

[rushitote]

@spacewander Can you please tell me which check we could remove? I feel all are necessary and don't see any similar checks.

[spacewander]

What about:

local casbin_enforcer

local function new_enforcer_if_need(conf)
    local model_path = conf.model_path
    local policy_path = conf.policy_path

    if model_path and policy_path then
        if not conf.casbin_enforcer then
            conf.casbin_enforcer = casbin:new(model_path, policy_path)
        end
        return true
    end

    local metadata = plugin.plugin_metadata(plugin_name)
    if not (metadata and metadata.value.model and metadata.value.policy) then
        return nil, "not enough configuration to create enforcer"
    end

    local modifiedIndex = metadata.modifiedIndex
    if not casbin_enforcer or casbin_enforcer.modifiedIndex ~= modifiedIndex then
        local model = metadata.value.model
        local policy = metadata.value.policy
        casbin_enforcer = casbin:newEnforcerFromText(model, policy)
        casbin_enforcer.modifiedIndex = modifiedIndex
    end
    return true
end


function _M.rewrite(conf, ctx)
    -- creates an enforcer when request sent for the first time
    local ok, err = new_enforcer_if_need(conf)
    if not ok then
        return 503, {message = err}
    end

    local path = ctx.var.uri
    local method = ctx.var.method
    local username = get_headers()[conf.username]
    if not username then username = "anonymous" end

    if conf.casbin_enforcer then
        if not conf.casbin_enforcer:enforce(username, path, method) then
            return 403, {message = "Access Denied"}
        end
    else
        if not casbin_enforcer:enforce(username, path, method) then
            return 403, {message = "Access Denied"}
        end
    end
end

[rushitote]

I think this is good, I have added a commit with this.

[spacewander]

To make the misc checker pass, you need to update

apisix/ci/ASF-Release.cfg

Line 59 in 9f01ef8

[Excludes]

To make the doc linter pass, you need to update https://github.com/apache/apisix/blob/master/docs/en/latest/config.json

[rushitote]

To make the misc checker pass, you need to update

apisix/ci/ASF-Release.cfg

Line 59 in 9f01ef8

[Excludes]

To make the doc linter pass, you need to update https://github.com/apache/apisix/blob/master/docs/en/latest/config.json

Thanks, changed it.

[tzssangglass]

Suggested change

	"uri": "/*"
	"uri": "/hello"

[tzssangglass]

ditto

[rushitote]

Changed it.