#5407 closed enhancement (complete)

Feature Suggestion: Add a flag to disable forensic logging specifically by scope.

Reported by: mcnally Owned by: marcin
Priority: high Milestone: Kea1.4
Component: hooks Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Premium Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Apologies if this is a duplicate. The customer who suggested
this makes reference to "a feature request in the hopper" but
I couldn't find an existing ticket that matched.

Received via Support RT:

There is a feature request in the hopper to introduce support for
libdhcp_legal_log.so writing out to postgresql instead of flat
files.

After a dialog with Tomek about potential database overhead issues
arising from volumes of uninteresting log messages being written
to the database, he asked if this approach to mitigate that issue
would be acceptable:

"subnet4": [

{

"subnet": "192.0.2.0/24",
"user-context": {

"forensic-logging": false

},
your other parameters here

}

]

I said it would be fine, and he asked me to submit a ticket here
for that.

Subtickets

Change History (15)

comment:1 Changed 20 months ago by tomek

  • Component changed from Unclassified to hooks
  • Milestone changed from Kea-proposed to Kea1.4

comment:2 Changed 20 months ago by tomek

  • Priority changed from medium to high

comment:3 Changed 19 months ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:4 Changed 19 months ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

This ticket is now ready for review.

Proposed ChangeLog entry for Kea core:

13XX.	[doc]		marcin
	Updated User's Guide describing how to selectively disable
	legal logging for a subnet.
	(Trac #5407, git cafe)

and for premium:

XX.	[func]		marcin
	Legal logging hooks library allows for disabling logs for
	selected subnets.
	(Trac #5407, git cafe)

comment:5 Changed 19 months ago by marcin

  • Owner changed from UnAssigned to marcin

comment:6 Changed 19 months ago by marcin

  • Owner changed from marcin to UnAssigned

I previously forgot to suppress logging messages for control commands. Now I implemented this and I put it back to review.

comment:7 Changed 19 months ago by fdupont

I am late but it is a good use case for advanced classification as proposed by #5374...

comment:8 Changed 19 months ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:9 follow-up: Changed 19 months ago by fdupont

  • Owner changed from fdupont to marcin

IMHO the code added in command_processed should be more robust:

  • it assumes there is a subnet-id even it is not required in all variants, now it raises an exception and gives an error message as previous code which follows...
  • even it should not happen IMHO it is bad to defer the cfg variable with checking first if it is not null. So please add a sanity check with a comment saying it should not happen.

Same concern about legalLog[46]Handler.

At these exceptions which are far to be critical the code is OK and passed my checks on macOS.

comment:10 in reply to: ↑ 9 Changed 19 months ago by marcin

  • Owner changed from marcin to UnAssigned

Replying to fdupont:

IMHO the code added in command_processed should be more robust:

  • it assumes there is a subnet-id even it is not required in all variants, now it raises an exception and gives an error message as previous code which follows...

Right. I now made this callout more robust.

  • even it should not happen IMHO it is bad to defer the cfg variable with checking first if it is not null. So please add a sanity check with a comment saying it should not happen.

Same concern about legalLog[46]Handler.

I didn't make these changes. We have tons of places where we assume that these pointers are non-null and they actually guarantee to be non-null. In my opinion this would make the code unnecessarily more complex.

At these exceptions which are far to be critical the code is OK and passed my checks on macOS.

comment:11 Changed 19 months ago by marcin

  • Owner changed from UnAssigned to fdupont

comment:12 follow-up: Changed 19 months ago by fdupont

  • Owner changed from fdupont to marcin

I said the code was OK. Now if you'd like I re-review last changes please push them...

comment:13 in reply to: ↑ 12 Changed 19 months ago by marcin

  • Owner changed from marcin to fdupont

Replying to fdupont:

I said the code was OK. Now if you'd like I re-review last changes please push them...

Not sure what is going on. The changes had been pushed to the premium repo. Maybe you forgot to pull?

comment:14 Changed 19 months ago by fdupont

  • Owner changed from fdupont to marcin

Got the changes which are OK.

comment:15 Changed 19 months ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged with commits 68119eb826b8d4995d7a33bc27fc1487c1ba6933 and 469080abd711f8e88a5133f76f4ab31a5549a858 to premium and core repos respectively.

Note: See TracTickets for help on using tickets.