[RFC] Diags scrubbing mechanism #1567

danobi · 2017-03-10T19:29:15Z

This should be the squashed commit message:

Diagnostic log scrubbing is intended an extra security measure to prevent sensitive strings from leaking to diagnostic logs. This is provided via a config option proxy.config.diags.scrubs.

For each line that is to be written to diagnostic logs, a regular expression(s) is applied to the line. If there are any matches, the matched substring is replaced with the replacement string.

bryancall · 2017-03-10T19:30:04Z

Please squash the commits.

atsci · 2017-03-10T19:31:12Z

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/34/

atsci · 2017-03-10T19:31:20Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/47/

Diagnostic log scrubbing is intended an extra security measure to prevent sensitive strings from leaking to diagnostic logs. This is provided via a config option proxy.config.diags.scrubs. For each line that is to be written to diagnostic logs, a regular expression(s) is applied to the line. If there are any matches, the matched substring is replaced with the replacement string.

SolidWallOfCode · 2017-03-10T19:33:26Z

doc/admin-guide/files/records.config.en.rst

+
+.. note::
+
+   Multiple captures are not supported. (ie. ``s/foo/bar/g`` is not suppored while ``s/foo/bar/`` is). Furthermore,


"Multiple replacements"? That is, can I have capture groups? More than one capture group? Also, "suppored".

atsci · 2017-03-10T19:33:41Z

FreeBSD11 build failed! https://ci.trafficserver.apache.org/job/freebsd-github/1729/

atsci · 2017-03-10T19:36:15Z

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/35/

atsci · 2017-03-10T19:36:21Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/48/

atsci · 2017-03-10T19:37:12Z

FreeBSD11 build failed! https://ci.trafficserver.apache.org/job/freebsd-github/1730/

SolidWallOfCode · 2017-03-10T19:38:42Z

lib/ts/Diags.h

+struct Scrub {
+  ~Scrub()
+  {
+    if (pattern) {


ats_scoped_str might be useful here.

atsci · 2017-03-10T19:40:08Z

AU check failed! https://ci.trafficserver.apache.org/job/autest-github/33/

SolidWallOfCode · 2017-03-10T19:40:59Z

Lots of allocation to be removed. This should be done without any allocation.

atsci · 2017-03-10T19:41:06Z

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/36/

atsci · 2017-03-10T19:41:54Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/49/

atsci · 2017-03-10T19:42:13Z

FreeBSD11 build failed! https://ci.trafficserver.apache.org/job/freebsd-github/1731/

atsci · 2017-03-10T19:43:22Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/159/

atsci · 2017-03-10T19:45:13Z

AU check failed! https://ci.trafficserver.apache.org/job/autest-github/34/

atsci · 2017-03-10T19:46:16Z

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1624/

atsci · 2017-03-10T19:48:21Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/160/

atsci · 2017-03-10T19:50:57Z

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1625/

SolidWallOfCode · 2017-03-10T19:57:08Z

Spoke with Dan, we'll get the allocations cleaned up before Leif has a stroke.

atsci · 2017-03-10T19:58:02Z

clang-analyzer build failed! https://ci.trafficserver.apache.org/job/clang-analyzer-github/291/

atsci · 2017-03-10T20:02:19Z

clang-analyzer build failed! https://ci.trafficserver.apache.org/job/clang-analyzer-github/292/

danobi · 2017-03-10T23:02:37Z

The last commit gets error.log scrubbed too. I'll fix the allocation stuff on Tuesday. Nobody tell Leif.

atsci · 2017-03-14T22:57:37Z

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1757/

atsci · 2017-03-14T22:58:58Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/188/

atsci · 2017-03-14T23:12:48Z

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/320/

SolidWallOfCode · 2017-03-16T15:17:55Z

lib/ts/Diags.h

@@ -226,6 +242,10 @@ class Diags
  mutable ink_mutex tag_table_lock; // prevents reconfig/read races
  DFA *activated_tags[2];           // 1 table for debug, 1 for action

+  // Scrubbing variables -- replaces predefined regexp matches with the replacement string
+  bool scrub_enabled = false;
+  Scrubber *scrubber = nullptr;


This is a good place to use std::unique_ptr.

As discussed, due to namespace issues in Vec.h, using unique_ptr is more trouble than it's worth.

My long range plan is KIWF on Vec.h so we'll live with this for now.

SolidWallOfCode · 2017-03-16T15:18:41Z

lib/ts/Scrubber.cc

+Scrubber::Scrubber(const char *config)
+{
+  bool state_expecting_regex = true;
+  const ts::StringView delimiters("->;,", ts::StringView::literal);


Make this static also, so that it's constructed at process start.

SolidWallOfCode · 2017-03-16T15:19:35Z

lib/ts/Scrubber.cc

+  const ts::StringView delimiters("->;,", ts::StringView::literal);
+  ts::StringView regex;
+  ts::StringView replacement;
+  ts::StringView text(config);


This is problematic, because the StringView will be pointing in to external memory. This needs to point to the local memory in this->config.

SolidWallOfCode · 2017-03-16T15:20:17Z

lib/ts/Scrubber.cc

+
+  // This means that there was some kind of config or parse error.
+  // This doesn't catch *all* errors, and is just a heuristic so we can't actually print any meaningful message.
+  if ((regex && !replacement) || (!regex && replacement)) {


Why check this, if nothing is done?

SolidWallOfCode · 2017-03-16T15:25:01Z

lib/ts/Scrubber.cc

+    memmove(buffer_ptr, buffer + scrub->ovector[1], buffer_len - scrub->ovector[1] + 1);
+
+    // the last char should ALWAYS be a '\0' otherwise we did our math wrong
+    ink_assert(buffer[strlen(buffer)] == '\0');


This is expensive and pointless - by definition, strlen will find a null so this will always be true, unless it segfaults. The index should be computed from the other lengths, not searched for, if you want to verify your math.

I must have been asleep at the wheel here.

SolidWallOfCode · 2017-03-16T17:27:20Z

lib/ts/Scrubber.cc

+    int n_slide = buffer_len - scrub->ovector[0] - replacement_len;
+    if (n_slide < 0) {
+      // replacement string is too long, we need to shorten it
+      replacement_len -= -n_slide;


-= - instead of +=?

SolidWallOfCode · 2017-03-16T17:29:18Z

lib/ts/Scrubber.h

+      pcre_free(const_cast<pcre *>(compiled_re));
+    }
+  }
+  static const int OVECCOUNT = 30;


Where does 30 come from? POOMA?

It was used in the PCRE example documentation. Really this could be any value >= 3.

SolidWallOfCode · 2017-03-16T17:30:22Z

lib/ts/Scrubber.h

+   */
+  void scrub_buffer(char *buffer, Scrub *scrub) const;
+
+  char *config = nullptr;


I recommend using ats_scoped_str here, to avoid having to explicitly clean up.

SolidWallOfCode · 2017-03-16T17:31:55Z

proxy/logging/LogObject.h

@@ -299,6 +299,10 @@ class LogObject : public RefCountObj
  unsigned m_buffer_manager_idx;
  LogBufferManager *m_buffer_manager;

+  // scrubbing data -- helps us scrub buffers of sensitive data
+  bool scrub_enabled = false;


Are both of these needed? Does not scubber suffice by being null or not null?

Maybe. Depends on if we want to let users toggle scrubbing at runtime.

Hmmm, I'd need to think about if that make sense.

SolidWallOfCode · 2017-03-16T17:33:11Z

Looking better.

danobi · 2017-03-16T20:41:09Z

Personally, I don't think this feature has much use in open source. If nothing else, it probably encourages poor security practices. Even with this patch, someone could still do a fprintf(stdout, "%s\n", sensitive_data").

bryancall · 2017-06-08T18:07:21Z

[approve ci]

zwoop · 2017-07-20T14:45:03Z

What's the future of this PR? Are we planning on landing this? Is it still WIP?

zwoop · 2017-07-20T14:45:15Z

[approve ci]

danobi · 2017-07-26T17:14:53Z

IIRC this patch is ready to merge. There are no performance penalties for not using this feature (other than a few branch mispredicts, if that). It's a nifty feature to have in my opinion.

If anyone else is +1 on this I'm willing to merge this and potentially apply fixes if it breaks in the future.

bryancall · 2017-08-16T16:12:03Z

I really think this should be done on post processing and this adds more complexity to the proxy.

zwoop · 2017-08-16T16:12:35Z

Since diagnostics is something you generally do not run as part of a production environment (due to performance), it feels to me that a better approach is an external tool (perl or python script) that scrubs the logs. That would be a lot easier to maintain and add features to, and could have a lot more features as well.

bryancall added the Logging label Mar 10, 2017

bryancall assigned danobi Mar 10, 2017

SolidWallOfCode reviewed Mar 10, 2017

View reviewed changes

danobi force-pushed the diags_scrubbing branch from 575d3d1 to 70c8b3b Compare March 10, 2017 19:33

Fix typos

6436c2e

SolidWallOfCode reviewed Mar 10, 2017

View reviewed changes

lib/ts/Diags.h Outdated

struct Scrub {

~Scrub()

{

if (pattern) {

Copy link

Member

SolidWallOfCode Mar 10, 2017

Choose a reason for hiding this comment

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

ats_scoped_str might be useful here.

SolidWallOfCode added this to the 8.0.0 milestone Mar 10, 2017

Create separate Scrubber library and scrub error.log

4316ca9

SolidWallOfCode reviewed Mar 16, 2017

View reviewed changes

Code review fixes

3205d29

apache deleted a comment from atsci Jun 8, 2017

bryancall closed this Aug 16, 2017

zwoop removed this from the 8.0.0 milestone May 17, 2018


		.. note::

		Multiple captures are not supported. (ie. ``s/foo/bar/g`` is not suppored while ``s/foo/bar/`` is). Furthermore,

[RFC] Diags scrubbing mechanism #1567

[RFC] Diags scrubbing mechanism #1567

Conversation

danobi commented Mar 10, 2017

bryancall commented Mar 10, 2017 • edited

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

Choose a reason for hiding this comment

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

Choose a reason for hiding this comment

atsci commented Mar 10, 2017

SolidWallOfCode commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

SolidWallOfCode commented Mar 10, 2017 • edited

atsci commented Mar 10, 2017

atsci commented Mar 10, 2017

danobi commented Mar 10, 2017

atsci commented Mar 14, 2017

atsci commented Mar 14, 2017

atsci commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SolidWallOfCode commented Mar 16, 2017

danobi commented Mar 16, 2017

bryancall commented Jun 8, 2017

zwoop commented Jul 20, 2017

zwoop commented Jul 20, 2017

danobi commented Jul 26, 2017

bryancall commented Aug 16, 2017

zwoop commented Aug 16, 2017

bryancall commented Mar 10, 2017 •

edited

SolidWallOfCode commented Mar 10, 2017 •

edited