Add logger-handler to firewall #151

storojs72 · 2018-04-06T07:56:03Z

No description provided.

Lagovas

add please saving state on changes
reloading forbidden queries now can wait

Lagovas · 2018-04-06T09:13:52Z

firewall/firewall_test.go

+	loggingHandler.MarkQueryAsForbidden(testQueries[1])
+	loggingHandler.MarkQueryAsForbidden(testQueries[2])
+
+	blacklist.AddQueries(loggingHandler.GetForbiddenQueries())


Lagovas · 2018-04-06T09:16:49Z

firewall/handlers/logging_handler.go

+	"encoding/json"
+)
+
+type LoggingHandler struct {


what about to add filepath field and NewLoggingHandler function that will return new handler with loaded queries from file?

Lagovas · 2018-04-06T09:18:32Z

firewall/handlers/logging_handler.go

+	queryInfo := &QueryInfo{}
+	queryInfo.rawQuery = query
+	queryInfo.isForbidden = false
+	handler.Queries = append(handler.Queries, *queryInfo)


will be better to save new queries to file to commit current state of handler

Lagovas · 2018-04-06T09:19:24Z

firewall/handlers/logging_handler.go

+func (handler *LoggingHandler) MarkQueryAsForbidden(query string) {
+	for index, queryInfo := range handler.Queries {
+		if strings.EqualFold(query, queryInfo.rawQuery) {
+			handler.Queries[index].isForbidden = true


here will be good too to save new state to file

Lagovas · 2018-04-06T09:20:55Z

firewall/handlers/logging_handler.go

+func (handler *LoggingHandler) GetAllInputQueries() []string{
+	var queries []string
+	for _, queryInfo := range handler.Queries {
+		queries = append(queries, queryInfo.rawQuery)


in a future would be great to check file update on new queries and if file was updated reload and return new values
it will be useful when separate instance of webui will add changes and acraserver's handler should update too without restarting

Lagovas · 2018-04-12T09:31:11Z

cmd/acraserver/config.go

-func (config *Config) SetFirewall(fw firewall.FirewallInterface) {
-	config.firewall = fw
+func (config *Config) SetFirewall(censorConfigPath string) error {
+	firewall := &firewall.Firewall{}


better to assign one time config.firewall = &firewall.Firewall{} instead 5 extra lines

Lagovas · 2018-04-12T09:36:18Z

firewall/handlers/logging_handler.go

+			handler.Queries[index].IsForbidden = true
+		}
+	}
+	handler.Serialize()


what if error?

Lagovas · 2018-04-12T09:36:47Z

firewall/handlers/logging_handler.go

+
+func (handler *LoggingHandler) Serialize() error {
+	jsonFile, err := json.Marshal(handler.Queries)
+	err = ioutil.WriteFile(handler.filePath, jsonFile, 0600)


you can return result because it err itself

Lagovas · 2018-04-12T09:37:07Z

firewall/handlers/logging_handler.go

+	if err != nil {
+		return err
+	}
+	json.Unmarshal(bufferBytes, &handler.Queries)


err := json.Unmarshal(...)

Lagovas · 2018-04-12T09:37:24Z

firewall/handlers/logging_handler.go

+		return err
+	}
+	json.Unmarshal(bufferBytes, &handler.Queries)
+	if err != nil {


here you check ReadFile call

vixentael · 2018-04-12T09:55:24Z

Please add info log to see if query was allowed/filtered here:
https://github.com/cossacklabs/acra/blob/master/firewall/firewall_implementation.go#L23

And show example of json output

vixentael · 2018-04-12T13:00:59Z

acracensor/acracensor_implementation.go

+}
+
+func (acraCensor *AcraCensor) HandleQuery(query string) error {
+	log.Infof("Censor works")


we will receive this log message on every query. which is not very useful, i'd remove it

vixentael · 2018-04-12T13:05:51Z

acracensor/acracensor_implementation.go

+	for _, handler := range acraCensor.handlers {
+		log.Infof("Handler: %s", handler.GetName())
+		if err := handler.CheckQuery(query); err != nil {
+			return err


log.WithFields(log.Fields{"acracensor" : handler.GetName()}).Infof("Forbidden query: %s", query)

vixentael · 2018-04-12T13:06:22Z

acracensor/acracensor_implementation.go

+func (acraCensor *AcraCensor) HandleQuery(query string) error {
+	log.Infof("Censor works")
+	for _, handler := range acraCensor.handlers {
+		log.Infof("Handler: %s", handler.GetName())


i think we should remove this

vixentael · 2018-04-12T13:06:56Z

acracensor/acracensor_implementation.go

+		log.Infof("Handler: %s", handler.GetName())
+		if err := handler.CheckQuery(query); err != nil {
+			return err
+		}


log.WithFields(log.Fields{"acracensor" : handler.GetName()}).Infof("Allowed query: %s", query)

vixentael · 2018-04-12T13:08:14Z

cmd/acraserver/config.go

@@ -50,7 +51,7 @@ type Config struct {
 	postgresql              bool
 	configPath              string
 	debug                   bool
-	firewall                firewall.FirewallInterface
+	firewall                acracensor.AcracensorInterface


censor instead of firewall?

Agree. Renamed

vixentael · 2018-04-12T13:08:37Z

logging/event_codes.go

@@ -52,8 +52,9 @@ const (
 	EventCodeErrorCantParseAuthData       = 557
 	EventCodeErrorCantDumpConfig          = 558

-	// firewall
+	// acracensor
 	EventCodeErrorFirewallQueryIsNotAllowed = 560


censor instead of firewall?

Agree. Renamed

Lagovas · 2018-04-12T14:32:03Z

cmd/acraserver/config.go

+	if err != nil {
+		return err
+	}
+	config.censor = acraCensor


unnecessary

Lagovas · 2018-04-12T14:32:44Z

acracensor/acracensor_implementation.go

+func (acraCensor *AcraCensor) HandleQuery(query string) error {
+	for _, handler := range acraCensor.handlers {
+		if err := handler.CheckQuery(query); err != nil {
+			return err


log error. will be good to log handler name too

storojs72 added 3 commits April 6, 2018 10:48

Add logging handler to firewall

327c974

remove debug printers

e770e80

Merge branch 'master' of github.com:cossacklabs/acra

f272487

vixentael requested a review from Lagovas April 6, 2018 08:01

vixentael added the acra-censor label Apr 6, 2018

Lagovas requested changes Apr 6, 2018

View reviewed changes

storojs72 added 4 commits April 12, 2018 09:39

Merge branch 'master' of github.com:cossacklabs/acra

959567f

Refactor SetFirewall stub

2a88e24

Update firewall logger

614e3d6

circleci fail fix

ab042a5

Lagovas requested changes Apr 12, 2018

View reviewed changes

storojs72 added 3 commits April 12, 2018 13:16

Renaming firewall <-> acracensor

6c6079b

LoggingHandler requested changes

d4c58cd

Some refactoring + Add censor logging to main acraserver log stream

c9c82e3

storojs72 requested a review from vixentael April 12, 2018 11:43

firewall -> censor

62c42e5

vixentael reviewed Apr 12, 2018

View reviewed changes

storojs72 added 3 commits April 12, 2018 16:36

Requested changes

ee1325b

firewall <-> censor

d999744

changing censor logging

ac86bed

storojs72 requested a review from mozhmike April 12, 2018 13:42

mozhmike approved these changes Apr 12, 2018

View reviewed changes

Lagovas requested changes Apr 12, 2018

View reviewed changes

Lagovas approved these changes Apr 12, 2018

View reviewed changes

storojs72 added 2 commits April 12, 2018 17:47

Requested changes

f120687

Add raw query string to main log stream

25bb3f6

vixentael approved these changes Apr 12, 2018

View reviewed changes

update acraserver yaml config

9aa0b07

vixentael merged commit de97f23 into cossacklabs:master Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logger-handler to firewall #151

Add logger-handler to firewall #151

storojs72 commented Apr 6, 2018

Lagovas left a comment

Lagovas Apr 6, 2018

Lagovas Apr 6, 2018

Lagovas Apr 6, 2018

Lagovas Apr 6, 2018

Lagovas Apr 6, 2018

Lagovas Apr 12, 2018

Lagovas Apr 12, 2018

Lagovas Apr 12, 2018

Lagovas Apr 12, 2018

Lagovas Apr 12, 2018

vixentael commented Apr 12, 2018

vixentael Apr 12, 2018

vixentael Apr 12, 2018 •

edited

vixentael Apr 12, 2018

vixentael Apr 12, 2018

vixentael Apr 12, 2018

storojs72 Apr 12, 2018 •

edited

vixentael Apr 12, 2018

storojs72 Apr 12, 2018 •

edited

Lagovas Apr 12, 2018

Lagovas Apr 12, 2018

Add logger-handler to firewall #151

Add logger-handler to firewall #151

Conversation

storojs72 commented Apr 6, 2018

Lagovas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vixentael commented Apr 12, 2018

Choose a reason for hiding this comment

vixentael Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storojs72 Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storojs72 Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vixentael Apr 12, 2018 •

edited

storojs72 Apr 12, 2018 •

edited

storojs72 Apr 12, 2018 •

edited