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

SecGeoLookupDb directive support #170

Closed
wants to merge 12 commits into from
Closed

SecGeoLookupDb directive support #170

wants to merge 12 commits into from

Conversation

syinwu
Copy link
Member

@syinwu syinwu commented Feb 18, 2022

Thank you for contributing to Coraza WAF, your effort is greatly appreciated
Before submitting check if what you want to add to coraza list meets quality standards before sending pull request. Thanks!

Note: that go.mod and go.sum can only be modified for tested dependency updates or justified new features.

Make sure that you've checked the boxes below before you submit PR:

Thanks for your PR ❤️

@syinwu syinwu requested a review from jptosso February 18, 2022 02:50
Copy link
Member

@jptosso jptosso left a comment

Choose a reason for hiding this comment

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

We should review waf.GeoIPDB and if the numeric operators supports floats

@@ -34,11 +34,11 @@ func (o *eq) Init(data string) error {
}

func (o *eq) Evaluate(tx *coraza.Transaction, value string) bool {
d1, err := strconv.Atoi(o.data.Expand(tx))
d1, err := strconv.ParseFloat(o.data.Expand(tx), 64)
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure ge, eq, lt, etc... support floats instead of ints?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah! Using float64 can support the comparison of positive and negative numbers, and the memory occupied on 64 bit machines is the same as that of int type(most machines should be 64 bit)

operators/geo_lookup.go Outdated Show resolved Hide resolved
operators/geo_lookup_test.go Outdated Show resolved Hide resolved
@syinwu syinwu requested a review from jptosso February 21, 2022 08:12
@sonarcloud
Copy link

sonarcloud bot commented Feb 22, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

86.4% 86.4% Coverage
0.0% 0.0% Duplication

Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Looks good!

Do we need to push the testdata to this repo? Can't we do this in the Github Actions PIpeline? I'm worried about making this repo too heavy...

@fzipi
Copy link
Member

fzipi commented Feb 26, 2022

Also, looks like there are some conflicts in the go.mod file 🤔

@jptosso
Copy link
Member

jptosso commented Feb 28, 2022

Can we transform this into a plugin until we find how to manage the databases? It will be an official plugin with official support.

I think we must keep it simple, in the meantime we can use the waf.Config.Get("geoip") interface to store the database for each engine (one plugin per engine)

I have created this as a sample: https://github.com/corazawaf/coraza-geoip

@syinwu
Copy link
Member Author

syinwu commented Mar 1, 2022

Can we transform this into a plugin until we find how to manage the databases? It will be an official plugin with official support.

I think we must keep it simple, in the meantime we can use the waf.Config.Get("geoip") interface to store the database for each engine (one plugin per engine)

I have created this as a sample: https://github.com/corazawaf/coraza-geoip

Sure! Thank you very much!

@syinwu syinwu closed this Mar 1, 2022
@syinwu syinwu deleted the v2/add-geoip branch March 8, 2022 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The GeoIP and Dos Protection in coreruleset crs-setup.conf is not supported in coraza WAF
3 participants