Skip to content

Commit 6ebe882

Browse files
committed
Fixing remap acl rules for @action=allow
If a user specifies an @action=allow remap.config ACL rule, then the implication is that requests with methods not in the allow list would be denied. Before this patch, allow ACL rules would just never deny. This fixes that behavior so that allow ACL rules that match on IP but not on method deny.
1 parent e935f3e commit 6ebe882

File tree

10 files changed

+601
-37
lines changed

10 files changed

+601
-37
lines changed

include/proxy/http/HttpTransact.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -682,7 +682,7 @@ class HttpTransact
682682
bool is_upgrade_request = false;
683683
bool is_websocket = false;
684684
bool did_upgrade_succeed = false;
685-
bool client_connection_enabled = true;
685+
bool client_connection_allowed = true;
686686
bool acl_filtering_performed = false;
687687
bool api_cleanup_cache_read = false;
688688
bool api_server_response_no_store = false;

include/proxy/http/remap/AclFiltering.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,10 @@ class acl_filter_rule
7070
acl_filter_rule *next = nullptr;
7171
char *filter_name = nullptr; // optional filter name
7272
unsigned int allow_flag : 1, // action allow deny
73-
src_ip_valid : 1, // src_ip range valid
74-
in_ip_valid : 1,
75-
active_queue_flag : 1, // filter is in active state (used by .useflt directive)
76-
internal : 1; // filter internal HTTP requests
73+
src_ip_valid : 1, // src_ip (client's src IP) range is specified and valid
74+
in_ip_valid : 1, // in_ip (client's dest IP) range is specified and valid
75+
active_queue_flag : 1, // filter is in active state (used by .useflt directive)
76+
internal : 1; // filter internal HTTP requests
7777

7878
// we need arguments as string array for directive processing
7979
int argc = 0; // argument counter (only for filter defs)

include/proxy/http/remap/UrlMapping.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ class url_mapping
116116
referer_info *referer_list = nullptr;
117117
redirect_tag_str *redir_chunk_list = nullptr;
118118
bool ip_allow_check_enabled_p = false;
119-
acl_filter_rule *filter = nullptr; // acl filtering (list of rules)
119+
acl_filter_rule *filter = nullptr; // acl filtering (linked list of rules)
120120
LINK(url_mapping, link); // For use with the main Queue linked list holding all the mapping
121121
std::shared_ptr<NextHopSelectionStrategy> strategy = nullptr;
122122
std::string remapKey;

src/proxy/http/HttpSM.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4301,7 +4301,7 @@ HttpSM::check_sni_host()
43014301
swoc::bwprint(error_bw_buffer, "No SNI for TLS request: connecting to {} for host='{}', returning a 403",
43024302
t_state.client_info.dst_addr, std::string_view{host_name, static_cast<size_t>(host_len)});
43034303
Log::error("%s", error_bw_buffer.c_str());
4304-
this->t_state.client_connection_enabled = false;
4304+
this->t_state.client_connection_allowed = false;
43054305
}
43064306
} else if (strncasecmp(host_name, sni_value, host_len) != 0) { // Name mismatch
43074307
Warning("SNI/hostname mismatch sni=%s host=%.*s action=%s", sni_value, host_len, host_name, action_value);
@@ -4310,7 +4310,7 @@ HttpSM::check_sni_host()
43104310
swoc::bwprint(error_bw_buffer, "SNI/hostname mismatch: connecting to {} for host='{}' sni='{}', returning a 403",
43114311
t_state.client_info.dst_addr, std::string_view{host_name, static_cast<size_t>(host_len)}, sni_value);
43124312
Log::error("%s", error_bw_buffer.c_str());
4313-
this->t_state.client_connection_enabled = false;
4313+
this->t_state.client_connection_allowed = false;
43144314
}
43154315
} else {
43164316
SMDbg(dbg_ctl_ssl_sni, "SNI/hostname successfully match sni=%s host=%.*s", sni_value, host_len, host_name);

src/proxy/http/HttpTransact.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1106,7 +1106,7 @@ HttpTransact::EndRemapRequest(State *s)
11061106
/////////////////////////////////////////////////////////////////////////
11071107
// We must close this connection if client_connection_enabled == false //
11081108
/////////////////////////////////////////////////////////////////////////
1109-
if (!s->client_connection_enabled) {
1109+
if (!s->client_connection_allowed) {
11101110
build_error_response(s, HTTP_STATUS_FORBIDDEN, "Access Denied", "access#denied");
11111111
s->reverse_proxy = false;
11121112
goto done;
@@ -6491,7 +6491,7 @@ void
64916491
HttpTransact::process_quick_http_filter(State *s, int method)
64926492
{
64936493
// connection already disabled by previous ACL filtering, don't modify it.
6494-
if (!s->client_connection_enabled) {
6494+
if (!s->client_connection_allowed) {
64956495
return;
64966496
}
64976497

@@ -6527,7 +6527,7 @@ HttpTransact::process_quick_http_filter(State *s, int method)
65276527
TxnDbg(dbg_ctl_ip_allow, "Line %d denial for '%.*s' from %s", acl.source_line(), method_str_len, method_str,
65286528
ats_ip_ntop(&s->client_info.src_addr.sa, ipb, sizeof(ipb)));
65296529
}
6530-
s->client_connection_enabled = false;
6530+
s->client_connection_allowed = false;
65316531
}
65326532
}
65336533
}

src/proxy/http/remap/UrlRewrite.cc

Lines changed: 41 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,7 @@ UrlRewrite::ReverseMap(HTTPHdr *response_header)
399399
void
400400
UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map)
401401
{
402-
if (unlikely(!s || s->acl_filtering_performed || !s->client_connection_enabled)) {
402+
if (unlikely(!s || s->acl_filtering_performed || !s->client_connection_allowed)) {
403403
return;
404404
}
405405

@@ -411,43 +411,50 @@ UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map)
411411

412412
ink_release_assert(ats_is_ip(&s->client_info.src_addr));
413413

414-
s->client_connection_enabled = true; // Default is that we allow things unless some filter matches
414+
s->client_connection_allowed = true; // Default is that we allow things unless some filter matches
415415

416-
for (acl_filter_rule *rp = map->filter; rp; rp = rp->next) {
417-
bool match = true;
416+
int rule_index = 0;
417+
for (acl_filter_rule *rp = map->filter; rp; rp = rp->next, ++rule_index) {
418+
bool method_matches = true;
418419

419420
if (rp->method_restriction_enabled) {
420421
if (method_wksidx >= 0 && method_wksidx < HTTP_WKSIDX_METHODS_CNT) {
421-
match = rp->standard_method_lookup[method_wksidx];
422+
method_matches = rp->standard_method_lookup[method_wksidx];
422423
} else if (!rp->nonstandard_methods.empty()) {
423-
match = false;
424+
method_matches = false;
424425
} else {
425426
int method_str_len;
426427
const char *method_str = s->hdr_info.client_request.method_get(&method_str_len);
427-
match = rp->nonstandard_methods.count(std::string(method_str, method_str_len));
428+
method_matches = rp->nonstandard_methods.count(std::string(method_str, method_str_len));
428429
}
430+
} else {
431+
// No method specified, therefore all match.
432+
method_matches = true;
429433
}
430434

431-
if (match && rp->src_ip_valid) {
432-
match = false;
433-
for (int j = 0; j < rp->src_ip_cnt && !match; j++) {
435+
// Is there a @src_ip specified? If so, check it.
436+
bool ip_matches = false;
437+
if (rp->src_ip_valid) {
438+
ip_matches = false;
439+
for (int j = 0; j < rp->src_ip_cnt && !ip_matches; j++) {
434440
bool in_range = rp->src_ip_array[j].contains(s->client_info.src_addr);
435441
if (rp->src_ip_array[j].invert) {
436442
if (!in_range) {
437-
match = true;
443+
ip_matches = true;
438444
}
439445
} else {
440446
if (in_range) {
441-
match = true;
447+
ip_matches = true;
442448
}
443449
}
444450
}
445451
}
446452

447-
if (match && rp->in_ip_valid) {
448-
Debug("url_rewrite", "match was true and we have specified a in_ip field");
449-
match = false;
450-
for (int j = 0; j < rp->in_ip_cnt && !match; j++) {
453+
// Is there an @in_ip specified? If so, check it.
454+
if (ip_matches && rp->in_ip_valid) {
455+
Debug("url_rewrite", "src_ip match was true, checking the specified in_ip range.");
456+
ip_matches = false;
457+
for (int j = 0; j < rp->in_ip_cnt && !ip_matches; j++) {
451458
IpEndpoint incoming_addr;
452459
incoming_addr.assign(s->state_machine->get_ua_txn()->get_netvc()->get_local_addr());
453460
if (is_debug_tag_set("url_rewrite")) {
@@ -460,28 +467,36 @@ UrlRewrite::PerformACLFiltering(HttpTransact::State *s, url_mapping *map)
460467
bool in_range = rp->in_ip_array[j].contains(incoming_addr);
461468
if (rp->in_ip_array[j].invert) {
462469
if (!in_range) {
463-
match = true;
470+
ip_matches = true;
464471
}
465472
} else {
466473
if (in_range) {
467-
match = true;
474+
ip_matches = true;
468475
}
469476
}
470477
}
471478
}
472479

473480
if (rp->internal) {
474-
match = s->state_machine->get_ua_txn()->get_netvc()->get_is_internal_request();
475-
Debug("url_rewrite", "%s an internal request", match ? "matched" : "didn't match");
481+
ip_matches = s->state_machine->get_ua_txn()->get_netvc()->get_is_internal_request();
482+
Debug("url_rewrite", "%s an internal request", ip_matches ? "matched" : "didn't match");
476483
}
477484

478-
if (match) {
479-
// We have a match, stop evaluating filters
480-
Debug("url_rewrite", "matched ACL filter rule, %s request", rp->allow_flag ? "allowing" : "denying");
481-
s->client_connection_enabled = rp->allow_flag;
485+
Debug("url_rewrite", "%d: ACL filter %s rule matches by ip: %s, by method: %s", rule_index,
486+
(rp->allow_flag ? "allow" : "deny"), (ip_matches ? "true" : "false"), (method_matches ? "true" : "false"));
487+
488+
if (ip_matches) {
489+
// The rule matches. Handle the method according to the rule.
490+
if (method_matches) {
491+
// Did they specify allowing the listed methods, or denying them?
492+
Debug("url_rewrite", "matched ACL filter rule, %s request", rp->allow_flag ? "allowing" : "denying");
493+
s->client_connection_allowed = rp->allow_flag;
494+
} else {
495+
Debug("url_rewrite", "ACL rule matched on IP but not on method, action: %s, %s the request",
496+
(rp->allow_flag ? "allow" : "deny"), (rp->allow_flag ? "denying" : "allowing"));
497+
s->client_connection_allowed = !rp->allow_flag;
498+
}
482499
break;
483-
} else {
484-
Debug("url_rewrite", "did NOT match ACL filter rule, %s request", rp->allow_flag ? "denying" : "allowing");
485500
}
486501
}
487502
} /* end of for(rp = map->filter;rp;rp = rp->next) */
Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
'''
2+
Verify remap.config acl behavior.
3+
'''
4+
# Licensed to the Apache Software Foundation (ASF) under one
5+
# or more contributor license agreements. See the NOTICE file
6+
# distributed with this work for additional information
7+
# regarding copyright ownership. The ASF licenses this file
8+
# to you under the Apache License, Version 2.0 (the
9+
# "License"); you may not use this file except in compliance
10+
# with the License. You may obtain a copy of the License at
11+
#
12+
# http://www.apache.org/licenses/LICENSE-2.0
13+
#
14+
# Unless required by applicable law or agreed to in writing, software
15+
# distributed under the License is distributed on an "AS IS" BASIS,
16+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
# See the License for the specific language governing permissions and
18+
# limitations under the License.
19+
20+
import os
21+
import re
22+
from typing import List, Tuple
23+
24+
Test.Summary = '''
25+
Verify remap.config acl behavior.
26+
'''
27+
28+
29+
class Test_remap_acl:
30+
"""Configure a test to verify remap.config acl behavior."""
31+
32+
_ts_counter: int = 0
33+
_server_counter: int = 0
34+
_client_counter: int = 0
35+
36+
def __init__(
37+
self, name: str, replay_file: str, ip_allow_content: str, deactivate_ip_allow: bool, acl_configuration: str,
38+
named_acls: List[Tuple[str, str]], expected_responses: list[int]):
39+
"""Initialize the test.
40+
41+
:param name: The name of the test.
42+
:param replay_file: The replay file to be used.
43+
:param ip_allow_content: The ip_allow configuration to be used.
44+
:param deactivate_ip_allow: Whether to deactivate the ip_allow filter.
45+
:param acl_configuration: The ACL configuration to be used.
46+
:param named_acls: The set of named ACLs to configure and use.
47+
:param expect_responses: The in-order expected responses from the proxy.
48+
"""
49+
self._replay_file = replay_file
50+
self._ip_allow_content = ip_allow_content
51+
self._deactivate_ip_allow = deactivate_ip_allow
52+
self._acl_configuration = acl_configuration
53+
self._named_acls = named_acls
54+
self._expected_responses = expected_responses
55+
56+
tr = Test.AddTestRun(name)
57+
self._configure_server(tr)
58+
self._configure_traffic_server(tr)
59+
self._configure_client(tr)
60+
61+
def _configure_server(self, tr: 'TestRun') -> None:
62+
"""Configure the server.
63+
64+
:param tr: The TestRun object to associate the server process with.
65+
"""
66+
name = f"server-{Test_remap_acl._server_counter}"
67+
server = tr.AddVerifierServerProcess(name, self._replay_file)
68+
Test_remap_acl._server_counter += 1
69+
self._server = server
70+
71+
def _configure_traffic_server(self, tr: 'TestRun') -> None:
72+
"""Configure Traffic Server.
73+
74+
:param tr: The TestRun object to associate the Traffic Server process with.
75+
"""
76+
77+
name = f"ts-{Test_remap_acl._ts_counter}"
78+
ts = tr.MakeATSProcess(name, enable_cache=False, enable_tls=True)
79+
Test_remap_acl._ts_counter += 1
80+
self._ts = ts
81+
82+
ts.addDefaultSSLFiles()
83+
ts.Disk.ssl_multicert_config.AddLine('dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key')
84+
ts.Disk.records_config.update(
85+
{
86+
'proxy.config.diags.debug.enabled': 1,
87+
'proxy.config.diags.debug.tags': 'http|url|remap',
88+
'proxy.config.http.push_method_enabled': 1,
89+
'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir,
90+
'proxy.config.quic.no_activity_timeout_in': 0,
91+
'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir,
92+
'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE',
93+
'proxy.config.http.connect_ports': self._server.Variables.http_port,
94+
})
95+
96+
remap_config_lines = []
97+
if self._deactivate_ip_allow:
98+
remap_config_lines.append('.deactivatefilter ip_allow')
99+
100+
# First, define the name ACLs (filters).
101+
for name, definition in self._named_acls:
102+
remap_config_lines.append(f'.definefilter {name} {definition}')
103+
# Now activate them.
104+
for name, _ in self._named_acls:
105+
remap_config_lines.append(f'.activatefilter {name}')
106+
107+
remap_config_lines.append(f'map / http://127.0.0.1:{self._server.Variables.http_port} {self._acl_configuration}')
108+
ts.Disk.remap_config.AddLines(remap_config_lines)
109+
ts.Disk.ip_allow_yaml.AddLines(self._ip_allow_content.split("\n"))
110+
111+
def _configure_client(self, tr: 'TestRun') -> None:
112+
"""Run the test.
113+
114+
:param tr: The TestRun object to associate the client process with.
115+
"""
116+
117+
name = f"client-{Test_remap_acl._client_counter}"
118+
p = tr.AddVerifierClientProcess(name, self._replay_file, https_ports=[self._ts.Variables.ssl_port])
119+
Test_remap_acl._client_counter += 1
120+
p.StartBefore(self._server)
121+
p.StartBefore(self._ts)
122+
123+
codes = [str(code) for code in self._expected_responses]
124+
p.Streams.stdout += Testers.ContainsExpression(
125+
'.*'.join(codes), "Verifying the expected order of responses", reflags=re.DOTALL | re.MULTILINE)
126+
127+
128+
IP_ALLOW_CONTENT = f'''
129+
ip_allow:
130+
- apply: in
131+
ip_addrs: 0/0
132+
action: allow
133+
'''
134+
135+
test_ip_allow_optional_methods = Test_remap_acl(
136+
"Verify non-allowed methods are blocked.",
137+
replay_file='remap_acl_get_post_allowed.replay.yaml',
138+
ip_allow_content=IP_ALLOW_CONTENT,
139+
deactivate_ip_allow=True,
140+
acl_configuration='@action=allow @src_ip=127.0.0.1 @method=GET @method=POST',
141+
named_acls=[],
142+
expected_responses=[200, 200, 403, 403, 403])
143+
144+
test_ip_allow_optional_methods = Test_remap_acl(
145+
"Verify denied methods are blocked.",
146+
replay_file='remap_acl_get_post_denied.replay.yaml',
147+
ip_allow_content=IP_ALLOW_CONTENT,
148+
deactivate_ip_allow=True,
149+
acl_configuration='@action=deny @src_ip=127.0.0.1 @method=GET @method=POST',
150+
named_acls=[],
151+
expected_responses=[403, 403, 200, 200, 400])
152+
153+
test_ip_allow_optional_methods = Test_remap_acl(
154+
"Verify defined filters are evaluated before remap lines.",
155+
replay_file='remap_acl_all_denied.replay.yaml',
156+
ip_allow_content=IP_ALLOW_CONTENT,
157+
deactivate_ip_allow=True,
158+
acl_configuration='@action=allow @src_ip=127.0.0.1 @method=GET @method=POST',
159+
named_acls=[('deny', '@action=deny @src_ip=0.0.0.0-255.255.255.255')],
160+
expected_responses=[403, 403, 403, 403, 403])
161+
162+
test_ip_allow_optional_methods = Test_remap_acl(
163+
"Verify a default deny filter rule works.",
164+
replay_file='remap_acl_all_denied.replay.yaml',
165+
ip_allow_content=IP_ALLOW_CONTENT,
166+
deactivate_ip_allow=True,
167+
acl_configuration='@action=allow @src_ip=1.2.3.4 @method=GET @method=POST',
168+
named_acls=[('deny', '@action=deny @src_ip=0.0.0.0-255.255.255.255')],
169+
expected_responses=[403, 403, 403, 403, 403])

0 commit comments

Comments
 (0)