Opened 4 years ago

Closed 4 years ago

#4298 closed enhancement (complete)

Legal logging: hook library that stores MAC,IP,timestamp, circuit-id, remote-id in a file

Reported by: tomek Owned by: tmark
Priority: high Milestone: Kea1.1
Component: hooks Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 24 Add Hours to Ticket: 1
Total Hours: 61 Internal?: no

Description

Once #4297 is done, we need to develop a library that logs assigned addresses in a separate file. The assumption is that it will provide all logging information necessary for legal purposes. In particular, it must cover logging timestamp, IP adress (for both v4 and v6) and as much client information as possible. In particular, it must log circuit-id and remote-id (if available).

Here's a proposal of a log entry:

2016-02-03 16:58:11 Address 192.0.2.1 has been assigned for 4 hours to a
device with hardware address 01:02:03:04:05:06, client-id XYZ connected via relay device identified by circuit-id GigabitEthernet0/0/48:10.4096
abc-def-gh-11/0/0/0/0/0 and remote-id 0a:0b:0c:0d:0e:0f.

This should log independently of the logging system (to avoid any misconfiguration). The output files should be rotated on a daily basis.

Subtickets

Change History (15)

comment:1 Changed 4 years ago by fdupont

IMHO we must check with customers if there is a missing (or an extra even it is far easier to remove something than the opposite) item. As it is for (national) legal purpose we have no real view about what is/will be required to provide.
For the MAC address in DHCPv6 we should provide the way it was inferred too.

comment:2 Changed 4 years ago by fdupont

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

comment:3 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to assigned

I created the hierarchy and copied some basic files from the user check directory. Leaving the ticket to someone else...

comment:4 Changed 4 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:5 Changed 4 years ago by tmark

  • Estimated Difficulty changed from 0 to 24
  • Owner changed from tmark to unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 48

The implementation exists in a new directory, src/hooks/dhcp/legal_log. The primary files are:

legal_file.h/cc - which implement an appending text file that
rotates to a new file on a daily basis. It does not dictate
format content, other than to prefix each line with a time-stamp
and terminate it with an EOL marker (std::endl).

load_unload.cc - provides the load() and unload() functions for
the library

lease4_cos.cc - provides the following functions:

  • std::string genLease4Entry(Pkt4Ptr query, Lease4Ptr lease, bool renewal) Generates a string containing the legal entry text for a lease event.
  • int legallog4_handler(CalloutHandle?& handle, bool renewal) Produces a DHCPv4 legal file entry given a Callout handle. Called by lease4_select and lease4_renew callouts.


  • int pkt4_receive(CalloutHandle?& handle) Stores the inbound packet in the callout context, since it is not currently a context argument for the lease4_select or lease4_renew callouts.


  • int lease4_select(CalloutHandle?& handle) Callout for the lease4_select hook point. Uses legallog4_handler() to generate legal entries for lease assignments.
  • int lease4_renew(CalloutHandle?& handle) Callout for the lease4_renew hook point. Uses legallog4_handler() to generate legal entries for lease renewals.

lease6_cos.cc - provides the following functions:

  • std::string genLease6Entry(Pkt6Ptr query, Lease6Ptr lease, bool renewal) Generates a string containing the legal entry text for a lease event
  • int legallog6_handler(CalloutHandle?& handle, bool renewal) Produces a DHCPv6 legal file entry given a Callout handle. Called by lease6_select, lease6_renew, and lease6_rebind callouts.


  • int pkt6_receive(CalloutHandle?& handle) Stores the inbound packet in the callout context, since it is not currently a context argument for the lease6_* callouts


  • int lease6_select(CalloutHandle?& handle) Callout for the lease6_select hook point. Uses legallog6_handler() to generate legal entrien for lease assignments.
  • int lease6_renew(CalloutHandle?& handle) Callout for the lease6_renew hook point. Uses legallog6_handler() to generate legal entries for lease renewals.
  • int lease6_rebind(CalloutHandle?& handle) Callout for the lease6_rebind hook point. Uses legallog6_handler() to generate legal entries for lease rebinds.

To configure the hook for use:

"hooks-libraries": [
{

"library": "<your path here>/src/hooks/dhcp/legal_log/.libs/libdhcp_legal_log.so"
,
"parameters": {

"path": "<some path>",
"base-name": "<some text>"
}

}

path - is the directory in which the legal files should be written. the default is:

<prefix>/var/kea

base-name - is an arbitrary prefix used in the legal file name, the default value is:

"kea-legal"

The full file name is given by:

<path>/<base-name>.<CCYYMMDD>.txt

Unit Testing:

Typical unit tests are found in legal_hooks/tests. These tests exercise components of
the legal log hook library by linking the library into the test executable. This
makes testing fairly straight forward and allows access the library's LegalFile? instance.

There is a second directory, libloadtests, which contain tests that are conducted by
loading the library via the HooksManager? rather than linking into the executable. This
allows testing of the load and unload functions. This was done primarily to provide a
means to test the load() functions use of Hook library parameters. It seems the only
way to populate a library's parameter set is to actually load the library, and loading
a library into an executable which was linked against it is fraught with issues.

System Testing:

System tests that can be run via Forge need to be created for this. This should be
relatively easy to do, as we already have tests for user_chk hook library. The only
real issue involved is working around time stamp values in the legal log files when
it comes to expected results versus actual results.

Documentation:

None other than the code commentary and this text. I felt we should get a baseline
review first. As this is intended to be a separate product we will need to think
about how the documentation should be organized.

comment:6 Changed 4 years ago by tmark

Note once #4481 has been merged, this implementation can be modified to not cache the query<x> argument as is currently being done in pkt<x>_receive callouts.

comment:7 Changed 4 years ago by tmark

  • Owner changed from unassigned to tmark

comment:8 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 0 to 3
  • Owner changed from tmark to Unassigned
  • Total Hours changed from 48 to 51

Implementation has been updated to utilize the query<x> argument, rather than caching it in the callout context during pkt<x>_receive callout. Thus the pkt<x>_receive callouts have been eliminated.

Ticket is again ready for review

comment:9 Changed 4 years ago by marcin

  • Owner changed from Unassigned to marcin

comment:10 follow-up: Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 3 to 6
  • Total Hours changed from 51 to 57

Reviewed commit 18705fd01b5841669f07f89caf94adad07dd569f

The distcheck failed for me with the following error:

 (cd legal_log && make  top_distdir=../../../../kea-1.0.0-git distdir=../../../../kea-1.0.0-git/src/hooks/dhcp/legal_log \
     am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[5]: make[5]: don't know how to make libdhcp_legal_log.dox. Stop

The new library comes with no documentation. I see your comment about it, but it seems we need a separate ticket to be opened. Can you do it?
On that matter, the doxygen comments are provided for respective classes and functions but this documentation will not be included in the generated html, because Doxyfile file hasn't been updated to point to the new directory.

A general comment. The legal logging library implements lease4_select and lease4_renew hook points. These hook points are triggered when the server has assigned a lease but it doesn't guarantee that the client will actually use this lease. There are two considerations. The client which detects that IP address is in use by a different host will send a decline and not use the address assigned. Another time when the client will not use the assigned address is when there is another hook installed which modifies the assigned address.

I believe the decline case is pretty rare and when the client declines a lease, it may try to request another lease and that fact will be logged. However, in cases when the client doesn't request another lease for any reason, the log will indicate that the lease has been assigned to the client but the client will not use this lease. I guess we should think about implementing lease4_decline hook in legal logging library to make sure that the information about the leases assigned is consistent. If you concur we should go down that path I wouldn't mind if we do it in a separate ticket.

The second consideration is that when the order of hooks libraries causes hooks belonging to legal logging library be invoked before other hooks and those other hooks modify the assigned address, the information in the legal log will be incorrect. There is not much we can do about it probably, other than documenting that fact and perhaps suggest to the users that legal logging library should be included after other libraries (in a config file).

src/hooks/dhcp/legal_log/Makefile.am
Oddly enough, the libdhcp_legal_log.dox is included in EXTRA_DIST but this file doesn't exist.

src/hooks/dhcp/legal_log/lease4_cos.cc

/// @file lease4_co.cc Defines lease4_select and lease4_renew callout functions.

Typo "lease4_co.cc". It is in fact "lease4_cos.cc".

BTW, I don't understand what the "_cos" part stands for? We normally try to give files self explanatory names.

Creates an entry based on the given DHCPv4 DHCPREQUEST and corresponding
...

The DHCPv4 prior to DHCPREQUEST seems to be redundant. Technically, this is correct, but it reads odd.

genLease4Entry: It would be slightly more efficient if "query" and "lease" were passed by const reference, rather than by value. In addition, the "renewal" could be const.

There should be a space character after "return" in genLease4Entry, lease4_select and lease4_renew.

Use of LegalFile::vectorHexDump should be replaced with existing utility functions (i.e. util::encode::encodeHex) and the vectorHexDump should be removed to avoid code duplication. Note that the hook library is linked with libkea-util.

I don't see a point of using non-camel notation for legallog4_handler function. First of all, per our coding standards we use camel notation for function names. This function is not really an exception. I realize that we sometimes use underscore type of naming for handler functions (like callback functions), but I am not even sure this is correct per our coding guidelines. The current name is in my opinion a poor choice, because in "legallog" it is hard to distinguish the two words from each other. And then, it is surprisingly followed by the underscore before handler. How about: handleLegalLog4, createLegalLogEntry4, writeLegalLogEntry4 ?

In legallog4_handler the "renewal" parameter could be const.

lease4_select: it would be possible to use:

return (fake_allocation ? 0 : legallog4_handler(handle, false));

for brevity.

src/hooks/dhcp/legal_log/lease6_cos.cc
hwaddrSourceToString - hyphen should be removed after @return tag in the doxygen description. So as after "source" in the same description. Update after further review: I see you use hyphens as convention so I stopped picking those further in the review. But in general, I don't think we should use hyphens after parameters in general.

I don't see any function to convert the HW source to string in the core code, but I wonder if this function wouldn't be better placed in !CfgMACSource. On the flip side, it would require linking with libdhcpsrv. Maybe put these conversion functions in libdhcp++.

For the rest of this file I have the same comments as for lease4_cos.cc

src/hooks/dhcp/legal_log/legal_file.cc
getDurationString: I am not certain that the commentary in this function is accurate. It says that we operate on uint32_t and posix_time on longs. So, why wouldn't I be able to do this:

time_duration td = seconds(static_cast<long>(secs));

?

getDurationString: why do we care about singular form for "day" vs "days", but not about "hrs", "secs". And, why "min" is singular?

src/hooks/dhcp/legal_log/legal_file.h

In the file description:

/// @file legal_file.h Defines the class, LegalFile, which implements a
/// an appending text file which rotates to a new file on a daily basis.

remove "a" after implements.

The LegalFile name doesn't correspond to a description. I understand that the purpose of the class is to store legal logs, but the description doesn't say so. Instead, it says that the file rotates. I think the description is correct because there is nothing specific to legal logging, so perhaps it would be better to call this class RotatingFile?

Also, it would be useful to mention that the LegalFile class has virtual methods because this is needed for unit testing, rather than anything else.

The description of the LegalFile class will not be very well formatted in the generated html output. In particular, enumeration of "path", "base_name" and "date" will not have indentation in the generated html. It may be better to use hyphens (and bold), like this:

///     - <b>path</b> - is the pathname supplied via the constructor. The path
///       must exist and be writable by the process
///
///     - <b>base_name</b> - an arbitrary text label supplied via the constructor
///
///     - <b>date</b> - is the system date, at the time the file is opened, in local
///       time.  The format of the value is CCYYMMDD (century,year,month,day)

In the following text:

    /// where CCYYMMDD is the current date in local time.

replace dot with a comma.

writeln: description requires better formatting as in case of class description.

If today() and now() is exposed to simplify testing, couldn't it be protected?

getFileName should be const, so as getFileDay(). Also, include a space right after "return" in those functions.

getDurationString: secs could be const and the formatting of the examples may be improved.

src/hooks/dhcp/legal_log/legal_log_log.cc
Our current loggers use hyphen rather than underscore for names.
Copy paste error "user check":

/// Defines the logger used by the user check hooks library.
#include <legal_log_log.h>

src/hooks/dhcp/legal_log/legal_log_messages.mes
LEGAL_FILE_HOOK_LEASE4_NO_LEGAL_FILE: There is no such thing like LegalFileHook. Maybe better to say libdhcp_legal_log? Same comment for other messages.
Apart from that the description says: "This is an error message issued when the LegalLogHook? library attempted to
write a IPv4 lease renewal notice to the legal file and the file instance has not been created." But, not only is this for renewal notice but also for new lease assignment, so this should be reworded a bit.

LEGAL_FILE_HOOK_LEASE4_WRITE_ERROR: typo assignement

LEGAL_FILE_HOOK_PKT4_RECEIVE_ERROR: "expected" => "unexpected". Same for PKT6 counterpart.
BTW, are these messages used anywhere?

src/hooks/dhcp/legal_log/libloadtests/Makefile.am
Do we really have to link with all these libraries:

libdhcp_legal_log_unittests_LDADD = $(top_builddir)/src/lib/dhcpsrv/libkea-dhcpsrv.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la

This is an honest question. Maybe they depend on each other so much that all have to be included? But dns++?

src/hooks/dhcp/legal_log/libloadtests/load_unload_unittests.cc
It might be better to rename LibLoadTest? to LegalLibLoadTest? to indicate that it refers to legal logging library. Oterwise, from the unit tests log it is hard to distinguish which tests cover which library.

In the test fixture class description it says: "Test fixture for testing LegalFile". It should rather be "Test fixture for testing loading and unloading legal library".

You could tehcnically remove this:

::remove(expected_filename_.c_str());

because dangling files are removed in the test fixture class destructor.

invalidLoad: Last comment is invalid, no? The library fails to load so, there should be no file created.

src/hooks/dhcp/legal_log/load_unload.cc
There are some unnecessary headers included, e.g. fstream, errno.h, iostream.

src/hooks/dhcp/legal_log/tests/.gitignore
What is test_data_files_config.h?

src/hooks/dhcp/legal_log/tests/Makefile.am
Again, do we need to link with all those libraries? Like dns++, cryptolink etc?

src/hooks/dhcp/legal_log/tests/legal_file_unittests.cc
invalidConstruction: in the second assert did you mean to use "TEST_DATA_BUILDDIR" in quotes?

src/hooks/dhcp/legal_log/tests/test_utils.h
I suggest to use #ifdef in this header file to avoid multiple inclusions.

CalloutTest? description: Looks like it is copy pasted from LegalFileTest with no change.

getCalloutManager is not documented.

Once you enable generation of HTML documentation for the new library you'll see some new doxygen warnings. Please correct them.

comment:11 Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 6 to 1
  • Owner changed from marcin to tmark
  • Total Hours changed from 57 to 58

I also manually checked that the library loads during server startup and that it logs to a desired file.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by tmark

Replying to marcin:

Reviewed commit 18705fd01b5841669f07f89caf94adad07dd569f

The distcheck failed for me with the following error:

 (cd legal_log && make  top_distdir=../../../../kea-1.0.0-git distdir=../../../../kea-1.0.0-git/src/hooks/dhcp/legal_log \
     am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[5]: make[5]: don't know how to make libdhcp_legal_log.dox. Stop

I have commented out the entry from the Makefile.am with a todo referring to the new documentation ticket 4511.

The new library comes with no documentation. I see your comment about it, but it seems we need a separate ticket to be opened. Can you do it?
On that matter, the doxygen comments are provided for respective classes and functions but this documentation will not be included in the generated html, because Doxyfile file hasn't been updated to point to the new directory.

I have commented out the entry from the Makefile.am with a todo referring to the new documentation ticket 4511.

A general comment. The legal logging library implements lease4_select and lease4_renew hook points. These hook points are triggered when the server has assigned a lease but it doesn't guarantee that the client will actually use this lease. There are two considerations. The client which detects that IP address is in use by a different host will send a decline and not use the address assigned. Another time when the client will not use the assigned address is when there is another hook installed which modifies the assigned address.

I believe the decline case is pretty rare and when the client declines a lease, it may try to request another lease and that fact will be logged. However, in cases when the client doesn't request another lease for any reason, the log will indicate that the lease has been assigned to the client but the client will not use this lease. I guess we should think about implementing lease4_decline hook in legal logging library to make sure that the information about the leases assigned is consistent. If you concur we should go down that path I wouldn't mind if we do it in a separate ticket.

If we go down this path, then we should not also log releases? We can do this. Maybe see what Tomek thinks?

The second consideration is that when the order of hooks libraries causes hooks belonging to legal logging library be invoked before other hooks and those other hooks modify the assigned address, the information in the legal log will be incorrect. There is not much we can do about it probably, other than documenting that fact and perhaps suggest to the users that legal logging library should be included after other libraries (in a config file).

This is a good point. I think the only thing we can really do about it is document it as you suggest. Because it is being done with hooks, ulitmately, we have no way to gaurantee they use an order that is accurate.

src/hooks/dhcp/legal_log/Makefile.am
Oddly enough, the libdhcp_legal_log.dox is included in EXTRA_DIST but this file doesn't exist.

See above.

src/hooks/dhcp/legal_log/lease4_cos.cc

/// @file lease4_co.cc Defines lease4_select and lease4_renew callout functions.

Typo "lease4_co.cc". It is in fact "lease4_cos.cc".

BTW, I don't understand what the "_cos" part stands for? We normally try to give files self explanatory names.

"cos" was short for "callouts". I have renamed them lease4_callouts.cc and lease6_callouts.cc.

Creates an entry based on the given DHCPv4 DHCPREQUEST and corresponding
...

The DHCPv4 prior to DHCPREQUEST seems to be redundant. Technically, this is correct, but it reads odd.

Gone.

genLease4Entry: It would be slightly more efficient if "query" and "lease" were passed by const reference, rather than by value. In addition, the "renewal" could be const.

Done.

There should be a space character after "return" in genLease4Entry, lease4_select and lease4_renew.

Fixed it here and many other places.

Use of LegalFile::vectorHexDump should be replaced with existing utility functions (i.e. util::encode::encodeHex) and the vectorHexDump should be removed to avoid code duplication. Note that the hook library is linked with libkea-util.

The function, encode::encodeHex() does not allow for delimeters. Because I am using toText() methods for converting DUID an HWAddr to strings and these include ":" delimeters, I wrote this function so things like CIRCUIT_IDs would also have colon delimeters. I did not want inconsistent formats for hex values. So in order to keep all hex strings displayed the same way:

1) I could alter encodeHex() to allow for an option delimeter
2) I could use encodeHex() to output all values, instead of using DUID.toText() and HWAddr.toText(), meaning none would have ":"
3) I could use some other util function which gives me colon delimited hex strings that I haven't found yet. I imagine if it existing the aforementioned toText() methods would have used it.
4) I could use the function I wrote

My personal favorite here is #4.

I don't see a point of using non-camel notation for legallog4_handler function. First of all, per our coding standards we use camel notation for function names. This function is not really an exception. I realize that we sometimes use underscore type of naming for handler functions (like callback functions), but I am not even sure this is correct per our coding guidelines. The current name is in my opinion a poor choice, because in "legallog" it is hard to distinguish the two words from each other. And then, it is surprisingly followed by the underscore before handler. How about: handleLegalLog4, createLegalLogEntry4, writeLegalLogEntry4 ?

Only because I created it initially by copy-pasting the lease4_select() callout and for some reason I didn't switch back to camel case when I renamed it. If the callouts had been camel-cased it would never have happened. ;)

In legallog4_handler the "renewal" parameter could be const.

Done

lease4_select: it would be possible to use:

return (fake_allocation ? 0 : legallog4_handler(handle, false));

for brevity.

Changed.

src/hooks/dhcp/legal_log/lease6_cos.cc
hwaddrSourceToString - hyphen should be removed after @return tag in the doxygen description. So as after "source" in the same description. Update after further review: I see you use hyphens as convention so I stopped picking those further in the review. But in general, I don't think we should use hyphens after parameters in general.

Took them out.

I don't see any function to convert the HW source to string in the core code, but I wonder if this function wouldn't be better placed in !CfgMACSource. On the flip side, it would require linking with libdhcpsrv. Maybe put these conversion functions in libdhcp++.

This branch is not going to be merged into master as we do not want it publicly distributed, so I thus far have avoiding adding anything into Kea proper under this ticket. If you want me to create a ticket to add such functions I can.

For the rest of this file I have the same comments as for lease4_cos.cc

src/hooks/dhcp/legal_log/legal_file.cc
getDurationString: I am not certain that the commentary in this function is accurate. It says that we operate on uint32_t and posix_time on longs. So, why wouldn't I be able to do this:

time_duration td = seconds(static_cast<long>(secs));

?

The primary issue here is that a lease lifetime of 0xFFFFFFFF is valid and does not mean -1. I have expanded the commentary.

getDurationString: why do we care about singular form for "day" vs "days", but not about "hrs", "secs". And, why "min" is singular?

Somehow it sounded better and honestly the "requirements" didn't even account for days so I just made something up. I didn't think "cops" would want to 1234893 hrs. I've made them all plural. If you'd like some other format, please suggest one.

src/hooks/dhcp/legal_log/legal_file.h

In the file description:

/// @file legal_file.h Defines the class, LegalFile, which implements a
/// an appending text file which rotates to a new file on a daily basis.

remove "a" after implements.

Done.

The LegalFile name doesn't correspond to a description. I understand that the purpose of the class is to store legal logs, but the description doesn't say so. Instead, it says that the file rotates. I think the description is correct because there is nothing specific to legal logging, so perhaps it would be better to call this class RotatingFile?

Renamed it to RotatingFile?.

Also, it would be useful to mention that the LegalFile class has virtual methods because this is needed for unit testing, rather than anything else.

Done.

The description of the LegalFile class will not be very well formatted in the generated html output. In particular, enumeration of "path", "base_name" and "date" will not have indentation in the generated html. It may be better to use hyphens (and bold), like this:

///     - <b>path</b> - is the pathname supplied via the constructor. The path
///       must exist and be writable by the process
///
///     - <b>base_name</b> - an arbitrary text label supplied via the constructor
///
///     - <b>date</b> - is the system date, at the time the file is opened, in local
///       time.  The format of the value is CCYYMMDD (century,year,month,day)

Done

In the following text:

    /// where CCYYMMDD is the current date in local time.

replace dot with a comma.

Done

writeln: description requires better formatting as in case of class description.

Done

If today() and now() is exposed to simplify testing, couldn't it be protected?

Done

getFileName should be const, so as getFileDay(). Also, include a space right after "return" in those functions.

Done

getDurationString: secs could be const and the formatting of the examples may be improved.

Done.

src/hooks/dhcp/legal_log/legal_log_log.cc
Our current loggers use hyphen rather than underscore for names.

Done

Copy paste error "user check":

/// Defines the logger used by the user check hooks library.
#include <legal_log_log.h>

Done

src/hooks/dhcp/legal_log/legal_log_messages.mes
LEGAL_FILE_HOOK_LEASE4_NO_LEGAL_FILE: There is no such thing like LegalFileHook. Maybe better to say libdhcp_legal_log? Same comment for other messages.

I changed such references in the messages to be "Legal Log library"
I also have rename the log message identifiers to be LEGAL_LOG_* instead of LEGAL_FILE_HOOK_*.

Apart from that the description says: "This is an error message issued when the LegalLogHook? library attempted to
write a IPv4 lease renewal notice to the legal file and the file instance has not been created." But, not only is this for renewal notice but also for new lease assignment, so this should be reworded a bit.

Fixed.

LEGAL_FILE_HOOK_LEASE4_WRITE_ERROR: typo assignement

Fixed.

LEGAL_FILE_HOOK_PKT4_RECEIVE_ERROR: "expected" => "unexpected". Same for PKT6 counterpart.
BTW, are these messages used anywhere?

No they're dead. Removed.

src/hooks/dhcp/legal_log/libloadtests/Makefile.am
Do we really have to link with all these libraries:

libdhcp_legal_log_unittests_LDADD = $(top_builddir)/src/lib/dhcpsrv/libkea-dhcpsrv.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la

This is an honest question. Maybe they depend on each other so much that all have to be included? But dns++?

Trimmed down.

src/hooks/dhcp/legal_log/libloadtests/load_unload_unittests.cc
It might be better to rename LibLoadTest? to LegalLibLoadTest? to indicate that it refers to legal logging library. Oterwise, from the unit tests log it is hard to distinguish which tests cover which library.

Done

In the test fixture class description it says: "Test fixture for testing LegalFile". It should rather be "Test fixture for testing loading and unloading legal library".

Done

You could tehcnically remove this:

::remove(expected_filename_.c_str());

because dangling files are removed in the test fixture class destructor.

Done

invalidLoad: Last comment is invalid, no? The library fails to load so, there should be no file created.

Fixed.

src/hooks/dhcp/legal_log/load_unload.cc
There are some unnecessary headers included, e.g. fstream, errno.h, iostream.

Removed.

src/hooks/dhcp/legal_log/tests/.gitignore
What is test_data_files_config.h?

Removed.

src/hooks/dhcp/legal_log/tests/Makefile.am
Again, do we need to link with all those libraries? Like dns++, cryptolink etc?

Trimmed down.

src/hooks/dhcp/legal_log/tests/legal_file_unittests.cc
invalidConstruction: in the second assert did you mean to use "TEST_DATA_BUILDDIR" in quotes?

I was checking to see if anyone would notice. Fixed.

src/hooks/dhcp/legal_log/tests/test_utils.h
I suggest to use #ifdef in this header file to avoid multiple inclusions.

Done

CalloutTest? description: Looks like it is copy pasted from LegalFileTest with no change.

getCalloutManager is not documented.

Man I suck. Fixed.

Once you enable generation of HTML documentation for the new library you'll see some new doxygen warnings. Please correct them.

Duly noted.

comment:13 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 1 to 2
  • Owner changed from tmark to marcin
  • Total Hours changed from 58 to 60

comment:14 in reply to: ↑ 12 Changed 4 years ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

Replying to marcin:

Reviewed commit 18705fd01b5841669f07f89caf94adad07dd569f

The distcheck failed for me with the following error:

 (cd legal_log && make  top_distdir=../../../../kea-1.0.0-git distdir=../../../../kea-1.0.0-git/src/hooks/dhcp/legal_log \
     am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[5]: make[5]: don't know how to make libdhcp_legal_log.dox. Stop

I have commented out the entry from the Makefile.am with a todo referring to the new documentation ticket 4511.

The new library comes with no documentation. I see your comment about it, but it seems we need a separate ticket to be opened. Can you do it?
On that matter, the doxygen comments are provided for respective classes and functions but this documentation will not be included in the generated html, because Doxyfile file hasn't been updated to point to the new directory.

I have commented out the entry from the Makefile.am with a todo referring to the new documentation ticket 4511.

A general comment. The legal logging library implements lease4_select and lease4_renew hook points. These hook points are triggered when the server has assigned a lease but it doesn't guarantee that the client will actually use this lease. There are two considerations. The client which detects that IP address is in use by a different host will send a decline and not use the address assigned. Another time when the client will not use the assigned address is when there is another hook installed which modifies the assigned address.

I believe the decline case is pretty rare and when the client declines a lease, it may try to request another lease and that fact will be logged. However, in cases when the client doesn't request another lease for any reason, the log will indicate that the lease has been assigned to the client but the client will not use this lease. I guess we should think about implementing lease4_decline hook in legal logging library to make sure that the information about the leases assigned is consistent. If you concur we should go down that path I wouldn't mind if we do it in a separate ticket.

If we go down this path, then we should not also log releases? We can do this. Maybe see what Tomek thinks?

We can ask what people think and address it in a separate ticket if necessary.

The second consideration is that when the order of hooks libraries causes hooks belonging to legal logging library be invoked before other hooks and those other hooks modify the assigned address, the information in the legal log will be incorrect. There is not much we can do about it probably, other than documenting that fact and perhaps suggest to the users that legal logging library should be included after other libraries (in a config file).

This is a good point. I think the only thing we can really do about it is document it as you suggest. Because it is being done with hooks, ulitmately, we have no way to gaurantee they use an order that is accurate.

src/hooks/dhcp/legal_log/Makefile.am
Oddly enough, the libdhcp_legal_log.dox is included in EXTRA_DIST but this file doesn't exist.

See above.

src/hooks/dhcp/legal_log/lease4_cos.cc

/// @file lease4_co.cc Defines lease4_select and lease4_renew callout functions.

Typo "lease4_co.cc". It is in fact "lease4_cos.cc".

BTW, I don't understand what the "_cos" part stands for? We normally try to give files self explanatory names.

"cos" was short for "callouts". I have renamed them lease4_callouts.cc and lease6_callouts.cc.

Creates an entry based on the given DHCPv4 DHCPREQUEST and corresponding
...

The DHCPv4 prior to DHCPREQUEST seems to be redundant. Technically, this is correct, but it reads odd.

Gone.

genLease4Entry: It would be slightly more efficient if "query" and "lease" were passed by const reference, rather than by value. In addition, the "renewal" could be const.

Done.

There should be a space character after "return" in genLease4Entry, lease4_select and lease4_renew.

Fixed it here and many other places.

Are you sure? I still see it in lease4_select for example, lease4_renew.

Use of LegalFile::vectorHexDump should be replaced with existing utility functions (i.e. util::encode::encodeHex) and the vectorHexDump should be removed to avoid code duplication. Note that the hook library is linked with libkea-util.

The function, encode::encodeHex() does not allow for delimeters. Because I am using toText() methods for converting DUID an HWAddr to strings and these include ":" delimeters, I wrote this function so things like CIRCUIT_IDs would also have colon delimeters. I did not want inconsistent formats for hex values. So in order to keep all hex strings displayed the same way:

1) I could alter encodeHex() to allow for an option delimeter
2) I could use encodeHex() to output all values, instead of using DUID.toText() and HWAddr.toText(), meaning none would have ":"
3) I could use some other util function which gives me colon delimited hex strings that I haven't found yet. I imagine if it existing the aforementioned toText() methods would have used it.
4) I could use the function I wrote

My personal favorite here is #4.

ok.

I don't see a point of using non-camel notation for legallog4_handler function. First of all, per our coding standards we use camel notation for function names. This function is not really an exception. I realize that we sometimes use underscore type of naming for handler functions (like callback functions), but I am not even sure this is correct per our coding guidelines. The current name is in my opinion a poor choice, because in "legallog" it is hard to distinguish the two words from each other. And then, it is surprisingly followed by the underscore before handler. How about: handleLegalLog4, createLegalLogEntry4, writeLegalLogEntry4 ?

Only because I created it initially by copy-pasting the lease4_select() callout and for some reason I didn't switch back to camel case when I renamed it. If the callouts had been camel-cased it would never have happened. ;)

In legallog4_handler the "renewal" parameter could be const.

Done

lease4_select: it would be possible to use:

return (fake_allocation ? 0 : legallog4_handler(handle, false));

for brevity.

Changed.

src/hooks/dhcp/legal_log/lease6_cos.cc
hwaddrSourceToString - hyphen should be removed after @return tag in the doxygen description. So as after "source" in the same description. Update after further review: I see you use hyphens as convention so I stopped picking those further in the review. But in general, I don't think we should use hyphens after parameters in general.

Took them out.

I don't see any function to convert the HW source to string in the core code, but I wonder if this function wouldn't be better placed in !CfgMACSource. On the flip side, it would require linking with libdhcpsrv. Maybe put these conversion functions in libdhcp++.

This branch is not going to be merged into master as we do not want it publicly distributed, so I thus far have avoiding adding anything into Kea proper under this ticket. If you want me to create a ticket to add such functions I can.

Yes, please.

For the rest of this file I have the same comments as for lease4_cos.cc

src/hooks/dhcp/legal_log/legal_file.cc
getDurationString: I am not certain that the commentary in this function is accurate. It says that we operate on uint32_t and posix_time on longs. So, why wouldn't I be able to do this:

time_duration td = seconds(static_cast<long>(secs));

?

The primary issue here is that a lease lifetime of 0xFFFFFFFF is valid and does not mean -1. I have expanded the commentary.

ok.

getDurationString: why do we care about singular form for "day" vs "days", but not about "hrs", "secs". And, why "min" is singular?

Somehow it sounded better and honestly the "requirements" didn't even account for days so I just made something up. I didn't think "cops" would want to 1234893 hrs. I've made them all plural. If you'd like some other format, please suggest one.

No. I think it best looks when it is consistent.

src/hooks/dhcp/legal_log/legal_file.h

In the file description:

/// @file legal_file.h Defines the class, LegalFile, which implements a
/// an appending text file which rotates to a new file on a daily basis.

remove "a" after implements.

Done.

The LegalFile name doesn't correspond to a description. I understand that the purpose of the class is to store legal logs, but the description doesn't say so. Instead, it says that the file rotates. I think the description is correct because there is nothing specific to legal logging, so perhaps it would be better to call this class RotatingFile?

Renamed it to RotatingFile?.

Also, it would be useful to mention that the LegalFile class has virtual methods because this is needed for unit testing, rather than anything else.

Done.

The description of the LegalFile class will not be very well formatted in the generated html output. In particular, enumeration of "path", "base_name" and "date" will not have indentation in the generated html. It may be better to use hyphens (and bold), like this:

///     - <b>path</b> - is the pathname supplied via the constructor. The path
///       must exist and be writable by the process
///
///     - <b>base_name</b> - an arbitrary text label supplied via the constructor
///
///     - <b>date</b> - is the system date, at the time the file is opened, in local
///       time.  The format of the value is CCYYMMDD (century,year,month,day)

Done

In the following text:

    /// where CCYYMMDD is the current date in local time.

replace dot with a comma.

Done

writeln: description requires better formatting as in case of class description.

Done

If today() and now() is exposed to simplify testing, couldn't it be protected?

Done

getFileName should be const, so as getFileDay(). Also, include a space right after "return" in those functions.

Done

getDurationString: secs could be const and the formatting of the examples may be improved.

Done.

src/hooks/dhcp/legal_log/legal_log_log.cc
Our current loggers use hyphen rather than underscore for names.

Done

Copy paste error "user check":

/// Defines the logger used by the user check hooks library.
#include <legal_log_log.h>

Done

src/hooks/dhcp/legal_log/legal_log_messages.mes
LEGAL_FILE_HOOK_LEASE4_NO_LEGAL_FILE: There is no such thing like LegalFileHook. Maybe better to say libdhcp_legal_log? Same comment for other messages.

I changed such references in the messages to be "Legal Log library"
I also have rename the log message identifiers to be LEGAL_LOG_* instead of LEGAL_FILE_HOOK_*.

Apart from that the description says: "This is an error message issued when the LegalLogHook? library attempted to
write a IPv4 lease renewal notice to the legal file and the file instance has not been created." But, not only is this for renewal notice but also for new lease assignment, so this should be reworded a bit.

Fixed.

LEGAL_FILE_HOOK_LEASE4_WRITE_ERROR: typo assignement

Fixed.

LEGAL_FILE_HOOK_PKT4_RECEIVE_ERROR: "expected" => "unexpected". Same for PKT6 counterpart.
BTW, are these messages used anywhere?

No they're dead. Removed.

src/hooks/dhcp/legal_log/libloadtests/Makefile.am
Do we really have to link with all these libraries:

libdhcp_legal_log_unittests_LDADD = $(top_builddir)/src/lib/dhcpsrv/libkea-dhcpsrv.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
libdhcp_legal_log_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la

This is an honest question. Maybe they depend on each other so much that all have to be included? But dns++?

Trimmed down.

src/hooks/dhcp/legal_log/libloadtests/load_unload_unittests.cc
It might be better to rename LibLoadTest? to LegalLibLoadTest? to indicate that it refers to legal logging library. Oterwise, from the unit tests log it is hard to distinguish which tests cover which library.

Done

In the test fixture class description it says: "Test fixture for testing LegalFile". It should rather be "Test fixture for testing loading and unloading legal library".

Done

You could tehcnically remove this:

::remove(expected_filename_.c_str());

because dangling files are removed in the test fixture class destructor.

Done

invalidLoad: Last comment is invalid, no? The library fails to load so, there should be no file created.

Fixed.

src/hooks/dhcp/legal_log/load_unload.cc
There are some unnecessary headers included, e.g. fstream, errno.h, iostream.

Removed.

src/hooks/dhcp/legal_log/tests/.gitignore
What is test_data_files_config.h?

Removed.

src/hooks/dhcp/legal_log/tests/Makefile.am
Again, do we need to link with all those libraries? Like dns++, cryptolink etc?

Trimmed down.

src/hooks/dhcp/legal_log/tests/legal_file_unittests.cc
invalidConstruction: in the second assert did you mean to use "TEST_DATA_BUILDDIR" in quotes?

I was checking to see if anyone would notice. Fixed.

src/hooks/dhcp/legal_log/tests/test_utils.h
I suggest to use #ifdef in this header file to avoid multiple inclusions.

Done

CalloutTest? description: Looks like it is copy pasted from LegalFileTest with no change.

getCalloutManager is not documented.

Man I suck. Fixed.

Once you enable generation of HTML documentation for the new library you'll see some new doxygen warnings. Please correct them.

Duly noted.

The distcheck passed for me this time. Apart from making sure that you put spaces between "return" and "(" where required there is nothing more to do here. So, once you make sure that spaces exist, you're done. I am not sure what the next step is going to be if this is not intended to be merged into master.

comment:15 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 2 to 1
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 60 to 61

I fixed the return(s.
I created ticket #4512 to add conversion functions for hardware address source.
I have sent out an email asking for input onf the DECLINE/RELEASE question and about how to repo the code.

This ticket is complete.

Note: See TracTickets for help on using tickets.