[PATCH] NAT lookups upgrade for FreeBSD and OpenBSD
Current OpenBSD implementation of PF divert-to works similarly to TPROXY
and only requires a getsockname() lookup to locate the TCP packet
original destination.
The work by Marios with some additional tweaks discovered in recent
testing has now gone into 3.HEAD providing Squid with working http_port
tproxy option.
We can use the same PF configuration to preform intercept option but
the old PF transparent code does lookups on /dev/pf which fails badly on
the new PF versions. getsockname() is what is really required and
already performed by TcpAcceptor on all incoming connections, so there
is no need for a special PF lookup code now.
This patch adds a new ./configure option --with-nat-devpf to enable the
old /dev/pf NAT lookup code in a backward-compatible way for older OS
versions and OpenBSD based distros which have not yet ported the new PF
code. The option is disabled by default since the systems requiring it
are fairly old now.
This also removes the getsockname() lookup in the IPFW lookup
implementation which is redundant behind TcpAcceptor.
NP: we still do not support the new PF rdr-to which is doing more
NAT-like operations that TPROXY-like ones. However nobody has been able
to supply any information on how we would lookup those details. So until
that appears we support both http(s)_port intercept and tproxy options
using only the PF divert-to syntax.
Amos
=== modified file 'configure.ac'
--- configure.ac2013-01-22 06:29:59 +
+++ configure.ac2013-04-01 12:04:25 +
@@ -1459,7 +1459,17 @@
[unrecognized argument to --enable-pf-transparent: $enableval])
])
#will be AC_DEFINEd later, after checking for appropriate infrastructure
-AC_MSG_NOTICE([PF-based transparent proxying requested:
${enable_pf_transparent:=auto}])
+AC_MSG_NOTICE([PF-based transparent proxying requested:
${enable_pf_transparent:=no}])
+
+dnl Enable /dev/pf support for older PF Transparent Proxy systems (OpenBSD 4.x
and older)
+AC_ARG_WITH(nat-devpf,
+ AS_HELP_STRING([--with-nat-devpf],
+[Enable /dev/pf support for NAT on older OpenBSD and FreeBSD kernels.]), [
+ SQUID_YESNO([$enableval],
+ [unrecognized argument to --with-nat-devpf: $enableval])
+])
+#will be AC_DEFINEd later, after checking for appropriate infrastructure
+AC_MSG_NOTICE([NAT lookups via /dev/pf: ${with_nat_devpf:=no}])
# Linux Netfilter Transparent Proxy
AC_ARG_ENABLE(linux-netfilter,
@@ -3349,22 +3359,24 @@
CXXFLAGS=-DSOLARIS2=$solrev $CXXFLAGS
fi
-dnl PF support requires a header file.
-if test x$enable_pf_transparent != xno ; then
+dnl PF /dev/pf support requires a header file.
+if test x$with_nat_devpf != xno ; then
if test x$ac_cv_header_net_pfvar_h = xyes -o \
x$ac_cv_header_net_pf_pfvar_h = xyes; then
-if test x$enable_pf_transparent = xauto ; then
- enable_pf_transparent=yes
+if test x$with_nat_devpf = xauto ; then
+ with_nat_devpf=no
fi
else
-if test x$enable_pf_transparent = xyes ; then
- AC_MSG_ERROR([PF-based transparent proxy requested but needed header not
found])
+if test x$with_nat_devpf = xyes ; then
+ AC_MSG_ERROR([PF /dev/pf based NAT requested but needed header not
found])
fi
-enable_pf_transparent=no
+with_nat_devpf=no
fi
fi
-SQUID_DEFINE_BOOL(PF_TRANSPARENT,$enable_pf_transparent,
+SQUID_DEFINE_BOOL(PF_TRANSPARENT,${enable_pf_transparent:=no},
[Enable support for PF-style transparent proxying])
+SQUID_DEFINE_BOOL(USE_NAT_DEVPF,${with_nat_devpf:no},
+ [Enable support for /dev/pf NAT lookups])
if test x$enable_linux_netfilter != xno ; then
if test x$ac_cv_header_linux_netfilter_ipv4_h = xyes; then
=== modified file 'src/ip/Intercept.cc'
--- src/ip/Intercept.cc 2013-04-01 09:54:22 +
+++ src/ip/Intercept.cc 2013-04-01 11:27:20 +
@@ -164,26 +164,15 @@
Ip::Intercept::IpfwInterception(const Comm::ConnectionPointer newConn, int
silent)
{
#if IPFW_TRANSPARENT
-struct sockaddr_storage lookup;
-socklen_t len = sizeof(struct sockaddr_storage);
-newConn-local.GetSockAddr(lookup, AF_INET);
-
-/** \par
- * Try lookup for IPFW interception. */
-if ( getsockname(newConn-fd, (struct sockaddr*)lookup, len) != 0 ) {
-if ( !silent ) {
-debugs(89, DBG_IMPORTANT, HERE IPFW getsockname(...) failed:
xstrerror());
-lastReported_ = squid_curtime;
-}
-debugs(89, 9, HERE address: newConn);
-return false;
-} else {
-newConn-local = lookup;
-debugs(89, 5, HERE address NAT: newConn);
-return true;
-}
+/* The getsockname() call performed already provided the TCP packet
details.
+ * There is no way to identify whether they came from NAT or not.
+ * Trust the user configured properly.
+ */
+debugs(89, 5, HERE address NAT: newConn);
+return true;
+#else
+return false;
#endif
-return false;
}
bool
@@ -286,9 +275,8 @@
Re: [RFC] Time to talk about StringNG merge again?
-- This is not a review, but I wanted to clarify or emphasize some of the points in Amos' review. Thank you Amos for working on this! On 03/30/2013 02:32 AM, Amos Jeffries wrote: in src/SBuf.cc: * SBuf::chop() - the TODO optimization is not possible. We cannot be completely sure we are teh only user of the I think it is possible, but the MemBlob cooperation would be required to ensure correctness. * SBuf::operator []() documentation is incorrect. It *does* check access bounds, but returns \0 for any bytes outside instead of throwing exception. The documentation is correct. The implementation is no longer correct. Please revert to the recent code version that did none of the above. The [] operator is meant for performance-sensitive loops and should not do any checks. Even the stats collection should be removed from it IMO. We provide at() that trades speed for safety. std::string does the same. in src/SBuf.cci BTW, IIRC, we discussed moving all .cci stuff into .h. Or was that a different project? * Sbuf::bufEnd() is lacking any checks about whether the space returned is safe to use, no cow() or whatever. bufEnd() is a private convenience method. It is unsafe and non-cowing by design. The callers should check and cow, as needed. If the documentation does not reflect that, it should be fixed. Thank you, Alex.
Re: [RFC] Time to talk about StringNG merge again?
Hi, I forgot to mention another couple of changes:
- I added an explicit copy-constructor to import std::string
- I relaxed the behavior of the chop() method for out-of-bounds cases:
instead of throwing, it'll now ignore the part of the specified limits
that is out of bounds.
On Mon, Apr 1, 2013 at 7:43 PM, Kinkie gkin...@gmail.com wrote:
My audit, I'm sure Alex will have more.
+1 for the src/MemBlob.cc and src/MemBlob.h operator addition to go into
trunk immediately. The rest has a lot of polishing still needed.
Ok, I'll wait for Alex' review before merging.
Design Question: with all the stats being collected is it worth collecting
an assign-to-self counter? to see if there are any obvious logic bugs in
usage which we could avoid?
I tried to measure most operations clusters, to measure the
effectiveness of the shared-storage approach during the transition
phase.
Once we have a clearer picture, we can gradually remove the
meaningless statistics.
in src/SBuf.cc:
* can we consider making it a guideline to place constructor lists with the
colon after the constructor definition and the initializer list one to a
line following, with indentation? It makes it so much clearer to see what
member is being initialized when there is any kind of complex parameters for
it. NP: astyle enforces only the indentation.
ie:
+SBuf::SBuf() :
+store_(GetStorePrototype()),
+off_(0),
+len_(0)
+{
It uses lots of vertical space, and unintialized members can get
caught by Coverity anyway.
This said, I have no strong opinion.
** along with this can we ensure that the constructor arguments are all on
one line. If it breaks the 80-character line style preference we need to
reconsider what its doing.
- case in point there being the constructor for OutOfBoundsException.
Well, OutOfBoundsException is 19 chars long; add the class name and
half the vertical screen estate has been used.
Again, no strong opinion. I don't think we should put constraints on
length of class names etc. Besides, implementation classes are not
really meant to be read often are they?
* SBuf::vappendf()
- comment //times 1.2 for instance... does nt match the reserveSpace()
call it is about. The second line of comment can be erased.
Done.
- according to the comments about Linux vsnprintf() return of -1 is
guaranteed to violate the Must() condition when reserveSpace(-1*2) is
called. The if-statement sz0 condition ensures it.
Changed the two cases, now throws TextException in case of output
error - I'd assume it's unrecoverable.
- please avoid C-style casting in new code whenever possible. May occur
elsewhere also.
Ok.
- the line +if (operator[](len_-1) == 0) { should be comparing
against '\0' instead of 0 to ensure byte/word alignment changes by the
compiler don't involve magic type castings.
Ok, done.
* SBuf::compare() please move the stats.compareSlow counting up top. Just
under the Must() keeps it in line with the pther stats collections.
Ok.
* SBuf::startsWith() lacks the length debugs() statement used in
operatror==() immediately below it.
Added.
* SBuf::copy() why not do: const SBuf::size_type toexport = min(n,
length());
Done.
* SBuf::forceSize() - we need to consider what happens (or already has
happened) when a size greater than capacity is forced.
Right.
What about throwing an OutOfBoundsException? Overflow has likely happened.
Not changed yet.
* SBuf::chop() - the TODO optimization is not possible. We cannot be
completely sure we are teh only user of the
Removed the TODO. It's a very unlikely case anyway.
* SBuf::rfind() - typo in weridness comment memrhr.
Fixed.
* Please move operator () to an inline function.
Done (via .cci for now. I recall Alex was suggesting to get rid of
.cci, will move to .h if I can find confirmation of that).
* SBuf::reAlloc() comment needs to be doxygen format.
Done.
in SBuf.h
* Debug.h is not needed here. Please move to SBuf.cci instead.
- probably also SquidString.h can also be removed, but check that with a
build test.
Needed for String import/export functions.
Hopefully String will just go in a relatively short time after merging.
* several places saying * \note bounds is 0 = pos length() are
incorrect. SBuf::checkAccessBounds() tests for: 0 = pos = length()
Then checkAccessBounds is wrong. Changed unit test accordingly.
* s/minCapcity/minCapacity/
Fixed.
* SBuf::operator []() documentation is incorrect. It *does* check access
bounds, but returns \0 for any bytes outside instead of throwing exception.
Gah! Changed to match documentation.
* SBuf::vappendf() documentation lacking empty line separate from earlier
method declaration above.
Added.
* SBuf::chop()
- documentation contains HTML tags, Please avoid that. 'i' and 'n' (quoted)
is a better written style of emphasis.
Fixed.
- documentatin of n==0 case lacks the info that SBuf is
Re: [RFC] Time to talk about StringNG merge again?
On 04/01/2013 11:43 AM, Kinkie wrote: Attached is a bundle containing the changes from this review. Will you commit that to your stringng branch on lp? Or should I patch that branch before reviewing/testing instead? Thank you, Alex.
Re: [RFC] Time to talk about StringNG merge again?
On 2/04/2013 6:57 a.m., Alex Rousskov wrote: -- This is not a review, but I wanted to clarify or emphasize some of the points in Amos' review. Thank you Amos for working on this! On 03/30/2013 02:32 AM, Amos Jeffries wrote: in src/SBuf.cc: * SBuf::chop() - the TODO optimization is not possible. We cannot be completely sure we are teh only user of the I think it is possible, but the MemBlob cooperation would be required to ensure correctness. * SBuf::operator []() documentation is incorrect. It *does* check access bounds, but returns \0 for any bytes outside instead of throwing exception. The documentation is correct. The implementation is no longer correct. Please revert to the recent code version that did none of the above. The [] operator is meant for performance-sensitive loops and should not do any checks. Even the stats collection should be removed from it IMO. We provide at() that trades speed for safety. std::string does the same. in src/SBuf.cci BTW, IIRC, we discussed moving all .cci stuff into .h. Or was that a different project? We had. However since it was not done yet I reviewed in-place. Kinkie; I have a little time in the next few hours to try my hand at this as a test of the shared-access branch. * Sbuf::bufEnd() is lacking any checks about whether the space returned is safe to use, no cow() or whatever. bufEnd() is a private convenience method. It is unsafe and non-cowing by design. The callers should check and cow, as needed. If the documentation does not reflect that, it should be fixed. Thank you, Alex.
