Added ACL for checking client's connection marks by k0rv1n · Pull Request #106 · squid-cache/squid

k0rv1n · 2017-12-15T14:12:40Z

Sometimes it could be useful to check client's connection mark with ACL.

squid-prbot · 2017-12-15T14:12:41Z

Can one of the admins verify this patch?

rousskov

Thank you for improving Squid!

I see several problems in the patch, but connection marking is not my area of the expertise, so I hope that somebody else reviews it instead (and works with you to fix those problems).

Meanwhile, could you please clarify the relationship between your ACL and clientside_mark directive? Your ACL reads the markings set by client when talking to Squid, right? If clientside_mark is in effect, will it overwrite those client-set markings or do both set of markings co-exist in that case? Your explanation will affect some of the required fixes.

rousskov · 2017-12-15T15:46:17Z

OK to test

k0rv1n · 2017-12-15T16:05:41Z

client_connmark ACL and clientside_mark directive are not interchangable. New ACL aims to check connection mark that is set by iptables, just like it's done for source or destination addresses, but it does not change a mark.
This could interfere with clientside_mark in certain conditions, however if firewall uses connmarks for packets handling, new ACL should be useful. Obviously, changing client's mark in such configurations will break packet processing logic.

rousskov · 2017-12-15T16:13:44Z

I interpret your answer as

A given TCP connection has at most one mark. The mark can be set externally (e.g., by iptables) and/or internally (by Squid). The last mark set wins (i.e., overwrites the previous mark). The new ACL checks the current mark of the client-Squid connection.

Please correct me if my understanding is wrong.

yadij · 2017-12-15T15:37:25Z

src/acl/ConnMark.cc

+ACLConnMark::parse()
+{
+    while (char *t = ConfigParser::strtokFile()) {
+	std::string token(t);


Please use SBuf and Parser::Tokenizer for this parsing instead of std::string. That should remove the need for the above strToInt() custom implementation and allow detection of invalid data in or after the mask value.

Also the indentation should be 8 spaces, not tabs.

yadij · 2017-12-15T15:38:52Z

src/acl/ConnMark.cc

+	}
+
+        if ((_mark == 0 && _mask == 0xffffffff) || (_mask == 0)) {
+            debugs(28, DBG_CRITICAL, "aclParseConnMark: bad mark '" << t << "'");


Please use 'MYNAME << ": ' instead of '"aclParseConnMark: '.
same for the other critical level debugs

yadij · 2017-12-15T15:40:30Z

src/acl/ConnMark.cc

+    uint32_t conn_mark = checklist->conn()->getClientConnection()->client_nfmark;
+
+    if (marks.empty()) {
+        return 1;


surely this should be impossible code since the parse() method is terminating Squid with self_destruct() when there are no marks configured.

If it can happen at all, then -1 (unable to match) seems a more appropriate result than successful match.

yadij · 2017-12-15T15:41:53Z

src/acl/ConnMark.cc

+		continue;
+	}
+	uint32_t _mask = 0xffffffff;
+        uint32_t _mark = 0;


indentation is a little bit screwed up around these lines. probably because some are tabs and some are uneven number of spaces. Squid coding style uses 4 spaces for each level of indent.

Also, please use nfmark_t for Netfilter CONNMARK's.

yadij · 2017-12-15T15:53:26Z

src/acl/ConnMark.h

+#define SQUID_ACLCONNMARK_H
+
+#include "acl/Acl.h"
+#include "map"


Map is a STL include not a local Squid one. Use angle brackets instead of quotes here.

yadij · 2017-12-15T16:18:34Z

src/acl/Makefile.am

 	AclSizeLimit.cc \
-	AclSizeLimit.h
+	AclSizeLimit.h \
+	ConnMark.h \


Please move these additions up to the C section below ConnectionsEncryptted.* entries. It also goes .cc before .h in the listing.

yadij · 2017-12-15T16:22:26Z

src/client_side.h


    bool handleRequestBodyData();
+    // called to get client connmark
+    const Comm::ConnectionPointer getClientConnection();


clientConnection is a public member from ::Server. No need to extend the ConnStateData API.

yadij · 2017-12-15T16:33:31Z

src/ip/QosConfig.cc

 #endif
 }

+nfmark_t Ip::Qos::getNfmarkFromClient(const Comm::ConnectionPointer &client)


Most of this new code is exact duplicate of the getNfmarkFromServer() method. It would be better to make that existing method a more generic getNfMakeFromConnection(...) and have getNfmarkFromServer() and getNfmarkFromClient() specialize from that by sending the appropriate callback pointer to the generic shared method.

Once done the "nfmark" member of Comm::ConnectionPointer should be the remote endpoints CONNMARK, regardless of whether it is a server or client connection. So you can remove the new client_connmark member.

The debugs currently at line 121 (and new line 171), when in the generic method can be made to print the Comm::ConnectionPointer to self-document the connection instead of mentioning destination and source explicitly.

Please also take the opportunity to convert debugs about strerror(errno) to xstrerr(xerrno) construct on new and changed code lines. The debugs() macro uses time lookup syscalls which can change errno contents - so it must be preserved explicitly. See line 74-75 in this file for how to do it properly.

yadij · 2017-12-15T16:37:22Z

src/ip/QosConfig.cc

+                           struct nf_conntrack *ct,
+                           void *client_connmark)
+{
+    nfmark_t *mark = (nfmark_t *)client_connmark;


Please use appropriate C++ casting (static_cast here) in new and updated code instead of C casts.

yadij · 2017-12-15T16:50:51Z

src/tools.cc

        cap_list[ncaps] = CAP_NET_BIND_SERVICE;
        ++ncaps;
        if (Ip::Interceptor.TransparentActive() ||
+                Ip::Interceptor.InterceptActive() ||


Why? the existing Netfilter operations apparently do not need this level of privileges, so there should be no reason to add them now.

Without this capability netfilter says 'operation not permitted' when trying to get a connection mark. This makes sense, because we try to get a mark of a connection that is not created by squid. We query details of intercepted connection (that we do not own), so appropriate capability is a must here.

I think you have found a Linux bug then. It should not require root level privileges for reading socket state with getsockopt(), only for changing the state with setsockopt(). Please add a TODO about looking into better restrictions later.

gozzy · 2017-12-15T21:38:44Z

@rousskov, I'll try to clarify the idea of this (@k0rv1n and I are colleagues). Actually, this was designed for intercepted connections, not for explicit proxy configurations. For instance:

a client 1.1.1.1:55678 makes a connection to 2.2.2.2:80;
a firewall (e.g. iptables) marks the connection and redirects it to squid that works in interception mode;
squid accepts the connection and asks netfilter about a mark that was set for original addresses and ports (1.1.1.1:55678 -> 2.2.2.2:80);
mark is compared against expected value and a decision is made.

We've not checked if clientside_mark directive interferes with this new ACL, this case should be studied.

Also, probably the name of the new ACL is a bit confusing and something like 'origin_nfmark' will be a better option.

rousskov · 2017-12-15T22:10:33Z

this was designed for intercepted connections, not for explicit proxy configurations

I am trying to understand what that statement means for Squid admins: Does the new ACL work for explicitly proxied connections? You may ignore clientside_mark directive interference when answering this question (it is important but a separate issue) -- assume no clientside_mark directive is in use.

probably the name of the new ACL is a bit confusing

The right name will depend, in part, on the new ACL interaction with clientside_mark and on whether the new ACL works as expected for explicitly proxied connections. Both are currently open issues.

rousskov · 2017-12-15T22:15:08Z

@yadij, nice review! You have spotted most of the problems I have seen and a couple I have missed. The few remaining ones may disappear while addressing the already reported problems.

k0rv1n · 2017-12-16T08:23:44Z

Thanks for great rewiev. I'll continue work with code on Monday.

Added ACL for checking client's connection marks

5320c26

k0rv1n force-pushed the connmark_acl branch from 15b40ef to 5320c26 Compare December 15, 2017 14:24

rousskov reviewed Dec 15, 2017

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) S-waiting-for-more-reviewers needs a reviewer and/or a second opinion labels Dec 15, 2017

yadij requested changes Dec 15, 2017

View reviewed changes

rousskov removed the S-waiting-for-more-reviewers needs a reviewer and/or a second opinion label Dec 15, 2017

k0rv1n closed this Dec 19, 2017

k0rv1n mentioned this pull request Dec 19, 2017

Added ACL for checking client's connection marks #108

Closed

k0rv1n deleted the connmark_acl branch January 17, 2018 09:20

Conversation

k0rv1n commented Dec 15, 2017

Uh oh!

squid-prbot commented Dec 15, 2017

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

rousskov commented Dec 15, 2017

Uh oh!

k0rv1n commented Dec 15, 2017

Uh oh!

rousskov commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gozzy commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rousskov commented Dec 15, 2017

Uh oh!

rousskov commented Dec 15, 2017

Uh oh!

k0rv1n commented Dec 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gozzy commented Dec 15, 2017 •

edited

Loading