Added ACL for checking CONNMARK by k0rv1n · Pull Request #111 · squid-cache/squid

k0rv1n · 2017-12-21T12:17:44Z

Reworked #108.

squid-prbot · 2017-12-21T12:17:45Z

Can one of the admins verify this patch?

yadij · 2017-12-22T02:37:56Z

Can you please stop opening new PR for every little change. It should be sufficient to rebase your branch and re-push to update the initial PR. That way we get a branch trace of what was changed for audit request or elsewise, and don't have to manually schedule the QA testing.

yadij · 2017-12-22T02:39:42Z

OK to test

yadij · 2017-12-22T02:42:23Z

src/acl/ConnMark.h

+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+


extra line still present, but I'm going to run the formatter on this to check for tab issues anyway so no need to hold the merge.

rousskov · 2017-12-26T00:31:58Z

src/cf.data.pre

 	  # NOTE 2: IPv6 protocol does not contain ARP. MAC/EUI is either
 	  # encoded directly in the IPv6 address or not available.

+	acl aclname connmark mark[/mask] ...


AFAICT, the new code can only match markings on the client-Squid connections. If markings are possible on Squid-origin/peer connections as well, then we need a different name for this new ACL, right? Please note that it should not matter whether those Squid-server connection markings are set by Squid itself or something else.

Maybe clientside_connmark or client_connmark will be a better option?

I think we should use the clientside_ prefix for consistency with the existing clientside_mark and clientside_tos directives. My vote goes for clientside_mark because I see no compelling reason to spell the ACL name slightly differently than the matching directive name.

None of these names are perfect because "client side" is an overloaded/abused term in Squid context (that we try to shy away from in recent code) and because "mark" is easily confused with "tag" and "annotation". However, I have nothing better to offer in this issue scope.

Please use clientside_mark for the new ACL name.

rousskov

I found a few problems and added a few polishing requests. Please see inlined comments for details.

rousskov · 2017-12-27T15:36:18Z

src/cf.data.pre

 	  # NOTE 2: IPv6 protocol does not contain ARP. MAC/EUI is either
 	  # encoded directly in the IPv6 address or not available.

+	acl aclname connmark mark[/mask] ...


I think we should use the clientside_ prefix for consistency with the existing clientside_mark and clientside_tos directives. My vote goes for clientside_mark because I see no compelling reason to spell the ACL name slightly differently than the matching directive name.

None of these names are perfect because "client side" is an overloaded/abused term in Squid context (that we try to shy away from in recent code) and because "mark" is easily confused with "tag" and "annotation". However, I have nothing better to offer in this issue scope.

Please use clientside_mark for the new ACL name.

rousskov · 2017-12-27T15:42:02Z

src/acl/ConnMark.cc

+            _mark = static_cast<nfmark_t>(tmp_result);
+        else {
+            debugs(28, DBG_CRITICAL, MYNAME << ": bad mark '" << token << "'");
+            self_destruct();


When handling errors in any new parsing code, please throw a TexcHere() exception instead of killing Squid with self_destruct(). We do not want to kill the Squid process during hot reconfiguration, and we will eventually support atomic rejection of invalid configurations (i.e., configurations that throw).

Please note that you do not need to catch and report the exception. There is already code for that (and there are pending projects that work on improving that code). Your patched Squid may still die when you throw instead of self_destruct()ing, but that unfortunate side effect is temporary, and you do not need to worry about it as long as you throw correctly.

rousskov · 2017-12-27T15:45:16Z

src/acl/ConnMark.cc

+            continue;
+        }
+
+        nfmark_t _mask = 0xffffffff;


In Squid code, please do not start names with an underscore.

rousskov · 2017-12-27T15:46:41Z

src/acl/ConnMark.cc

+        nfmark_t _mark = 0;
+
+        Parser::Tokenizer tokenizer(token);
+        int64_t tmp_result;


Please use camelCase for names in Squid code. Here, I suggest calling the result storage number, but it is your call.

rousskov · 2017-12-27T15:47:50Z

src/cf.data.pre

+	acl aclname connmark mark[/mask] ...
+	  # [fast]
+	  # check CONNMARK
+	  # mark and mask - hex or decimal values


The "hex or decimal" description does not match the parsing code which accepts octal values as well.

rousskov · 2017-12-27T17:08:26Z

src/ip/QosConfig.h

+ */
+enum ConnType {
+    Incoming,
+    Outgoing


Avoid the terms "incoming" and "outgoing" because a packet sent by Squid to the client is treated as an "outgoing" packet in some contexts. It would be awkward to deal with outgoing packets on incoming connections :-).

It is a good idea to use a common prefix for enum values, especially when they are placed in a relatively large (and mostly unrelated!) context like Qos. There is a good change this enum will be moved to Connection.h (or similar) later.

See another comment for specific suggestions (but you may be able to find better prefixes/names).

rousskov · 2017-12-27T17:15:27Z

src/ip/QosConfig.h

+* @param conn_type Specifies connection type (incoming or outgoing)
 */
-void getNfmarkFromServer(const Comm::ConnectionPointer &server, const fde *clientFde);
+nfmark_t getNfmarkFromConnection(const Comm::ConnectionPointer &conn, enum ConnType conn_type = Outgoing);


Please remove the default parameter value. When there is no reasonable default, it is a good idea to make the caller think what it actually wants.

rousskov · 2017-12-27T17:21:39Z

src/acl/ConnMark.cc

+    }
+
+    if (marks.empty()) {
+        debugs(28, DBG_CRITICAL, MYNAME  << ": expect at least one connmark");


Please use typeString() instead of MYNAME -- MYNAME is expanded into useless-in-this-context parse. I would also add an "acl" prefix:

throw TexcHere(ToSBuf("acl ", typeString(), " requires at least one mark"));

Please check other new error reporting code for similar problems.

Eventually, context reporting will be handled by the upper level parser, but we are not there yet.

rousskov · 2017-12-27T17:30:56Z

src/comm/TcpAcceptor.cc

        return;
    }

+    newConnDetails->nfmark = Ip::Qos::getNfmarkFromConnection(newConnDetails, Ip::Qos::Incoming);


This adds performance overhead for all accepted connections even if the admin is not using the new ACL, right? How expensive are the associated system calls? Would it be easy to bypass this assignment when the new ACL is not used?

Similar concern applies to the FwdState code, but we were extracting the possibly useless mark there before your changes so I cannot argue a performance regression there.

rousskov · 2017-12-27T17:33:31Z

src/ip/QosConfig.cc

            if (x == -1) {
-                debugs(17, 2, "QOS: Failed to retrieve connection mark: (" << x << ") " << strerror(errno)
-                       << " (Destination " << server->remote << ", source " << server->local << ")" );
+                int xerrno = errno;


Please add const.

k0rv1n · 2017-12-31T17:35:26Z

Happy New Year, guys. I`ll commit changes at 8-9 of January.

rousskov · 2018-01-11T00:52:09Z

I suspect something went wrong when you tried to incorporate master changes into your feature branch -- the feature branch now shows 2K changed files and a few conflicts. FWIW, in most solo-developer cases, the best strategy is to rebase your changes on top of master (instead of merging master into your branch).

rousskov

The code looks much better, thank you.

I added a few suggestions based on the last commit alone, but I cannot easily review the whole thing due to master-merge problems that I mentioned a bit earlier.

rousskov · 2018-01-11T01:01:59Z

src/acl/ConnMark.cc

-                debugs(28, DBG_CRITICAL, MYNAME << ": bad mask '" << token << "'");
-                self_destruct();
+        if (!tokenizer.atEnd()) {
+            if (tokenizer.skip('/')) {


Please simplify by moving the slash check up and collapsing the two atEnd() checks into one:

if (tokenizer.skip('/') { ... parse the mask ... } if (!tokenizer.atEnd()) throw TextException(ToSBuf("acl ", typeString(), ": trailing garbage in ", token), Here());

rousskov · 2018-01-11T01:08:32Z

src/acl/ConnMark.cc

+        if (!tokenizer.atEnd()) {
+            if (tokenizer.skip('/')) {
+                if (tokenizer.int64(number, 0, false))
+                    mask = static_cast<nfmark_t>(number);


AFAICT, this code silently ignores user input due to a 64- to 32-bit conversion. Before you cast, add an overflow check. For example:

if (number > std::numeric_limits<nfmark_t>::max()) throw ... too big

BTW, you may want to encapsulate all those repeated number parsing and checks into a helper function, but I do not insist on that.

rousskov · 2018-01-11T01:10:40Z

src/acl/ConnMark.cc

        SBuf s;
-        s.Printf("0x%08x/0x%08x", m.first, m.second);
+        s.Printf("0x%08x", m.first);
+        if (m.second)


This condition does not look right because the default mask (i.e., the one that should not be printed) is 0xffffffff, not zero.

rousskov · 2018-01-11T01:12:32Z

src/acl/ConnMark.cc

+    for (const auto &m : marks) {
        SBuf s;
-        s.Printf("0x%08x/0x%08x", m.first, m.second);
+        s.Printf("0x%08x", m.first);


I suspect you will find writing good debugging statements, exceptions, and this dump() code easier if you add an operator to print std::pair<nfmark_t, nfmark_t> into std::ostream!

yadij

just some polishing to do. overall it looks good to me now.

yadij · 2018-01-11T16:08:41Z

src/acl/ConnMark.cc

+#include <string>
+
+bool
+Acl::ConnMark::empty () const


please remove the extra space between method name and brackets

yadij · 2018-01-11T16:09:21Z

src/acl/ConnMark.cc

+#include "Parsing.h"
+#include "sbuf/Stream.h"
+
+#include <string>


Please consider removing, this should no longer be necessary with the SBuf changes.

yadij · 2018-01-11T16:31:05Z

src/cf.data.pre

+	  # The client, various intermediaries, and Squid itself may set
+	  # CONNMARK at various times. The last CONNMARK set wins. However,
+	  # this ACL only checks the mark present on a freshly accepted
+	  # connection, before clientside_mark is checked.


That last sentence is incorrect. This ACL is based on the Comm::Connection stored mark. Which may have been updated already on a persistent connection by a previous HTTP request use of clientside_mark, or by the directive using the ACL happening after clientside_mark for the current transaction. eg http_access uses the previous transaction (or client original) marking, whereas logging for the same transaction uses the clientside_mark changed value.

The most that can be said is that this ACL checks for the most recent CONNMARK used on the client connection.

That last sentence is incorrect.

Agreed. I probably missed setSockNfmark() calls when drafting it. Sorry.

The most that can be said is that this ACL checks for the most recent CONNMARK used on the client connection.

That does not tell the admin what they need to know IMO: It is important to warn the admin that the mark they are testing at time X may not match the actual/real connection mark at that time. How about the following?

# The client, various intermediaries, and Squid itself may set # CONNMARK at various times. The last CONNMARK set wins. This ACL # checks the mark present on an accepted connection or set by # Squid afterwards, depending on the ACL check timing. This ACL # effectively ignores any mark set by other agents after Squid has # accepted the connection.

rousskov

Great progress, thank you! I agree that we are almost done, but I found one bug (ConnMark::marks type) so you will have to endure another polishing round.

rousskov · 2018-01-11T17:45:50Z

src/acl/ConnMark.cc

+operator <<(std::ostream &os, const std::pair<nfmark_t,nfmark_t> connmark)
+{
+    using namespace std;
+    ios_base::fmtflags oldFlags = os.flags();


Please make it const. I suggest a simpler version:

const auto oldFlags = os.flags();

BTW, after the above change, consider removing using namespace std; because there will be only two case of explicit namespace use: std::hex and std::showbase.

rousskov · 2018-01-11T17:48:05Z

src/acl/ConnMark.cc

+    return false;
+}
+
+inline std::ostream &


s/inline/static/ as more precise -- we do not really care whether the compiler inlines this operator.

rousskov · 2018-01-11T17:57:00Z

src/acl/ConnMark.cc

+        nfmark_t mark = 0;
+        nfmark_t mask = 0xffffffff;
+
+        mark = getNumber(tokenizer, token);


Please make mark constant:

const auto mark = getNumber(tokenizer, token);

rousskov · 2018-01-11T17:58:09Z

src/acl/ConnMark.cc

+            throw TexcHere(ToSBuf("acl ", typeString(), ": trailing garbage in ", token));
+        }
+
+        std::pair<nfmark_t, nfmark_t> connmark(mark, mask);


If possible, make connmark constant.

rousskov · 2018-01-11T18:00:58Z

src/acl/ConnMark.cc

+Acl::ConnMark::match(ACLChecklist *cl)
+{
+    const auto *checklist = Filled(cl);
+    nfmark_t connMark = checklist->conn()->clientConnection->nfmark;


If possible, make connMark constant. You can use const auto here AFAICT.

Also, please use either connMark or connmark for consistency sake. I recommend the former.

rousskov · 2018-01-11T18:25:06Z

src/ip/QosConfig.cc

+         * src = client address(remote), dst = our address(local)
         */
+        auto dst = conn->remote;
+        auto src = conn->local;


If possible, make these constant.

Please use one-line initialization and removing the comment about "swapping" as no longer necessary:

const auto src = (connDir == Ip::Qos::dirAccepted) ? conn->remote : conn->local; const auto dst = (connDir == Ip::Qos::dirAccepted) ? conn->local : conn->remote;

rousskov · 2018-01-11T18:54:24Z

src/acl/ConnMark.cc

+
+    if (marks.empty()) {
+        throw TexcHere(ToSBuf("acl ", typeString(), " requires at least one mark"));
+    }


FYI: You do not need curly braces around simple, one-line statements like this. I do not insist on removing them. Just mentioning this here in case you do not like them either but think they should be there for some vague reason satisfying the official code "style"...

rousskov · 2018-01-11T18:57:25Z

src/acl/ConnMark.cc

+}
+
+nfmark_t
+Acl::ConnMark::getNumber(Parser::Tokenizer &tokenizer, const SBuf &token)


If possible, please make this method const.

rousskov · 2018-01-11T18:58:31Z

src/ip/QosConfig.cc

-    fde *clientFde = (fde *)data;
-    clientFde->nfmarkFromServer = nfct_get_attr_u32(ct, ATTR_MARK);
-    debugs(17, 3, "QOS: Retrieved connection mark value: " << clientFde->nfmarkFromServer);
+    nfmark_t *mark = static_cast<nfmark_t *>(connmark);


Use auto to reduce code duplication:

auto mark = static_cast<nfmark_t *>(connmark);

rousskov · 2018-01-11T19:00:54Z

src/acl/ConnMark.cc

+
+    os << hex << showbase << connmark.first;
+    if (connmark.second != 0xffffffff) {
+        os << "/" << connmark.second;


Use single quotes to print single characters -- it could be much faster and may help the compiler to inline more: '/'.

* No need to 'quote' integers -- they cannot have spaces and such. * Rely on file and function names (auto-added by debugs()) for context.

rousskov · 2018-01-16T18:30:59Z

@k0rv1n, I took the liberty to push a few minor fixes and polishing updates. See the branch change log for details -- I isolated and documented every change. I also formatted and rebased the branch. The latter required a force push. If you are OK with these changes (and the tests pass), your branch will be merged. Otherwise, we can revert the branch to its original state.

rousskov · 2018-01-16T18:38:43Z

I wonder whether Squid should use uppercase for printing these hex numbers? We should probably use whichever case folks are more likely to use in their configuration files. I do not know what that is. Here is the current debugging sample:

ConnMark.cc(65) parse: added 0xabc/0xa
ConnMark.cc(65) parse: added 0xabcd/0xb
ConnMark.cc(84) match: skipped 0xabc/0xa mismatching 0
ConnMark.cc(84) match: skipped 0xabcd/0xb mismatching 0
ConnMark.cc(84) match: skipped 0xabc/0xa mismatching 0
ConnMark.cc(84) match: skipped 0xabcd/0xb mismatching 0
ConnMark.cc(84) match: skipped 0xabc/0xa mismatching 0x12
ConnMark.cc(84) match: skipped 0xabcd/0xb mismatching 0x12

TODO: We should probably convert the old code printing nfmarks (and ToS marks?) to use asHex() so that all debugging/reporting is consistent. This can be done as a separate polishing PR IMO.

k0rv1n · 2018-01-16T20:55:00Z

I don't mind merging this. Thank you.

rousskov · 2018-01-16T23:35:36Z

@k0rv1n, thank you and, if GitHub is not misleading me, congratulations on your first official Squid commit!

Do you know whether it is customary to enter CONNMARKs using uppercase (0xABCD) or lowercase (0xabcd) hex values?

k0rv1n · 2018-01-17T09:13:45Z

I suppose that it would be preferable to use lowercase marks.

Matches CONNMARK of accepted connections. Takes into account clientside_mark and qos_flows mark changes (because Squid-set marks are cached by Squid in conn->nfmark). Ignores 3rd-party marks set after Squid has accepted the connection from a client (because Squid never re-queries the connection to update/sync conn->nfmark). Also added a debugs()-friendly API to print hex values.

yadij · 2018-01-23T21:34:07Z

IMO we should accept either case on input, but print in upper case when displaying so the lower case 'x' is more quickly/clearly visible.

rousskov · 2018-01-24T00:24:08Z

Agreed regarding accepting both cases and regarding upper case resulting in a more readable text after the 0x prefix. I would be happy to change code to use upper case, but I am worried that (AFAICT) @k0rv1n thought that most admins would use lower case in squid.conf.

It would be nice if there were some "standard" CONNMARK rendering that we could point to as the authoritative "norm". How do various Unix tools render CONNMARKs? What case does their configuration documentation use?

yadij · 2018-01-24T01:07:36Z

Netfilter documentation uses decimal numbers.

Iptables tools produce lower-case hex, but for those tools all the output line is universally lower cased. Even names that are officially defined as being upper-case by netfilter.

yadij · 2018-01-24T01:09:05Z

It is not a big issue either way, if the patch is ready it can be merged.

rousskov · 2018-01-24T01:19:15Z

This PR was merged a week ago.

It is not too late to change the (default) asHex() rendering case to uppercase though. I just wish there would be a more clear indication of what the right choice is here, but it sounds like everybody is doing their own thing :-(.

yadij approved these changes Dec 22, 2017

View reviewed changes

rousskov reviewed Dec 26, 2017

View reviewed changes

rousskov requested changes Dec 27, 2017

View reviewed changes

rousskov requested changes Jan 11, 2018

View reviewed changes

k0rv1n force-pushed the connmark_acl_3 branch from 2a42d06 to f3c373c Compare January 11, 2018 11:34

yadij requested changes Jan 11, 2018

View reviewed changes

rousskov requested changes Jan 11, 2018

View reviewed changes

k0rv1n force-pushed the connmark_acl_3 branch 2 times, most recently from 5c655df to ba277bb Compare January 15, 2018 07:53

yadij approved these changes Jan 16, 2018

View reviewed changes

k0rv1n and others added 7 commits January 16, 2018 11:11

Added ACL for checking CONNMARK

6b3a6ce

Use asHex() wrapper instead of manually creating AsHex<> objects.

6d6ba11

Simplified debugging messages.

263ca4a

* No need to 'quote' integers -- they cannot have spaces and such. * Rely on file and function names (auto-added by debugs()) for context.

Document new class data members.

06539bc

Improved ConnMark ACL registration code consistency.

bcf4cb6

Fixed documented getNfmarkFromConnection() parameter name.

7ce1450

SourceFormat

c284441

rousskov force-pushed the connmark_acl_3 branch from ba277bb to c284441 Compare January 16, 2018 18:27

rousskov approved these changes Jan 16, 2018

View reviewed changes

rousskov merged commit 653d992 into squid-cache:master Jan 16, 2018

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

		* Please see the COPYING and CONTRIBUTORS files for details.
		*/

Conversation

k0rv1n commented Dec 21, 2017

Uh oh!

squid-prbot commented Dec 21, 2017

Uh oh!

yadij commented Dec 22, 2017

Uh oh!

yadij commented Dec 22, 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!

rousskov left a comment

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!

k0rv1n commented Dec 31, 2017

Uh oh!

rousskov commented Jan 11, 2018

Uh oh!

rousskov left a comment

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!

yadij left a comment

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!

rousskov left a comment

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