New application logging (zerolog) & error handling #75

zc-devs · 2023-07-12T19:50:51Z

Closes #70

Using zerolog for application logging
Introduced application error responses: variables err_code and err_msg - SPOA errors
error used for SPOE/SPOP errors

log/log.go

internal/spoa.go

config.yaml.default

zc-devs · 2023-07-15T12:13:53Z

Now processing error returns immediately as SPOA error instead of SPOE/SPOP timeout error.

log.txt

jcchavezs · 2023-07-20T07:18:41Z

config/config.go

@@ -32,6 +37,14 @@ type Application struct {
 	TransactionActiveLimit     int      `yaml:"transaction_active_limit"`
 }

+// Log is used to manage the SPOA logging.
+type Log struct {
+	Level     string `yaml:"level"`


I don't think we should provide two ways to do login in a connector. Can we stick to tx's logger?

No, we need application (coraza-spoa) logging. We need to log all initialization stuff as well as request/response before transaction is created.

jcchavezs · 2023-07-20T07:20:58Z

internal/spoa.go

-		fileEncoder := zapcore.NewJSONEncoder(pe)
-
-		pe.EncodeTime = zapcore.ISO8601TimeEncoder
+		wafConf := coraza.NewWAFConfig().


I'd prefer we get the platform logging and plug into an implementation coraza can use. For example we use the zap logger created by caddy and plug into coraza https://github.com/corazawaf/coraza-caddy/blob/main/coraza.go#L54

TBH, I had tried it before pushed the PR. It doesn't suit well.

jcchavezs · 2023-07-20T07:22:51Z

Thanks for raising this PR. I think in general I'd prefer we use the logger provided by the platform (in this case spoe) and inject it into coraza config to use a single/consistent logger. This will also prevent us from deviating on the log destination among connector and coraza itself. See for example what we do in caddy https://github.com/corazawaf/coraza-caddy/blob/main/coraza.go#L54 where we reuse the logger produced by caddy and make it pluggable to coraza's interface hence we reduce the configuration.

zc-devs · 2023-07-21T11:26:35Z

As I said before, we need application logger to log stuff not related to request/response processing (transaction, coraza). Related stuff I log via transaction logger (coraza).
It good that Coraza can use external logger, but we can't reach single/consistent logger because of other libraries. At least because of criteo/haproxy-spoe-go now. Moreover, I think we should not.
Suppose, we plug coraza-spoa logger into coraza.
Who (which part) is responsible for configuration?
If it's coraza-spoa, then SecDebugLog and SecDebugLogLevel would be ignored?
If it's coraza, then why is coraza-spoa 's log configured by other application/library?
As I understand, we plug-in only debug logger (which contains errors, I guess). What's about audit log (SecAuditLog)?
As I've tried it, I want to distinguish coraza-spoa logs from underlying libraries logs. When I tried, coraza-spoa debug logs were 'lost' in a lot of coraza's logs.
Ideally, it should be like this. But we have what we have....
Maybe later I will implement loggers by package/'class'.
There are two types of subsystems, I think:

One is internal which is configured by application and some of subsystem configuration may be exposed via application config itself. Configuration properties may differ from subsystem ones. Example: coraza-spoa - haproxy-spoe-go.
Another is one, which is configured by its own config, which we (application) just supply to that subsystem. Example: coraza-spoa - coraza .

To sum up, coraza-spoa / coraza / haproxy-spoe-go is the different subsystems. coraza has its own configuration and should be configured through it.

# Conflicts: # docker/haproxy/haproxy.cfg # internal/spoa.go

sts · 2023-07-30T20:06:24Z

In the current version a configuration error will produce duplicated logs:

{
  "level": "error",
  "error": "invalid WAF config from string: failed to parse string: failed to compile the directive \"secrule\": there is a another rule with id 10102",
  "app": "test",
  "time": "2023-07-29T14:42:14Z",
  "message": "Unable to create WAF instance"
}
{
  "level": "fatal",
  "error": "invalid WAF config from string: failed to parse string: failed to compile the directive \"secrule\": there is a another rule with id 10102",
  "time": "2023-07-29T14:42:14Z",
  "message": "Can't initialize SPOA"
}

How about we just log out the first line and not return in internal/spoa but rather continue?

While in context of an app, the app name should be included, for requests, the request id should also be included in the log line. Maybe this could be done using a hook in a central place? https://github.com/rs/zerolog#hooks

sts · 2023-07-30T20:12:46Z

docker/haproxy/haproxy.cfg

@@ -40,10 +40,14 @@ frontend test_frontend
    http-request silent-drop if { var(txn.coraza.action) -m str drop }
    http-response silent-drop if { var(txn.coraza.action) -m str drop }

-    # Deny in case of an error, when processing with the Coraza SPOA
+    # Deny in case of an error, when processing with the Coraza SPOE


The SPOE is the Haproxy part which talks to the SPOA. So I think this as correct.

HAProxy talks about SPOE/SPOP only.

There are txn.coraza.err_code and txn.coraza.err_msg for SPOA errors now.

# Conflicts: # config.yaml.default # go.mod

zc-devs · 2023-08-04T12:08:24Z

How about we just log out the first line

Fixed.

not return in internal/spoa but rather continue

HAProxy doesn't run if there is error in config, doesn't matter if error is in only one backend and others work. So, I would stick with this behavior.

While in context of an app, the app name should be included

As I sad Maybe later I will implement loggers by package/'class', I would make it in other issue/PR. Both of them introduce new loggers.

for requests, the request id should also be included in the log line

As I use Coraza's DebugLog with Str("transaction_id", tx.ID()) and tx = app.waf.NewTransactionWithID(req.id), seems it's done.

jcchavezs

I really don't see the need to have zerolog and logrus in here. Do we need both and also a log package? I would prefer to use coraza logger (embedded in the transaction) and std logger for those cases where it is not possible and from there explore the possibility of retrieving the logger from the WAF object or do a recommendation. The reason is this is the only connector where I see we need to introduce new loggers.

zc-devs changed the title ~~New application logging (zerolog)~~ Draft: New application logging (zerolog) Jul 12, 2023

zc-devs marked this pull request as draft July 12, 2023 19:53