Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 16 commits into from
Aug 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cb37e5f
feat: Added authz-casbin plugin and doc and tests for it
rushitote Jul 28, 2021
56668c0
fix: remove API and fix tests
rushitote Jul 30, 2021
38a1ff1
fix: lint and chaos test
rushitote Aug 1, 2021
67a433e
fix: using authz-casbin on multiple routes
rushitote Aug 1, 2021
2eb4cb5
fix: tests and added test for using plugin with files
rushitote Aug 1, 2021
22f76c5
fix: redundant metadata check
rushitote Aug 1, 2021
2fa5b82
fix: removed lrucache for plugin and fix docs
rushitote Aug 2, 2021
0aa4518
fix: lint and doc lint tests
rushitote Aug 2, 2021
b95d9af
fix: change ngx to ctx
rushitote Aug 2, 2021
bc7981f
fix: plugin behaviour while using metdata
rushitote Aug 3, 2021
c15ecf9
Update apisix/plugins/authz-casbin.lua
rushitote Aug 3, 2021
8824fc9
Merge branch 'master' into master
rushitote Aug 3, 2021
e0858c8
fix: Change plugin uri in test
rushitote Aug 3, 2021
842f9d1
feat: added model/policy text support in plugin config
rushitote Aug 4, 2021
a32d79e
fix: use oneOf in schema instead of anyOf
rushitote Aug 4, 2021
ba0b0c6
docs: specify route/metadata precendence in plugin
rushitote Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ jobs:
tar zxvf ${{ steps.branch_env.outputs.fullname }}

- name: Linux Get dependencies
run: sudo apt install -y cpanminus build-essential libncurses5-dev libreadline-dev libssl-dev perl
run: sudo apt install -y cpanminus build-essential libncurses5-dev libreadline-dev libssl-dev perl libpcre3 libpcre3-dev
Yiyiyimu marked this conversation as resolved.
Show resolved Hide resolved

- name: Linux Before install
run: sudo ./ci/${{ matrix.os_name }}_runner.sh before_install
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/chaos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ jobs:
bash ./t/chaos/utils/setup_chaos_utils.sh start_minikube
wget https://raw.githubusercontent.com/apache/apisix-docker/master/alpine-local/Dockerfile
mkdir logs
sudo apt install -y libpcre3 libpcre3-dev
docker build -t apache/apisix:alpine-local --build-arg APISIX_PATH=. -f Dockerfile .
minikube cache add apache/apisix:alpine-local -v 7 --alsologtostderr

Expand Down
115 changes: 115 additions & 0 deletions apisix/plugins/authz-casbin.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
--
-- Licensed to the Apache Software Foundation (ASF) under one or more
-- contributor license agreements. See the NOTICE file distributed with
-- this work for additional information regarding copyright ownership.
-- The ASF licenses this file to You under the Apache License, Version 2.0
-- (the "License"); you may not use this file except in compliance with
-- the License. You may obtain a copy of the License at
--
-- http://www.apache.org/licenses/LICENSE-2.0
--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
--

local casbin = require("casbin")
local core = require("apisix.core")
local plugin = require("apisix.plugin")
local ngx = ngx
local get_headers = ngx.req.get_headers

local plugin_name = "authz-casbin"

local schema = {
type = "object",
properties = {
model_path = { type = "string" },
policy_path = { type = "string" },
username = { type = "string"}
},
required = {"model_path", "policy_path", "username"},
additionalProperties = false
}

local metadata_schema = {
type = "object",
properties = {
model = {type = "string"},
policy = {type = "string"},
},
required = {"model", "policy"},
additionalProperties = false
}

local _M = {
version = 0.1,
priority = 2560,
name = plugin_name,
schema = schema,
metadata_schema = metadata_schema
}

function _M.check_schema(conf, schema_type)
if schema_type == core.schema.TYPE_METADATA then
return core.schema.check(metadata_schema, conf)
end
local ok, err = core.schema.check(schema, conf)
if ok then
return true
else
local metadata = plugin.plugin_metadata(plugin_name)
if metadata and metadata.value and conf.username then
tokers marked this conversation as resolved.
Show resolved Hide resolved
return true
end
end
return false, err
end


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

local e

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

local metadata = plugin.plugin_metadata(plugin_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if metadata and metadata.value.model and metadata.value.policy and not e then
local model = metadata.value.model
local policy = metadata.value.policy
e = casbin:newEnforcerFromText(model, policy)
conf.type = "metadata"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

conf.modifiedIndex = modifiedIndex
end

conf.casbin_enforcer = e
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) or
(conf.type == "metadata" and conf.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]
rushitote marked this conversation as resolved.
Show resolved Hide resolved
if not username then username = "anonymous" end

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


return _M
3 changes: 3 additions & 0 deletions ci/ASF-Release.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ t/toolkit
# Exclude subcomponents files
apisix/balancer/ewma.lua

# Exclude plugin-specific configuration files
t/plugin/authz-casbin

[Options]
# Not all code files allow licenses to appear starting at the first character
# of the file. This option tells the scan to allow licenses to appear starting
Expand Down
2 changes: 1 addition & 1 deletion ci/centos7-ci.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ install_dependencies() {

# install openresty to make apisix's rpm test work
yum install -y yum-utils && yum-config-manager --add-repo https://openresty.org/package/centos/openresty.repo
yum install -y openresty openresty-debug openresty-openssl111-debug-devel
yum install -y openresty openresty-debug openresty-openssl111-debug-devel pcre pcre-devel

# install luarocks
./utils/linux-install-luarocks.sh
Expand Down
1 change: 1 addition & 0 deletions conf/config-default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ plugins: # plugin list (sorted by priority)
- uri-blocker # priority: 2900
- request-validation # priority: 2800
- openid-connect # priority: 2599
- authz-casbin # priority: 2560
- wolf-rbac # priority: 2555
- hmac-auth # priority: 2530
- basic-auth # priority: 2520
Expand Down
3 changes: 2 additions & 1 deletion docs/en/latest/config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@
"plugins/authz-keycloak",
"plugins/wolf-rbac",
"plugins/openid-connect",
"plugins/hmac-auth"
"plugins/hmac-auth",
"plugins/authz-casbin"
]
},
{
Expand Down
10 changes: 5 additions & 5 deletions docs/en/latest/install-dependencies.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ sudo yum install yum-utils
sudo yum-config-manager --add-repo https://openresty.org/package/centos/openresty.repo

# install OpenResty and some compilation tools
sudo yum install -y openresty curl git gcc openresty-openssl111-devel unzip
sudo yum install -y openresty curl git gcc openresty-openssl111-devel unzip pcre pcre-devel

# install LuaRocks
curl https://raw.githubusercontent.com/apache/apisix/master/utils/linux-install-luarocks.sh -sL | bash -
Expand All @@ -81,7 +81,7 @@ tar -xvf etcd-v3.4.13-linux-amd64.tar.gz && \
sudo cp -a etcd etcdctl /usr/bin/

# install OpenResty and some compilation tools
sudo yum install -y openresty curl git gcc openresty-openssl111-devel
sudo yum install -y openresty curl git gcc openresty-openssl111-devel pcre pcre-devel

# install LuaRocks
curl https://raw.githubusercontent.com/apache/apisix/master/utils/linux-install-luarocks.sh -sL | bash -
Expand All @@ -107,7 +107,7 @@ tar -xvf etcd-v3.4.13-linux-amd64.tar.gz && \
sudo cp -a etcd etcdctl /usr/bin/

# install OpenResty and some compilation tools
sudo apt-get install -y git openresty curl openresty-openssl111-dev make gcc
sudo apt-get install -y git openresty curl openresty-openssl111-dev make gcc libpcre3 libpcre3-dev

# install LuaRocks
curl https://raw.githubusercontent.com/apache/apisix/master/utils/linux-install-luarocks.sh -sL | bash -
Expand Down Expand Up @@ -138,7 +138,7 @@ tar -xvf etcd-v3.4.13-linux-amd64.tar.gz && \
sudo cp -a etcd etcdctl /usr/bin/

# install OpenResty and some compilation tools
sudo apt-get install -y git openresty curl make openresty-openssl111-dev
sudo apt-get install -y git openresty curl make openresty-openssl111-dev libpcre3 libpcre3-dev

# install LuaRocks
curl https://raw.githubusercontent.com/apache/apisix/master/utils/linux-install-luarocks.sh -sL | bash -
Expand All @@ -151,7 +151,7 @@ nohup etcd &

```shell
# install OpenResty, etcd and some compilation tools
brew install openresty/brew/openresty luarocks lua@5.1 etcd curl git
brew install openresty/brew/openresty luarocks lua@5.1 etcd curl git pcre

# start etcd server
brew services start etcd
Expand Down
Loading