Add support for parsing and using the SYNPROXY jump target and its options #1127
Conversation
|
Okay so...I'm a little bit worked up right now, and I know (because I too have read #1100) that this is not going to fall on deaf ears, but I'm going to be completely honest: I would have been time, money, and remaining hair ahead to have just ripped the parser completely out and started from scratch implementing it in optparse. This is unbelievably bad. I have never seen Ruby code so unreadable, and as an OSP dev for something like ten years now, this is not a great example of what PuppetLabs can do. The issue I'm currently solving is that Assuming that all arguments across the ip[6]tables core and all the extensions are going to be globally unique in a tool that from its inception was based on the idea that an argument's context determines its meaning is the height of naïveté. Since I only JUST discovered that the target extension options will in some cases sort before some of the matcher extensions in the output of Then there's also the enjoyable case of I'm like, this close ( |
Rubocop's maximum value for allowed block length needs to be incremented by hand each time the 'firewall' type gains a new feature, because the maximum block length is hand-tuned to the size of that 'newtype' block. Having the 'Metrics/BlockLength' cop instead ignore lengths for the 'newtype' and 'provider' methods (which inherently have very long blocks) instead solves the problem for all 'newtype' and 'provider' blocks at one fell swoop, and with addition of one inline disable comment for the 'validate' block in the 'firewall' type, means all remaining block length warnings are squelched by a max value of only 64 lines.
Add the ability to parse iptables rules we encounter that include any option for the SYNPROXY jump target (including RedHat's --ecn option), instead of issuing a "Skipping unparseable iptables rule" warning. Every Firewall resource requires a full parse of all rules, and the warning appears each time the problem rule is parsed; therefore these warning messages are noisy (especially on long rule sets), and there is no way to work around this, make the parser ignore the rule, or create other rules with Puppet in any order aside from "above the unparseable rule."
Add support (and unit tests) for creating rules that use the SYNPROXY iptables extension and its configuration options, including the undocumented, RedHat-supported '--ecn' option.
Move tests for the ip6tables provider into the ip6tables provider unit test file. Even if it's subclassed from iptables, it should be reasonable to expect running all tests in the ip6tables provider's tests file to include all tests for the ip6tables provider. Also update several test definitions that had fallen out of sync with the iptables provider's test framework so the test cases for ip6tables are also no longer sensitive to specific hash order and stringified 'true'.
Provide the same support for rule parse and create for IPv6 as the previous commit provided for IPv4
By default the test failure output when a rule is unparseable is just `undefined method '[]' for nil:NilClass`, which does not meaningfully indicate that an error has happened in parsing, or which error. This makes any current or future warning from the parser appear in test failure output.
Take a few minutes and address the 40+ lines of deprecation warnings from the test framework, so nobody has to get constantly spammed while trying to do module development anymore.
Add support for testing against multiple kernel and iptables versions, to ensure that features gated by kernel or iptables version aren't rendered un-testable. Also, pull the handy construct for handling the addition of '--wait' in later iptables versions in the ip6tables test cases over to iptables tests as well, because with multi-version support, the test cases for iptables will start to need it as well.
Add the test cases not present in unit testing, but used in acceptance testing, to the unit tests as well. Not everyone who works on the module has the ability to run the acceptance tests, but acceptance test failures should not come as a surprise to developers being diligent about passing all unit tests.
The @resource_list variable is almost impossible to reason about or check sync between v4 vs. v6 provider implementations - align them!
The @resource_list determines the order in which arguments appear on the generated commandline for ip[6]tables; if there are options which belong to a match module interleaved with options which belong to a target extension, and both the target and match extensions are used in the same rule, perverse outputs like '-j SYNPROXY -m tcpmss --mss 1360 --sack-perm' are possible - where an entire match extension and its arguments are inserted between a target extension and its arguments. Group options which belong to a target extension together immediately behind :jump, so a target extension spec and its arguments cannot be separated; and add comments in the array to ensure it remains clear why this is important.
Not all arguments are globally unique across all iptables extensions, and we should not search the entire rule string for all arguments when each one is only ever valid within a known context. Define start and stop delimiters for existing known overlaps, and default to a full-string search if no start or end markers are defined. Also, stop being quite so terse; the Ruby authors do not charge by the letter for variable names, and readability is good.
The regular parse mechanisms can handle table, chain, and comment(s) on their own already, if a single space character is added to the head of the rule. :table was already in @resource_map, :chain is added to the parse output by the unnecessary special mechanism anyway, and :name already grabs ALL comments successfully since the escaping fixes. 'line' is used instead of 'line.dup' because string addition returns a new string, so String#dup would be redundant.
Parse the rule from the head to prevent the parser from finding aliases of the intended argument in the rule's heead. String#slice! searches from the head of a string, so if the parser is using it to iteratively search for strings ordered progressively further from the back of a rule, each iteration is a new opportunity to find an aliased substring in its head and completely moot the point of generating an ordered parse list in the first place. Additionally: - Untie the logical knots that were required for reverse-parsing - Don't use Enumerable#each to do absolutely everything by side effect when an iterator could just return the constructed value directly - Don't pre-define variables we don't need yet just so they can be reassigned - Freshen up pointlessly terse variable names that only exist in an ephemeral scope anyway, this is already hard enough to follow - Stop our value regex from capturing a second, nested group if we only need the outer to avoid any later need to transpose the result - Fix any remaining variables with misleading names (e.g., valrev is no longer "values, reversed") or errant comments that no longer accurately describe the operation of the code
The ip[6]tables providers understand handling extension modules; treat tcpmss as an extension module, rather than mangling its matcher
Parse the trailing arguments to '-m rpfilter' out of existing rules. Prior behavior included the '--' prefix along with the options themselves when pulling them out of the rule. For ip6tables, the provider could not correctly generate an ip6tables commandline that included '-m rpfilter' at all - its inclusion in the known booleans array precluded its options being expanded or included at all. Additionally: - Using a comma rather than a space as a separator character in the pre-parse munging doesn't require any quotes, nor does it require any new post-parse munging when there is already an existing iterator to handle splitting of comma-separated multiple elements into arrays - '-m rpfilter' on its own is supposed to be valid, and in fact is used in exactly this style of invocation in the examples included in 'man iptables-extensions'; but support for '-m rpfilter' in this module is limited to uses that include one or more modifying arguments. When this is eventually fixed (which I do not have time to do right now), the updated pre-parse munge logic will work with no further alteration - Adjusting the regex used by String#scan to capture the arguments to treat its capture groups differently SIGNIFICANTLY simplifies the logic around substitutions for the pre-parse munge such that no additional branching is required, and the operation is still safe
Options setting the mode of operation for the 'recent' match extension no longer parse from existing rule definitions with their long-option double-dashes still included. Note that these options are meant to be bang-invertible, but this module does not support that use case (and currently will explode if it encounters any such rule).
greatflyingsteve
force-pushed
the
support-synproxy
branch
from
41c3f55
to
deb2025
Compare
|
Well, that, um...went places. Frustration at the failure of acceptance tests I couldn't run and couldn't see without pushing here boiled over into yanking a bunch of the acceptance test cases over into the unit tests, which then quickly boiled over into frustration that I could not run a significant number of those unit tests because I couldn't effectively mock the kernel or ip[6]tables versions that unlocked the provider feature, which then boiled over into frustration that kind of a shocking number of the new unit tests failed immediately (and sometimes violently), which...you get the idea. ...oh, uh, and you can do all of those things now, by the way; I got all of them to actually work, you can mock kernel and iptables versions in iterative test cases, the tests all pass. That was a fun day-and-a-half foray into the Puppet and RSpec internals, figuring out why my mocked kernel version still wouldn't unlock There are still a fair few features that turn out to be miss-implemented, most of which I've noted in the commit messages where I partially-fixed them, which I still kinda want to get my grubby li'l paws on. But as @bastelfreak pointed out last week, it's not exactly worth the effort, and my employer at this point is probably pretty desperate for me to move on to other things. Oh, also this now accidentally resolves #1083 |
|
Also: I'm happy to split this into multiple PRs if that's preferred. Adding a bunch of the acceptance test cases to unit testing does mean that comes with a caveat: a bunch of the test failures that uncovered while I was working on this PR will show up as failures until a few of the fixes for those test cases are merged. The set of things that make it possible to parse |
|
@greatflyingsteve Apologies for the wait on having this looked at. |
No worries - it appears that I missed a couple of broken things in acceptance tests that had been left broken for one of the other features I fixed in this PR, so it would've needed some rework before merge anyway. Any idea how long before the work to convert it to the resource_api is expected to take? |
Although passing
jump => 'SYNPROXY'is currently allowed already, the SYNPROXY target extension moves a lot of work normally performed by the kernel's TCP stack out into netfilter, and as such needs to be passed a bunch of options so it knows how to handle the fields and options sent in the initial SYN packet, which are canonically unchangeable once the 3WHS is completed.This whole arrangement sounds a little crazy, but it's a means of welding the Linux 4.4+ TCP updates that provided better SYN flood protection into older kernels, without incurring the risk of unintended consequences for all the world's existing non-EOL'd, pre-4.4-based, production-facing Linux deployments. Although that's a dwindling number, there are going to be people migrating to later distro versions that will need to take their existing firewall rulesets with them to decouple OS migration from firewall revision. As a handy side effect, it can also be used to provide SYN flood protection for other back-end systems receiving forwarded traffic. The upshot of all this is that this extension is not likely to go away terribly soon.
As an added bonus, this PR also fixes the configuration for the Metrics/BlockLength cop, so that the max block length doesn't need to be constantly hand-incremented each time the Firewall type is given new features.
Additional info in the commit messages, but please ask about anything I forgot to include or messed up!
Resolves #1125.
Resolves #1083.