[Ferm] Inaccuracy about subchain concept

Wed Sep 18 21:09:02 CEST 2013

* [Tue, Aug 27, 2013 at 09:12:56AM +0200] Gian Piero Carrubba:
>>Attached you can find a draft patch

While looking at polishing this patch I've produced some other patches, 
that are attached. There are dependencies between some of them, so some 
work may be required in case of cherry-picking.
For convenience, at the end of the mail there's a description of the 
patches.

Back to the subject.
The patch is working, with the additional constraint that a subchain is 
not allowed to define a different protocol from the inherited one. This 
could be worked around, but I wonder what's the point in allowing it.

Anyway there still are some tricky situations:

     chain test-tcp proto tcp @subchain test-sub { tcp-option 1 NOP; }
     chain test-udp proto udp jump test-sub;

produces:

     test-sub --protocol tcp --tcp-option 1
     -A test-tcp --protocol tcp --jump test-sub
     -A test-udp --protocol udp --jump test-sub

so that the packet will never match if entered the test-sub chain from 
the test-udp chain. This specific case may not be a big surprise for the 
user, given that he has explicitly typed 'tcp-option'. Anyway, the 
following:

     chain test-tcp proto tcp @subchain test-sub { dport 1 NOP; }
     chain test-udp proto udp jump test-sub;

produces:

     -A test-sub --protocol tcp --dport 1
     -A test-tcp --protocol tcp --jump test-sub
     -A test-udp --protocol udp --jump test-sub

and as a user I'm pretty sure this is not what I intended.

All in all, my opinion is that the inheritance of the protocol in a 
subchain should be removed as it produces actions at a distance. As an 
alternative, the man page should make it clear that a subchain should 
never be reused.

Ciao,
Gian Piero.

1: Wed Sep 18 19:51:05 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Tests for 'Conditionally enable the inherited protocol in a subchain'

2: Wed Sep 18 19:16:51 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Conditionally enable the inherited protocol in a subchain
   A subchain unconditionally inherit the protocol from the outer rule, even if no
   keyword in the subchain requires a protocol to be set.
   This patch enables the protocol match inside the subchain only if there's a
   real need for it.
   For example:
       chain 'outer' proto tcp
           @subchain 'middle' {
               daddr 1.1.1.1
                   @subchain 'inner' { dport 1 NOP }
           }
   produces:
       -A inner --protocol tcp --dport 1
       -A middle --destination 1.1.1.1 --jump inner
       -A outer --protocol tcp --jump middle

3: Tue Sep 17 16:59:03 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Add test case for 'Prevent multiple protocols in the same rule'

4: Tue Sep 17 16:48:56 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Add makefile target 'check-error'
   This target allows the definition of tests that should result in an error
   thrown by ferm.
   Such tests need two files that must reside in the usual directories:
   - a '.ferm.error' file containing the ferm script
   - a '.result.error' file containing the expected error (printed to STDERR)
   The '.result.error' file must not contain the first line of the generated error
   (tipically: 'Error in <file> line <line>:').

5: Tue Sep 17 14:54:50 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Sort hash keys when iterating
   Perl 5.18 introduced true hash randomization, so that iterating over the "same"
   hash produces different results on different runs.
   This patch let the hash keys be sorted alphabetically during iterations, so
   that, given the same input, ferm produces the same output over different runs
   and different perl versions.
   The primary consequence is that the test suite does not fail anymore with perl
   5.18 (removal of false negatives), but more in general this means that the
   output is predictable. The latter is the reason for fixing also the iterators
   that does not have impact on the result of the test suite.
   On the other side, the string comparison order seems sub-optimal from a user
   point of view, that would probably prefer the output to follow the same order
   as given in the input file. Anyway, given that neither before the order in the
   input file was respected, this patch doesn't introduce a regression. Sensibly
   fixing this point could require an intrusive rewriting of the ferm logic (not
   verified).

6: Mon Sep 16 20:50:06 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Fix test suite
   Modify expected results in order for the tests to pass after applying the 'Sort
   hash keys when iterating' patch.

7: Tue Sep 17 11:57:47 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Prevent multiple protocols in the same rule
   The following doesn't seem to have sense:

   $ echo 'chain test proto udp proto tcp proto whatever NOP;' |
   > ferm --test --noflush -
   *filter
   :test - [0:0]
   -A test --protocol udp --protocol tcp --protocol whatever
   COMMIT

8: Tue Sep 17 11:20:59 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Typo fix

9: Mon Sep 16 11:22:39 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Useless variable $error in rollback() function
   Remove $error as it is never modified after declaration (undef) and it's only
   used once in a unless($error) conditional that always succeeds.

10: Mon Sep 16 11:17:39 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * [MAYBE] Inlining functions checks the wrong array during variable substitutions
   Why using parenthesis around @value only if @tokens != 1 ?
   @tokens is always > 1 at this stage (it includes at least '$' and var name), so
   the check always succeeds.
   It seems more sensible using parenthesis around @value iff *@value* is != 1 (or
   unconditionally use parenthesis, as it seems it's acting now, without the
   useless check).
   Marking [MAYBE] as I could miss something.

11: Sun Sep 15 12:46:46 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * Typo fix

12: Sun Sep 15 12:21:40 CEST 2013  Gian Piero Carrubba <gpiero at rm-rf.it>
   * git/master/fd7dc27cd24d831a95e6b3469283e991435c3895
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01_Tests_for__Conditionally_enable_the_inherited_protocol_in_a_subchain__.patch.gz
Type: application/octet-stream
Size: 1499 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02_Conditionally_enable_the_inherited_protocol_in_a_subchain_.patch.gz
Type: application/octet-stream
Size: 1414 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment-0001.obj>
-------------- next part --------------
‹í9R03_Add_test_case_for__Prevent_multiple_protocols_in_the_same_rule__.patch µ“ÝJÃ@…¯ÍSœ»‚é¦»Ikm,)â]-´/4È“]‹oï¦ÚZ¯DƒË°ì˜s>fg[j¡æP7álÊ «Çí¾TðTÄ56qƒUÌl’$Æò¥í…ˆ+Á™Wè{¸ÆCšBS§±;BÖ0F¦Wª5*Sê¢-	-7ºÙ7e‡¢†Î	]\Ø”4rÒ"Ë x
aÐ”©èL²Ïã¢Ç¦Io=9÷ON–â,yqå³M®é0¤ßBáJ-æRHeR…RÚòäéÀ•JJÇuÝ¡³^õœ„\uk‘…Ê§3oîÓ™òƒ>Ç·9Q!Çö§EŽ{Ìú€u´„IÛÏ×!µ…ÆX?oî†3aê¬8„Ê¥Ãß¹\ºü/™oI¿c3ö¢³Âqw91Ù-«‘šº|³××j¡µðúrÞOB£øÚ  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 04_Add_makefile_target__check_error__.patch.gz
Type: application/octet-stream
Size: 1013 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 05_Sort_hash_keys_when_iterating_.patch.gz
Type: application/octet-stream
Size: 1731 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 06_Fix_test_suite_.patch.gz
Type: application/octet-stream
Size: 981 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment-0004.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 07_Prevent_multiple_protocols_in_the_same_rule_.patch.gz
Type: application/octet-stream
Size: 540 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment-0005.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 08_Typo_fix_.patch.gz
Type: application/octet-stream
Size: 319 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment-0006.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 09_Useless_variable__error_in_rollback___function_.patch.gz
Type: application/octet-stream
Size: 561 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment-0007.obj>
-------------- next part --------------
‹í9R10__MAYBE__Inlining_functions_checks_the_wrong_array_during_variable_substitutions_.patch SaoÓ0ýLÅcª”´Iº¤-ëèÈ˜&´CHCBhÈMÜÖšcW¶³RMã·sîàK6ò%‰ïî½wïÎWZáš¯‘‘eÓl2ãüâú†i6>
¦ðYp£qÎŒ©çs†wËµ?ÈM•˜Å@¸ÓÐÇÍÕÙ··¸TR(¡–XÔªpB+‹bÅ‹;·âØM!Bb[”µñy÷Ì6—¶ž['\½+"Ì¯«-jëSÖÌpEåVXªÕµ*‘ß3Ysh%·äNßq¢z=C†÷TÜø
¹a[‹SŠ0G*èÈ:¶ä…ƒP…¬Kn}Hrf‚n F¤ŠU¼ÃjBôòw4€¶.
ÎK; à¥ƒå¼²¨´¡NˆYø–^’/ôŸ¾û^êN~¨
!’{Z•Â›Á¤ôNðb0j®a. h²›Ø”ÞÄØ·Òµï–{,Ë%·ûAô¼Þ+fî|r35»D¡kY¢”iuÅÉ)µtJ/21ŸÔÐ²LhLÅŠ	•¬vúÐšâpÁMÅ7m±N’$ÏÕ¾òË–¤ÇIö–önš
§GãÁ$ËŽÆoÒ£¢t˜¦(Šžãø/Œ<G2L'ãx‚hÿÎs²£åá?…u]ZûÐ}Ú¨›®@„ìö±‡‡öÊjÛŒx§ì>lÁ9i‡ùƒaïc½ ×IZ«hwvãnîÀŒ¶ê¤½œ¿gÛ¥·&Ûµ÷à1º"Æ°Ñö\3¾áYC’`Ø’ú.-]éð¯Q·˜ýB?ýÞA7öÖÿMîÝ·¹  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 11_Typo_fix_.patch.gz
Type: application/octet-stream
Size: 304 bytes
Desc: not available
URL: <http://foo-projects.org/pipermail/ferm/attachments/20130918/7e094401/attachment-0008.obj>