Problem/Motivation

Sometimes we want to be able to log messages which contain encrypted data. We can't use watchdog for this and also we want to keep it seperated from all the other messages which watchdog generates so we can give access to supereditors to this logs.

Proposed resolution

We already have dvg_logger, which currently is only used for:

* log failed login attempts
* log the change of the test BSN number

If we expand this functionality a bit and create an UI, we will be able to enhance dvg_logger for our purposes.

Remaining tasks

* Add encrypted field to dvg_logger table
* add $encrypted parameter to _dvg_logger_register_log()
* create UI
* create permissions
* create update hook

User interface changes

New user interface for admins and supereditors at:
/admin/reports/dvg-logger

API changes

New parameter $encrypted:

_dvg_logger_register_log($type, $name, $encrypted = FALSE);

Data model changes

New field "encrypted" (boolean) in table dvg_logger.

CommentFileSizeAuthor
#20 dvg-fix_short_array_syntax-2930491-20.patch635 bytespaulvandenburg
#14 interdiff-2930491-13-14.txt558 bytespaulvandenburg
#14 dvg-custom_encrypted_logger-2930491-14.patch11.63 KBpaulvandenburg
#13 2930491-13.patch11.59 KBDanny.Wouters
#13 interdiff-2930491-10-13.txt1.71 KBDanny.Wouters
#10 interdiff-2930491-8-9.txt536 bytesDanny.Wouters
#10 2930491-9.patch11.7 KBDanny.Wouters
#8 interdiff-2930491-7-8.txt520 bytesDanny.Wouters
#8 2930491-8.patch11.69 KBDanny.Wouters
#7 2930491-7.patch12.19 KBDanny.Wouters
#7 interdiff-2930491-5-7.txt1.12 KBDanny.Wouters
#5 interdiff.patch2.86 KBaskibinski
#5 2930491-dvg_logger-5.patch11.91 KBaskibinski
#2 2930491-dvg_logger-2.patch9.62 KBaskibinski

Comments

askibinski created an issue. See original summary.

askibinski’s picture

askibinski commented
Status: Active » Needs review
StatusFileSize
new 2930491-dvg_logger-2.patch9.62 KB
Danny.Wouters’s picture

Danny.Wouters commented
Status: Needs review » Reviewed & tested by the community

The patch works. Log messages with encrypted data are now logged in the dvg_logger table

askibinski’s picture

askibinski commented
Status: Reviewed & tested by the community » Needs work

We need to add a configurable option which types should be mailed. Now, all types are mailed in _dvg_logger_send_email() which would result in our encrypted entries also being mailed.

askibinski’s picture

askibinski commented
Status: Needs work » Needs review
StatusFileSize
new 2930491-dvg_logger-5.patch11.91 KB
new interdiff.patch2.86 KB
askibinski’s picture

askibinski commented
Status: Needs review » Needs work

Needs work:

1. expand options to purge logs. Currently only 1 month, 2 months. Should be 1 to 7 days, 2 weeks, 1 month, etc.
2. our overview query currently fails if the messages are very large because of memory limits:

  $query->fields('dl', [
    'lid',
    'type',
    'message',
    'hostname',
    'timestamp',
    'encrypted',
  ])
    ->extend('TableSort')
    ->extend('PagerDefault')
    ->orderByHeader($header)
    ->limit(25);

We aren't showing message any more in the table overview so this should be removed from the query.

Danny.Wouters’s picture

Danny.Wouters commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2930491-5-7.txt1.12 KB
new 2930491-7.patch12.19 KB

I created a new patch which adds additional time slot options to purge the logs. I also removed the 'message' field from the query

Danny.Wouters’s picture

Danny.Wouters commented
StatusFileSize
new 2930491-8.patch11.69 KB
new interdiff-2930491-7-8.txt520 bytes

I removed the changes to the .info file from the previous patch

heine’s picture

heine commented 
+ $query->condition('dl.type', check_plain($form_state['input']['filter_type']), '=');

Why the check_plain?

+    if ($row->encrypted) {
+      $message = decrypt($row->message);
+    }

Message is no longer used here.

+              _dvg_logger_register_log('dvg_logger_mail', t('E-mailed @period log of type @type', array('@period' => $period, '@type' => $type)));

I wonder why this is t() and not format_string(). If translations are desired, the watchdog approach would be more appropriate.

Is it possible to show only a part of the message (if non-encrypted) in the overview table? That would make the overview more useful. If dvg_logger is also required for debugging using large data dumps, wouldn't it be better to separate messages, message variables & dump? Or in other words; what is the purpose of this module?

Danny.Wouters’s picture

Danny.Wouters commented
StatusFileSize
new 2930491-9.patch11.7 KB
new interdiff-2930491-8-9.txt536 bytes

The function _dvg_logger_register_log didn't use the parameter $encrypted to encrypt the message.
I made a new patch to encrypt the message depending on the value of $encrypted

heine’s picture

heine commented

In addition to #9; dvg_logger_update_7002 uses schema. See https://www.drupal.org/node/150220 for why this will work now, but is not correct.

askibinski’s picture

askibinski commented
Status: Needs review » Needs work

@heine the idea is to extend dvg_logger into a more generic solution for dvg specific logging and this patch is a first step but we need to separate into more types to make the overview and dvg_logger more useful.

The check_plain isn't needed indeed and $message was leftover code from an earlier version.

Danny.Wouters’s picture

Danny.Wouters commented
StatusFileSize
new interdiff-2930491-10-13.txt1.71 KB
new 2930491-13.patch11.59 KB

@Heine thanks for the review. I added your suggestions to a new pach

paulvandenburg’s picture

paulvandenburg commented
Status: Needs work » Needs review
StatusFileSize
new dvg-custom_encrypted_logger-2930491-14.patch11.63 KB
new interdiff-2930491-13-14.txt558 bytes

Fixed @heine's feedback from #11.

I however took a different approach than the linked documentation.
This should mitigate the problem as well, without having to add redundant the schema definitions.

joshahubbers’s picture

joshahubbers commented
Status: Needs review » Reviewed & tested by the community

  • paulvandenburg committed d36fc10 on 7.x-1.x 
    Issue #2930491 by Danny.Wouters, askibinski, paulvandenburg, Heine,...
paulvandenburg’s picture

paulvandenburg commented
Status: Reviewed & tested by the community » Fixed
paulvandenburg’s picture

paulvandenburg commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

paulvandenburg’s picture

paulvandenburg commented
StatusFileSize
new dvg-fix_short_array_syntax-2930491-20.patch635 bytes

This issue accidentally added the short array syntax. Until we've formally dropped php 5.6 support (soon?), we shouldn't do that.
I've already committed this, but here is the patch separately anyway.