Skip to content

fortigate firewall policy support - #173

Closed
Ali-aqrabawi wants to merge 13 commits into
google:masterfrom
Ali-aqrabawi:feature/fortigate_support
Closed

fortigate firewall policy support#173
Ali-aqrabawi wants to merge 13 commits into
google:masterfrom
Ali-aqrabawi:feature/fortigate_support

Conversation

@Ali-aqrabawi

@Ali-aqrabawiAli-aqrabawi commented May 28, 2019

Copy link
Copy Markdown

in this pull request i added support for fortigate policies.

example .pol

header {
target:: fortigate from-id 2
target:: paloalto from-zone internal to-zone external
target:: juniper edge-filter
target:: cisco edge-filter
}
term allow-https-web {
destination-address:: WEB_SERVERS
destination-port:: WEB_SERVICES
protocol:: tcp
action:: accept
}
term customers-policy {
source-address:: CUSTOMERS
destination-address:: GOOGLE_PUBLIC_DNS_ANYCAST
destination-port:: WEB_SERVICES
protocol:: tcp
action:: deny
}
term customers-policy2 {
#source-interface:: wan1
#destination-interface:: wan2
source-address:: ALI_HOME
destination-address:: CRM
destination-port:: WEB_SERVICES
protocol:: tcp
action:: deny
logging:: true
}
term deny-any-any {
action:: deny
}
term deny-icmp {
action:: deny
protocol:: icmp
}
term accept-whois {
protocol:: udp
destination-port:: WHOIS
action:: accept
}

@ankenyrankenyr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to create tests for this before we can accept it. I provided a bunch of lint issues and a few general code comments. So far this is looking pretty good. Thank you for all the work on this!

Comment threadcapirca/aclgen.py
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py
Comment threadcapirca/lib/fortigate.py
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py
Comment threadcapirca/lib/fortigate.py Outdated
@Ali-aqrabawi

Ali-aqrabawi commented Jun 12, 2019

Copy link
Copy Markdown
Author

@ankenyr i have added unittest and resolved the code reviews.
for #173 (comment), i have create a Container object to hold the grouped commands like service-objects commands and network-objects commands

@ankenyrankenyr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will want to familiarize yourself with the style guide for python code owned by Google.
https://github.com/google/styleguide/blob/gh-pages/pyguide.md
80 characters is the maximum line length, indentation and line returns in correct places. Please review your code for adhereance to the standards. Resolve all comments that are outstanding and let me know when you are ready for the next pass through your code.

Comment threadcapirca/aclgen.py Outdated
@ankenyr

Copy link
Copy Markdown
Contributor

Hi Ali, I have not heard anything on this PR in a while. There are still pending changes requested.

@Ali-aqrabawi

Copy link
Copy Markdown
Author

Hi Rob, i have done the requested changes here, did you check it?

@ankenyr

Copy link
Copy Markdown
Contributor

sorry but I don't see the comments as resolved which is why I never checked on anything.
https://imgur.com/KwyGS0O
Can you make sure all the comments are resolved and then I can make another pass?

@Ali-aqrabawi

Copy link
Copy Markdown
Author

done

Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadtests/lib/fortigate_test.py Outdated
Comment threadtests/lib/fortigate_test.py
Comment threadtests/lib/fortigate_test.py Outdated
Comment threadtests/lib/fortigate_test.py Outdated
Comment threadtests/lib/fortigate_test.py Outdated
@ankenyr

Copy link
Copy Markdown
Contributor

Hey @Ali-aqrabawi did you have time to fix the remaining issues here?

@Ali-aqrabawi

Copy link
Copy Markdown
Author

@ankenyr i'm working on it

@Ali-aqrabawi

Copy link
Copy Markdown
Author

@ankenyr everything should be fixed now

@Ali-aqrabawi
Ali-aqrabawi requested a review from ankenyrMay 19, 2020 22:06
Comment threadcapirca/lib/fortigate.py
Comment threadcapirca/lib/fortigate.py

@ankenyrankenyr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, just a few small nitpick changes. Sorry this review has lasted so long. I was on paternity for a while and then some hecktic work responding to covid stuff and family issues. If you can fix these quick I can pull this in asap.

Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
@Ali-aqrabawi
Ali-aqrabawi requested a review from ankenyrJune 4, 2020 21:53
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadcapirca/lib/fortigate.py Outdated
Comment threadtests/lib/fortigate_test.py Outdated
Comment threadtests/lib/fortigate_test.py
Comment threadtests/lib/fortigate_test.py
Comment threadtests/lib/fortigate_test.py Outdated
Comment threadtests/lib/fortigate_test.py Outdated
Comment threadtests/lib/fortigate_test.py
Comment threadtests/lib/fortigate_test.py
Comment threadtests/lib/fortigate_test.py
Comment threadtests/lib/fortigate_test.py
Comment threadtests/lib/fortigate_test.py Outdated
Comment threadcapirca/lib/fortigate.py
@ankenyr

Copy link
Copy Markdown
Contributor

Ali, there are still changes that have not been made that I have requested.

@Ali-aqrabawi

Copy link
Copy Markdown
Author

Ali, there are still changes that have not been made that I have requested.

i checked the requested changes couple of times can't find which part i'm missing, sorry :/

@Slepwin

Copy link
Copy Markdown

Any updates here?

@nero85

Copy link
Copy Markdown
Contributor

We are closing this in preference for #222. Thanks for your input!

@nero85nero85 closed this Jan 6, 2022
Sign up for freeto 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.

5 participants

@Ali-aqrabawi@ankenyr@Slepwin@nero85@googlebot