You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/09/02 07:04:36 UTC
[GitHub] [apisix] spacewander commented on a diff in pull request #7827: feat(saml-auth): add saml-auth plugin
spacewander commented on code in PR #7827:
URL: https://github.com/apache/apisix/pull/7827#discussion_r961353226
##########
apisix/plugins/saml-auth.lua:
##########
@@ -0,0 +1,92 @@
+--
+-- 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 core = require("apisix.core")
+local resty_saml = require("resty.saml")
+
+local is_resty_saml_init = false
+
+local lrucache = core.lrucache.new({
+ ttl = 300, count = 512
+})
+
+local schema = {
+ type = "object",
+ title = "work with route or service object",
+ properties = {
+ sp_issuer = { type = "string" },
+ idp_uri = { type = "string" },
+ idp_cert = { type = "string" },
+ login_callback_uri = { type = "string" },
+ logout_uri = { type = "string" },
+ logout_callback_uri = { type = "string" },
+ logout_redirect_uri = { type = "string" },
+ sp_cert = { type = "string" },
+ sp_private_key = { type = "string" },
+ },
+ required = {
+ "sp_issuer",
+ "idp_uri",
+ "idp_cert",
+ "login_callback_uri",
+ "logout_uri",
+ "logout_callback_uri",
+ "logout_redirect_uri",
+ "sp_cert",
+ "sp_private_key",
+ }
+}
+
+local plugin_name = "saml-auth"
+
+local _M = {
+ version = 0.1,
+ priority = 2598,
+ name = plugin_name,
+ schema = schema,
+}
+
+function _M.check_schema(conf, schema_type)
+ return core.schema.check(schema, conf)
+end
+
+local function create_saml_obj(conf)
+ return resty_saml.new(conf)
+end
+
+function _M.rewrite(conf, ctx)
+ if not is_resty_saml_init then
+ local err = resty_saml.init({
+ debug = true,
+ data_dir = ctx.var.saml_data_dir,
+ })
+ if err then
+ core.log.error("saml init: ", err)
+ return 500
+ end
+ is_resty_saml_init = true
+ end
+
+ core.log.info("plugin rewrite phase, conf: ", core.json.delay_encode(conf))
+
+ local saml = core.lrucache.plugin_ctx(lrucache, ctx, nil, create_saml_obj, conf)
+
+ local data = saml:authenticate()
Review Comment:
It's weird to let a plugin hijack the request flow when the authentication is failed. It would bypass APISIX's execution code. lua-resty-openidc does this at some corners, but since lua-resty-saml is owned by us, we can do better.
Maybe we can do it in the future PR?
##########
apisix/plugins/saml-auth.lua:
##########
@@ -0,0 +1,92 @@
+--
+-- 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 core = require("apisix.core")
+local resty_saml = require("resty.saml")
+
+local is_resty_saml_init = false
+
+local lrucache = core.lrucache.new({
+ ttl = 300, count = 512
+})
+
+local schema = {
+ type = "object",
+ title = "work with route or service object",
+ properties = {
+ sp_issuer = { type = "string" },
+ idp_uri = { type = "string" },
+ idp_cert = { type = "string" },
+ login_callback_uri = { type = "string" },
+ logout_uri = { type = "string" },
+ logout_callback_uri = { type = "string" },
+ logout_redirect_uri = { type = "string" },
+ sp_cert = { type = "string" },
+ sp_private_key = { type = "string" },
+ },
+ required = {
+ "sp_issuer",
+ "idp_uri",
+ "idp_cert",
+ "login_callback_uri",
+ "logout_uri",
+ "logout_callback_uri",
+ "logout_redirect_uri",
+ "sp_cert",
+ "sp_private_key",
+ }
+}
+
+local plugin_name = "saml-auth"
+
+local _M = {
+ version = 0.1,
+ priority = 2598,
+ name = plugin_name,
+ schema = schema,
+}
+
+function _M.check_schema(conf, schema_type)
+ return core.schema.check(schema, conf)
+end
+
+local function create_saml_obj(conf)
+ return resty_saml.new(conf)
+end
+
+function _M.rewrite(conf, ctx)
+ if not is_resty_saml_init then
+ local err = resty_saml.init({
+ debug = true,
+ data_dir = ctx.var.saml_data_dir,
+ })
+ if err then
+ core.log.error("saml init: ", err)
+ return 500
Review Comment:
Better avoid using 500. Maybe we can use 503. People keep reporting issues when they get 500 from APISIX, even it is caused by their environment problem.
##########
ci/pod/docker-compose.plugin.yml:
##########
@@ -42,6 +42,19 @@ services:
networks:
apisix_net:
+ ## keycloak
+ apisix_keycloak_for_saml:
+ container_name: keycloak
+ image: quay.io/keycloak/keycloak:18.0.2
Review Comment:
Let's add a comment on why we choose a separate keycloak
##########
t/plugin/saml-auth.t:
##########
@@ -0,0 +1,219 @@
+#
+# 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.
+#
+use t::APISIX 'no_plan';
+
+log_level('warn');
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+run_tests;
+
+__DATA__
+
+=== TEST 1: Add route for sp1
+--- config
+ location /t {
+ content_by_lua_block {
+ local kc = require("lib.keycloak2")
+ local core = require("apisix.core")
+
+ local default_opts = kc.get_default_opts()
+ local opts = core.table.deepcopy(default_opts)
+ opts.sp_issuer = "sp"
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "host" : "127.0.0.1",
+ "plugins": {
+ "saml-auth": ]] .. core.json.encode(opts) .. [[
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 2: login and logout ok
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local httpc = http.new()
+ local kc = require "lib.keycloak2"
+
+ local path = "/uri"
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port
+ local username = "test"
+ local password = "test"
+
+ local res, err, saml_cookie, keycloak_cookie = kc.login_keycloak(uri .. path, username, password)
+ if err or res.headers['Location'] ~= path then
+ ngx.log(ngx.ERR, err)
+ ngx.exit(500)
+ end
+ res, err = httpc:request_uri(uri .. res.headers['Location'], {
+ method = "GET",
+ headers = {
+ ["Cookie"] = saml_cookie
+ }
+ })
+ assert(res.status == 200)
+ ngx.say(res.body)
+
+ res, err = kc.logout_keycloak(uri .. "/logout", saml_cookie, keycloak_cookie)
+ if err or res.headers['Location'] ~= "/logout_ok" then
+ ngx.log(ngx.ERR, err)
+ ngx.exit(500)
+ end
+ }
+ }
+--- request
+GET /t
+--- response_body_like
+uri: /uri
+cookie: .*
+host: 127.0.0.1:1984
+user-agent: .*
+x-real-ip: 127.0.0.1
+--- no_error_log
+[error]
+
+
+
+=== TEST 3: Add route for sp2
+--- config
+ location /t {
+ content_by_lua_block {
+ local kc = require("lib.keycloak2")
+ local core = require("apisix.core")
+
+ local default_opts = kc.get_default_opts()
+ local opts = core.table.deepcopy(default_opts)
+ opts.sp_issuer = "sp2"
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "host" : "127.0.0.2",
+ "plugins": {
+ "saml-auth": ]] .. core.json.encode(opts) .. [[
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 4: login sp1 and sp2, then do single logout
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local httpc = http.new()
+ local kc = require "lib.keycloak2"
+
+ local path = "/uri"
+
+ -- login to sp1
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port
+ local username = "test"
+ local password = "test"
+
+ local res, err, saml_cookie, keycloak_cookie = kc.login_keycloak(uri .. path, username, password)
+ if err or res.headers['Location'] ~= path then
+ ngx.log(ngx.ERR, err)
+ ngx.exit(500)
+ end
+ res, err = httpc:request_uri(uri .. res.headers['Location'], {
+ method = "GET",
+ headers = {
+ ["Cookie"] = saml_cookie
+ }
+ })
+ assert(res.status == 200)
+
+ -- login to sp2
+ local uri2 = "http://127.0.0.2:" .. ngx.var.server_port
+
+ local res, err, saml_cookie2 = kc.login_keycloak_for_second_sp(uri2 .. path, keycloak_cookie)
+ if err or res.headers['Location'] ~= path then
+ ngx.log(ngx.ERR, err)
+ ngx.exit(500)
+ end
+ res, err = httpc:request_uri(uri2 .. res.headers['Location'], {
+ method = "GET",
+ headers = {
+ ["Cookie"] = saml_cookie2
+ }
+ })
+ assert(res.status == 200)
+
+ -- SLO (single logout)
+ res, err = kc.single_logout(uri .. "/logout", saml_cookie, saml_cookie2, keycloak_cookie)
+ if err or res.headers['Location'] ~= "/logout_ok" then
+ ngx.log(ngx.ERR, err)
+ ngx.exit(500)
+ end
+ }
+ }
+--- request
+GET /t
+--- error_code: 200
Review Comment:
error_code 200 is checked by default
##########
apisix/plugins/saml-auth.lua:
##########
@@ -0,0 +1,92 @@
+--
+-- 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 core = require("apisix.core")
+local resty_saml = require("resty.saml")
+
+local is_resty_saml_init = false
+
+local lrucache = core.lrucache.new({
+ ttl = 300, count = 512
+})
+
+local schema = {
+ type = "object",
+ title = "work with route or service object",
Review Comment:
We don't need this title
##########
t/plugin/saml-auth.t:
##########
@@ -0,0 +1,219 @@
+#
+# 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.
+#
+use t::APISIX 'no_plan';
+
+log_level('warn');
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+run_tests;
+
+__DATA__
+
+=== TEST 1: Add route for sp1
+--- config
+ location /t {
+ content_by_lua_block {
+ local kc = require("lib.keycloak2")
+ local core = require("apisix.core")
+
+ local default_opts = kc.get_default_opts()
+ local opts = core.table.deepcopy(default_opts)
+ opts.sp_issuer = "sp"
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "host" : "127.0.0.1",
+ "plugins": {
+ "saml-auth": ]] .. core.json.encode(opts) .. [[
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
Review Comment:
Let's exact the common part like other plugin test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org