diff --git a/README.md b/README.md
index 6a14bb508..68ee2f101 100755
--- a/README.md
+++ b/README.md
@@ -159,6 +159,7 @@ For previous versions, please read:
## V3 changes
+* v3.0, 2020-02-04, For [#1186][bug #1186], refactor security check. 3.0.114
* v3.0, 2020-02-04, Fix [#939][bug #939], response right A/V flag in FLV header. 3.0.113
* v3.0, 2020-02-04, For [#939][bug #939], always enable fast FLV streaming.
* v3.0, 2020-02-02, [3.0 beta0(3.0.112)][r3.0b0] released. 121709 lines.
@@ -1662,6 +1663,7 @@ Winlin
[bug #1230]: https://github.com/ossrs/srs/issues/1230
[bug #1206]: https://github.com/ossrs/srs/issues/1206
[bug #939]: https://github.com/ossrs/srs/issues/939
+[bug #1186]: https://github.com/ossrs/srs/issues/1186
[bug #xxxxxxxxxxxxx]: https://github.com/ossrs/srs/issues/xxxxxxxxxxxxx
[exo #828]: https://github.com/google/ExoPlayer/pull/828
diff --git a/trunk/src/app/srs_app_config.cpp b/trunk/src/app/srs_app_config.cpp
index 04c1f2032..1b494b149 100644
--- a/trunk/src/app/srs_app_config.cpp
+++ b/trunk/src/app/srs_app_config.cpp
@@ -735,13 +735,28 @@ SrsConfDirective* SrsConfDirective::get_or_create(string n, string a0)
if (!conf) {
conf = new SrsConfDirective();
conf->name = n;
- conf->set_arg0(a0);
+ conf->args.push_back(a0);
directives.push_back(conf);
}
return conf;
}
+SrsConfDirective* SrsConfDirective::get_or_create(string n, string a0, string a1)
+{
+ SrsConfDirective* conf = get(n, a0);
+
+ if (!conf) {
+ conf = new SrsConfDirective();
+ conf->name = n;
+ conf->args.push_back(a0);
+ conf->args.push_back(a1);
+ directives.push_back(conf);
+ }
+
+ return conf;
+}
+
SrsConfDirective* SrsConfDirective::set_arg0(string a0)
{
if (arg0() == a0) {
diff --git a/trunk/src/app/srs_app_config.hpp b/trunk/src/app/srs_app_config.hpp
index cc936affc..a95a07830 100644
--- a/trunk/src/app/srs_app_config.hpp
+++ b/trunk/src/app/srs_app_config.hpp
@@ -215,6 +215,7 @@ public:
public:
virtual SrsConfDirective* get_or_create(std::string n);
virtual SrsConfDirective* get_or_create(std::string n, std::string a0);
+ virtual SrsConfDirective* get_or_create(std::string n, std::string a0, std::string a1);
virtual SrsConfDirective* set_arg0(std::string a0);
// Remove the v from sub directives, user must free the v.
virtual void remove(SrsConfDirective* v);
diff --git a/trunk/src/app/srs_app_security.cpp b/trunk/src/app/srs_app_security.cpp
index 06e886fc7..812766f08 100644
--- a/trunk/src/app/srs_app_security.cpp
+++ b/trunk/src/app/srs_app_security.cpp
@@ -39,54 +39,61 @@ SrsSecurity::~SrsSecurity()
srs_error_t SrsSecurity::check(SrsRtmpConnType type, string ip, SrsRequest* req)
{
srs_error_t err = srs_success;
-
+
// allow all if security disabled.
if (!_srs_config->get_security_enabled(req->vhost)) {
- return err;
+ return err; // OK
}
-
- // default to deny all when security enabled.
- err = srs_error_new(ERROR_SYSTEM_SECURITY, "allowed");
-
+
// rules to apply
SrsConfDirective* rules = _srs_config->get_security_rules(req->vhost);
+ return do_check(rules, type, ip, req);
+}
+
+srs_error_t SrsSecurity::do_check(SrsConfDirective* rules, SrsRtmpConnType type, string ip, SrsRequest* req)
+{
+ srs_error_t err = srs_success;
+
if (!rules) {
- return err;
+ return srs_error_new(ERROR_SYSTEM_SECURITY, "default deny for %s", ip.c_str());
+ }
+
+ // deny if matches deny strategy.
+ if ((err = deny_check(rules, type, ip)) != srs_success) {
+ return srs_error_wrap(err, "for %s", ip.c_str());
}
// allow if matches allow strategy.
- if (allow_check(rules, type, ip) == ERROR_SYSTEM_SECURITY_ALLOW) {
- srs_error_reset(err);
+ if ((err = allow_check(rules, type, ip)) != srs_success) {
+ return srs_error_wrap(err, "for %s", ip.c_str());
}
-
- // deny if matches deny strategy.
- if (deny_check(rules, type, ip) == ERROR_SYSTEM_SECURITY_DENY) {
- srs_error_reset(err);
- return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "denied");
- }
-
+
return err;
}
-int SrsSecurity::allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip)
+srs_error_t SrsSecurity::allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip)
{
- int ret = ERROR_SUCCESS;
-
+ int allow_rules = 0;
+ int deny_rules = 0;
+
for (int i = 0; i < (int)rules->directives.size(); i++) {
SrsConfDirective* rule = rules->at(i);
-
+
if (rule->name != "allow") {
+ if (rule->name == "deny") {
+ deny_rules++;
+ }
continue;
}
-
+ allow_rules++;
+
switch (type) {
case SrsRtmpConnPlay:
if (rule->arg0() != "play") {
break;
}
if (rule->arg1() == "all" || rule->arg1() == ip) {
- ret = ERROR_SYSTEM_SECURITY_ALLOW;
- break;
+ return srs_success; // OK
}
break;
case SrsRtmpConnFMLEPublish:
@@ -96,28 +103,23 @@ int SrsSecurity::allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std:
break;
}
if (rule->arg1() == "all" || rule->arg1() == ip) {
- ret = ERROR_SYSTEM_SECURITY_ALLOW;
- break;
+ return srs_success; // OK
}
break;
case SrsRtmpConnUnknown:
default:
break;
}
-
- // when matched, donot search more.
- if (ret == ERROR_SYSTEM_SECURITY_ALLOW) {
- break;
- }
}
-
- return ret;
+
+ if (allow_rules > 0 || (deny_rules + allow_rules) == 0) {
+ return srs_error_new(ERROR_SYSTEM_SECURITY_ALLOW, "not allowed by any of %d/%d rules", allow_rules, deny_rules);
+ }
+ return srs_success; // OK
}
-int SrsSecurity::deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip)
+srs_error_t SrsSecurity::deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip)
{
- int ret = ERROR_SUCCESS;
-
for (int i = 0; i < (int)rules->directives.size(); i++) {
SrsConfDirective* rule = rules->at(i);
@@ -131,8 +133,7 @@ int SrsSecurity::deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::
break;
}
if (rule->arg1() == "all" || rule->arg1() == ip) {
- ret = ERROR_SYSTEM_SECURITY_DENY;
- break;
+ return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "deny by rule<%s>", rule->arg1().c_str());
}
break;
case SrsRtmpConnFMLEPublish:
@@ -142,21 +143,15 @@ int SrsSecurity::deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::
break;
}
if (rule->arg1() == "all" || rule->arg1() == ip) {
- ret = ERROR_SYSTEM_SECURITY_DENY;
- break;
+ return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "deny by rule<%s>", rule->arg1().c_str());
}
break;
case SrsRtmpConnUnknown:
default:
break;
}
-
- // when matched, donot search more.
- if (ret == ERROR_SYSTEM_SECURITY_DENY) {
- break;
- }
}
- return ret;
+ return srs_success; // OK
}
diff --git a/trunk/src/app/srs_app_security.hpp b/trunk/src/app/srs_app_security.hpp
index e336f1a90..163b7d3a6 100644
--- a/trunk/src/app/srs_app_security.hpp
+++ b/trunk/src/app/srs_app_security.hpp
@@ -46,12 +46,9 @@ public:
// @param req the request object of client.
virtual srs_error_t check(SrsRtmpConnType type, std::string ip, SrsRequest* req);
private:
- // Security check the allow,
- // @return, if allowed, ERROR_SYSTEM_SECURITY_ALLOW.
- virtual int allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip);
- // Security check the deny,
- // @return, if denied, ERROR_SYSTEM_SECURITY_DENY.
- virtual int deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip);
+ virtual srs_error_t do_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip, SrsRequest* req);
+ virtual srs_error_t allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip);
+ virtual srs_error_t deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip);
};
#endif
diff --git a/trunk/src/core/srs_core_version3.hpp b/trunk/src/core/srs_core_version3.hpp
index 0977c5458..fe5e458d1 100644
--- a/trunk/src/core/srs_core_version3.hpp
+++ b/trunk/src/core/srs_core_version3.hpp
@@ -24,6 +24,6 @@
#ifndef SRS_CORE_VERSION3_HPP
#define SRS_CORE_VERSION3_HPP
-#define SRS_VERSION3_REVISION 113
+#define SRS_VERSION3_REVISION 114
#endif
diff --git a/trunk/src/utest/srs_utest_app.cpp b/trunk/src/utest/srs_utest_app.cpp
index 5d158d5d6..98c34a29a 100644
--- a/trunk/src/utest/srs_utest_app.cpp
+++ b/trunk/src/utest/srs_utest_app.cpp
@@ -26,6 +26,8 @@ using namespace std;
#include
#include
+#include
+#include
#include
@@ -372,3 +374,186 @@ VOID TEST(AppFragmentTest, CheckDuration)
}
}
+VOID TEST(AppSecurity, CheckSecurity)
+{
+ srs_error_t err;
+
+ // Deny if no rules.
+ if (true) {
+ SrsSecurity sec; SrsRequest rr;
+ HELPER_EXPECT_FAILED(sec.do_check(NULL, SrsRtmpConnUnknown, "", &rr));
+ }
+
+ // Deny if not allowed.
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnUnknown, "", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("others"); rules.get_or_create("any");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnUnknown, "", &rr));
+ }
+
+ // Deny by rule.
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "play", "all");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "play", "12.13.14.15");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "play", "11.12.13.14");
+ if (true) {
+ SrsConfDirective* d = new SrsConfDirective();
+ d->name = "deny";
+ d->args.push_back("play");
+ d->args.push_back("12.13.14.15");
+ rules.directives.push_back(d);
+ }
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "publish", "12.13.14.15");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "publish", "12.13.14.15");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnFlashPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "publish", "all");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnFlashPublish, "11.12.13.14", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "publish", "12.13.14.15");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnHaivisionPublish, "12.13.14.15", &rr));
+ }
+
+ // Allowed if not denied.
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "play", "all");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "play", "12.13.14.15");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "play", "12.13.14.15");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "11.12.13.14", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "publish", "12.13.14.15");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "publish", "12.13.14.15");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnUnknown, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "publish", "12.13.14.15");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFlashPublish, "11.12.13.14", &rr));
+ }
+
+ // Allowed by rule.
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "play", "12.13.14.15");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "play", "all");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "publish", "all");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "publish", "all");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFlashPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "publish", "all");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnHaivisionPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "publish", "12.13.14.15");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnHaivisionPublish, "12.13.14.15", &rr));
+ }
+
+ // Allowed if not denied.
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "play", "12.13.14.15");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("deny", "play", "all");
+ HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr));
+ }
+
+ // Denied if not allowd.
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "play", "11.12.13.14");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "play", "11.12.13.14");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "publish", "12.13.14.15");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "publish", "11.12.13.14");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnHaivisionPublish, "12.13.14.15", &rr));
+ }
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "publish", "11.12.13.14");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnUnknown, "11.12.13.14", &rr));
+ }
+
+ // Denied if dup.
+ if (true) {
+ SrsSecurity sec; SrsRequest rr; SrsConfDirective rules;
+ rules.get_or_create("allow", "play", "11.12.13.14");
+ rules.get_or_create("deny", "play", "11.12.13.14");
+ HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "11.12.13.14", &rr));
+ }
+
+ // SRS apply the following simple strategies one by one:
+ // 1. allow all if security disabled.
+ // 2. default to deny all when security enabled.
+ // 3. allow if matches allow strategy.
+ // 4. deny if matches deny strategy.
+}
+