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

add https audit log support #826

Merged
merged 8 commits into from
Jul 11, 2023
Merged

add https audit log support #826

merged 8 commits into from
Jul 11, 2023

Conversation

jptosso
Copy link
Member

@jptosso jptosso commented Jun 26, 2023

Following discussion #813, implements HTTPS audit log writer

  • MLOGC compatibility was dropped
  • We are not adding new headers
  • We send different mime for each formatter

Coraza v4 should consider improvements on this, like additional settings

@jptosso jptosso requested a review from a team as a code owner June 26, 2023 07:18
@jptosso jptosso marked this pull request as draft June 26, 2023 07:18
@jcchavezs
Copy link
Member

Unfortunatelly we can't do bathing as we can't close the exporter (WAF does not support close method and hence when terminating the APP the batched requests will be lost). I think it is a good idea to add a Close method now but not to force people to use it and maybe the exporter can listen to termination signal cc @anuraaga ?

@jcchavezs
Copy link
Member

Decisions we made during meeting https://owasp.slack.com/archives/C02BXH135AT/p1687955362171029

@jcchavezs
Copy link
Member

Any movement @jptosso?

@jptosso jptosso marked this pull request as ready for review July 10, 2023 08:23
@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Patch coverage: 43.90% and project coverage change: -0.01 ⚠️

Comparison is base (cc4ec80) 81.57% compared to head (42017a4) 81.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
- Coverage   81.57%   81.56%   -0.01%     
==========================================
  Files         158      159       +1     
  Lines        8959     9000      +41     
==========================================
+ Hits         7308     7341      +33     
- Misses       1406     1412       +6     
- Partials      245      247       +2     
Flag Coverage Δ
default 76.58% <43.90%> (+0.01%) ⬆️
examples 25.54% <0.00%> (-0.13%) ⬇️
ftw 46.89% <0.00%> (-0.23%) ⬇️
ftw-multiphase 49.01% <0.00%> (-0.24%) ⬇️
tinygo 72.74% <47.36%> (+0.09%) ⬆️

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

Impacted Files Coverage Δ
internal/auditlog/init.go 76.92% <0.00%> (-23.08%) ⬇️
internal/auditlog/https_writer.go 47.36% <47.36%> (ø)

... and 1 file with indirect coverage changes

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

"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
)

// httpsWriter is used to store logs in a single file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// httpsWriter is used to store logs in a single file
// httpsWriter sends logs to an HTTP endpoint

Copy link
Member

@jcchavezs jcchavezs Jul 11, 2023

Choose a reason for hiding this comment

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

Also include this please

@jptosso jptosso merged commit a838547 into main Jul 11, 2023
@jptosso jptosso deleted the implement-https branch July 11, 2023 01:20
@jptosso jptosso linked an issue Jul 11, 2023 that may be closed by this pull request
@jcchavezs
Copy link
Member

One thing missing here is the content type support. We could add it to formatters (yet not to the interface) and do interface assertion ok init and record the value to be added to the requests.

@jptosso
Copy link
Member Author

jptosso commented Jul 11, 2023

One thing missing here is the content type support. We could add it to formatters (yet not to the interface) and do interface assertion ok init and record the value to be added to the requests.

I will take care before v3.0.3 release

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.

add SecAuditLogType HTTPS for retrocompatibility with Modsecurity
3 participants