missing hostname in logs

[M4tteoP]

Hi there,
taking a look at #515, indeed resolvedIP variable is never populated.
To my eyes seems that the right value could be the ServerIPAddress, populated at the beginning of the transaction, when ProcessConnection is called:

coraza/internal/corazawaf/transaction.go

Line 638 in bffb435

tx.variables.serverAddr.Set(server)

Still, taking a look at the connector, this fix would just populate the field with the IP and may not be useful for @mac-chaffee usecase.
If it sounds reasonable I can, as proposed, catch the Host header adding it to a new tx.variables, but I'm wondering about sanitizing the retrieved value 🤔.

Attempt to close #515.

[mac-chaffee]

It does look like modsecurity sets that badly-named "hostname" variable to an IP address: https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/rule_message.cc#L45

Now that I think of it, I normally find out the real hostname by looking at the modsecurity audit logs which includes the full HTTP request that triggered the rule. Doesn't seem like that kind of audit logging exists in Coraza yet, so I do like the idea of an extra section in the existing logs in the meantime.

I'm not too worried about sanitization since Coraza is designed to log attack attempts anyway.

[jcchavezs]

@jptosso @fzipi could you chime in?

[fzipi]

@mac-chaffee Depends on the tool you are using to look at logs. I would say we should sanitize it before sending it out to a filesystem/tool. More information here.

[fzipi]

I think that resolveIP was meant to get the actual name from the ServerIPAddress(). I don't know if we want to do an extra name resolution before logging, but if someone needs it we might use a config parameter like HostnameLookups Off from Apache.

So the quick solution is to use the ServerIPAddress as this PR proposes for the resolveIP (probably adding a comment also), and in the future we can add configurable reverse resolution if needed.

[fzipi]

I think we can merge this one and create a follow up ticket.

[codecov-commenter]

Codecov Report

Base: 78.44% // Head: 78.43% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (f55f705) compared to base (6470648).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           v3/dev     #517      +/-   ##
==========================================
- Coverage   78.44%   78.43%   -0.01%     
==========================================
  Files         145      145              
  Lines        6981     6980       -1     
==========================================
- Hits         5476     5475       -1     
  Misses       1274     1274              
  Partials      231      231              

Flag	Coverage Δ
default	73.86% <100.00%> (-0.01%)	⬇️
examples	27.37% <100.00%> (-0.02%)	⬇️
ftw	56.59% <100.00%> (-0.01%)	⬇️
tinygo	71.98% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/corazarules/rule_match.go	49.53% <100.00%> (-0.47%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[M4tteoP]

FYI, related conversation ModSecurity side: owasp-modsecurity/ModSecurity#2906