Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concatenating SRX-specific options in aclcheck.py #216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dark1587
Copy link

When running aclcheck_cmdline.py, I ran into an issue when looking at an example SRX Filter:

chaynes@Clays-MBP test-policy % python3 ./aclcheck_cmdline.py --destination-port 6666 -s 172.16.0.0/12 -d 10.1.2.3/32 --protocol tcp  -p ./policies/pol/test.pol
  filter: from-zone
          term: ALLOW_PRIVATE
                accept
          term: PERMIT_TRUST
                accept
          term: DENY_PROD
                deny

Looking at the aclcheck.py code I saw this statement:

filtername = header.target[0].options[0]

Now for the most common filters, this would work fine, as most of them are only a single word and easily-split, as shown below:

header {
  comment:: "Example header for juniper and iptables filter."
  target:: juniper edge-filter
  target:: speedway INPUT
  target:: iptables INPUT
  target:: cisco edge-filter
}

However, on the SRX it uses multiple words to express the source and destination zone an ACL should apply to:

chaynes@Clays-MBP test-policy % cat policies/pol/my-custom.pol 
header {
  target:: srx from-zone TRUST to-zone UNTRUST
}

So what this does is attempt check to see target inside of the header is SRX, and if it is, concatenate all the words together with a space . By doing this, the test behaves much better:

chaynes@Clays-MBP test-policy % python3 ./aclcheck_cmdline.py --destination-port 6666 -s 172.16.0.0/12 -d 10.1.2.3/32 --protocol tcp  -p ./policies/pol/test.pol
  filter: from-zone UNTRUST to-zone TRUST
          term: ALLOW_PRIVATE
                accept
  filter: from-zone TRUST to-zone TRUST
          term: PERMIT_TRUST
                accept
  filter: from-zone TRUST to-zone UNTRUST
          term: DENY_PROD
                deny

I'm not much of a programmer, so I'm completely open to different ideas. Perhaps this could become the default for all filters, as Junos does require a unique filter for both family inet and family inet6?

Anyways, let me know what you think and if you have any further questions. Thanks!

@google-cla google-cla bot added the cla: yes label Aug 23, 2020
@nero85
Copy link
Contributor

nero85 commented Jan 6, 2022

Please add a unit test to tests/lib/aclcheck_test.py for this specific change. We have not touched this code for a while. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants