Updating firewall: use config file to set up rules #138
Conversation
| @@ -29,65 +32,44 @@ func (firewall *Firewall) SetFirewallConfiguration(configuration string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func updateFirewall(firewall *Firewall, configuration string) error { | |||
| func updateFirewall(firewall *Firewall, configuration []byte) error { | |||
There was a problem hiding this comment.
why is it a function instead method? func (firewall *Firewall) update(configuration []byte)error.
firewall/firewall_implementation.go
Outdated
| @@ -27,3 +29,9 @@ func (firewall *Firewall) HandleQuery(query string) error{ | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func (firewall *Firewall) PrintStatus() { | |||
There was a problem hiding this comment.
This is for debug purposes. Removed
| configLineType = structureConfigHeader | ||
| break; | ||
| case "\n": | ||
| whitelistHandler.AddQueries(handlerConfiguration.Queries) |
There was a problem hiding this comment.
what about check queries on add operation and return error here? then we doesn't need extra function like testConfigurationSyntex that will grow with each new rule and handler types
There was a problem hiding this comment.
If we have separate function to test syntax, we are able to modify it if necessary. And we can't do it if logic of syntax checking will be scattered over multiply functions
There was a problem hiding this comment.
Finally, I agree. It also helps to reduce handler public interface
| } | ||
| } | ||
|
|
||
| func (firewall *Firewall) SetFirewallConfiguration(configuration []byte) error { |
There was a problem hiding this comment.
what about firewall.SetFirewallConfiguration -> firewall.LoadConfiguration?
| return nil | ||
| } | ||
|
|
||
| func updateFirewall(firewall *Firewall, configuration []byte) error { |
There was a problem hiding this comment.
what about updateFirewall(firewall *Firewall, ...) -> func (firewall *Firewall) update(...)?
| //Check tables | ||
| if len(handler.tables) != 0 { | ||
| parsedQuery, err := sqlparser.Parse(query) | ||
| if err != nil { | ||
| return err | ||
| return errors.New(err.Error() + " | blacklist (tables)") |
There was a problem hiding this comment.
bad pattern to concatenate strings via "+" operator. And bad way to create error in such way. Better to add new type of error. Then we can easily check is it correct error type in unit-tests without string comparison.
There was a problem hiding this comment.
Agree. Refactored
firewall/firewall_interfaces.go
Outdated
| AddRules(rules []string) | ||
| RemoveRules(rules []string) | ||
|
|
||
| GetActiveQueries() []string |
There was a problem hiding this comment.
Our handler interface grow and grow and in a future will be very big monolitic interface. But at start it was designed like simple abstract interface for different handlers with simple function CheckQuery and encapsulated logic in custom handler itself.
Anyway what mean Active in name? Now it's just added queries/tables/rules to handler. So will be enough to name GetQueries. Because active tell us that handler has some different queries too
There was a problem hiding this comment.
I agree with your thought about handler interface. Actually we can remove some functions from it to make it simpler
| } | ||
| } | ||
|
|
||
| case *sqlparser.Update: | ||
|
|
||
| return errors.New("not implemented yet") |
There was a problem hiding this comment.
add new error like ErrNotImplemented and then in a future we can use it in another handlers
firewall/handlers/handlers_util.go
Outdated
| var ErrAccessToForbiddenTable = errors.New("query tries to access forbidden table") | ||
| var ErrForbiddenSqlStructure = errors.New("query's structure is forbidden") | ||
|
|
||
| var ErrAccessToForbiddenTableBlacklist = errors.New("query tries to access forbidden table | blacklist") |
There was a problem hiding this comment.
no need to add | blacklist at end of message for ErrAccessToForbiddenTableBlacklist error
firewall/test_firewall_rules
Outdated
| @@ -0,0 +1,19 @@ | |||
| [handler] | |||
There was a problem hiding this comment.
No. We don't. Removed
| } | ||
|
|
||
| func (handler *WhitelistHandler) RemoveRules(rules []string){ | ||
| func (handler * WhitelistHandler) RemoveRules(rules []string){ |
There was a problem hiding this comment.
gofmt will not agree with your space between * and type :)
please, run go fmt ./... in repo folder before pushing or we will not so excellent golang repo as now (https://goreportcard.com/report/github.com/cossacklabs/acra) :((
| @@ -56,7 +56,7 @@ func (firewall *Firewall) update(configuration []byte) error { | |||
| } | |||
|
|
|||
| firewallCheckers = append(firewallCheckers, whitelistHandler) | |||
There was a problem hiding this comment.
why not add handler here? instead of second loop after this
vixentael
changed the title
Add ability to configure firewall from configuration file