KETTLE-73: Allow censoring of sensitive information which may be present in URL of DataSource #49

amb26 · 2018-10-17T16:38:47Z

No description provided.

…ent in URL of DataSource

idrc-cms-bot · 2018-10-17T16:44:49Z

CI job failed: https://ci.fluidproject.org/job/kettle-pull-request/9/

idrc-cms-bot · 2018-10-17T16:59:04Z

CI job passed: https://ci.fluidproject.org/job/kettle-pull-request/10/

simonbates · 2018-10-30T19:37:59Z

lib/dataSource-url.js

+    var censorURL = function (url) {
+        sensitiveValues.forEach(function (sensitiveValue) {
+            if (sensitiveValue) {
+                url = url.replace(sensitiveValue, "[CENSORED]");


This straightforward string replacement will fail whenever there is percent-encoding in the URL. For example, if we change the test URL in file tests/DataSourceSimpleTests.js from "https://secret-user:secret-password@thing.available:997/path" to "https://secret-user:secret-%25-password@thing.available:997/path" (that is, a password with value "secret-%-password"), the test will fail.

In this case, the full URL is logged as "https://secret-user:secret-%25-password@thing.available:997/path".

Inside the censorURL function, sensitiveValue is decoded and has the value "secret-user:secret-%-password". Which will not be found in the URL for replacement.

Is it feasible to never log the URL?

An approach might be to encode the value we want to remove and do replacement with the encoded value.

But I fear that this may be difficult to get right as there are multiple possible notations that result in the same decoded value.

It looks like some reserved characters are processed leniently even if they are not escaped. For example, the following URL passes the test:

https://secret-user:secret-=-password@thing.available:997/path

Even though "=" is a reserved character. A URL with another reserved character fails:

https://secret-user:secret-#-password@thing.available:997/path

It's also possible (though less likely) to percent encode unreserved characters. For example the following are all equivalent:

AB

A%42

%41B

%41%42

Thanks for spotting this risk, @simonbates - I decided the readability advantages of logging the URL warranted keeping it, and changed the workflow to regenerate the URL by re-encoding the already censored broken-down fields rather than attempting to censor it in place. Ready for another look - cheers

idrc-cms-bot · 2018-11-21T12:31:05Z

CI job passed: https://ci.fluidproject.org/job/kettle-pull-request/11/

cindyli · 2018-11-30T16:58:07Z

tests/StaticTests.js

@@ -54,12 +54,13 @@ fluid.defaults("kettle.tests.middleware.verifyingUnmarked", {
    gradeNames: ["kettle.plainAsyncMiddleware"],
    middleware: kettle.tests.verifyingUnmarkedMiddleware
 });
-
+fluid.logObjectRenderChars = 10240;


This debug line can be removed.

amb26 · 2018-12-01T17:30:16Z

@cindyli, @simonbates - ready for another look

idrc-cms-bot · 2018-12-01T17:34:46Z

CI job passed: https://ci.fluidproject.org/job/kettle-pull-request/12/

cindyli · 2018-12-03T16:05:36Z

With the current censoring implementation, https://secret-user:secret-password@thing.available:997/path will be logged as https://thing.available:997/path. Also, when {auth: true} is set in censorRequestOptionsLog, the request option auth, along with headers information, will be removed from the log. This feels to me causes a loss of some useful information because having or without having these info will result in the same logged information.

I wonder if it's worthwhile to keep the presence of these options in the log but replacing their values with a string like [SENSITIVE] so we don't leak sensitive values but are still aware these info have been sent.

amb26 · 2018-12-05T14:04:03Z

Thanks for this good suggestion, @cindyli - ready for another look

idrc-cms-bot · 2018-12-05T14:09:05Z

CI job passed: https://ci.fluidproject.org/job/kettle-pull-request/13/

cindyli · 2018-12-05T15:54:06Z

tests/DataSourceSimpleTests.js

 fluid.defaults("kettle.tests.KETTLE34dataSource", {
    gradeNames: "kettle.dataSource.URL",
    url: "https://user:password@thing.available:997/path",
    headers: {
        "x-custom-header": "x-custom-value"
+    },
+    censorRequestOptionsLog: {
+        auth: false


Please add a test for auth: true with sensitive values being replaced.

But that is the default value - is the idea just to test that options merging works correctly?

cindyli · 2018-12-05T16:14:12Z

I understand auth: true is the default value. But I don't find a test that tests it with censoring sensitive options such as "auth" or "header.Authorization". [The only relevant test|https://github.com//pull/49/files#diff-f338ac10c905127921ee7d0c4ab2f01bR73] doesn't have options that need to be censored.

amb26 · 2018-12-05T16:23:26Z

I couldn't follow the link in your comment. This test https://github.com/fluid-project/kettle/pull/49/files#diff-f338ac10c905127921ee7d0c4ab2f01bR142 shows the sensitive option "auth" being censored from the logs

cindyli · 2018-12-05T16:52:46Z

Right. Was thinking to enhance this test to have sensitive options, which doesn't seem necessary with the test you pointed out. Thanks.

Wonder if "header.Authorization" request field should be censored by default in kettle too. In universal, this field holds access tokens when requests are sent to the cloud based flow manager for fetching or saving user settings. See the get endpoint doc for your reference.

amb26 · 2018-12-05T18:15:04Z

@cindyli - good idea, implementation and tests enhanced to allow "deep censoring" - ready for another look

idrc-cms-bot · 2018-12-05T18:20:56Z

CI job passed: https://ci.fluidproject.org/job/kettle-pull-request/14/

cindyli · 2018-12-05T20:43:12Z

docs/DataSources.md

@@ -144,6 +144,15 @@ We document these configuration options in the next section:
            nonexistent file, or an HTTP resource giving a 404) will result in a <code>resolve</code> with an empty
            payload rather than a <code>reject</code> response.</td>
        </tr>
+        <tr>
+            <td><code>censorRequestOptionsLog</code></td>
+            <td><code>Object</code> (map of <code>String</code> to <code>Boolean</code>) (default: <code>{auth: true}</code>)</td>


Add "headers.Authorization": true into the default.

Well caught. Ready for another look

cindyli · 2018-12-05T21:08:02Z

@simonbates, this pull request looks good to me.

idrc-cms-bot · 2018-12-05T21:09:07Z

CI job passed: https://ci.fluidproject.org/job/kettle-pull-request/15/

cindyli · 2018-12-05T21:17:55Z

Merged at 281d9aa

KETTLE-73: Allow censoring of sensitive information which may be pres…

afc6f72

…ent in URL of DataSource

KETTLE-73: Linting and doc fixes

2bba238

simonbates self-requested a review October 17, 2018 17:03

simonbates reviewed Oct 30, 2018

View reviewed changes

KETTLE-73: Improvements to censoring following review

61b83e9

cindyli reviewed Nov 30, 2018

View reviewed changes

cindyli mentioned this pull request Nov 30, 2018

GPII-3551: Improvements and bug fix for /ready and /health endpoint GPII/universal#713

Merged

amb26 mentioned this pull request Nov 30, 2018

Fix app dependencies GPII/gpii-app#69

Merged

KETTLE-73: Removed debugging definition after review

8f737cd

KETTLE-73: Further improvements following review and further dep update

e480098

cindyli reviewed Dec 5, 2018

View reviewed changes

KETTLE-73: Further improvements to censoring following review

6c2adc9

cindyli reviewed Dec 5, 2018

View reviewed changes

KETTLE-73: Doc fixes after review

1fbf663

cindyli merged commit 1fbf663 into fluid-project:master Dec 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KETTLE-73: Allow censoring of sensitive information which may be present in URL of DataSource #49

KETTLE-73: Allow censoring of sensitive information which may be present in URL of DataSource #49

amb26 commented Oct 17, 2018

idrc-cms-bot commented Oct 17, 2018

idrc-cms-bot commented Oct 17, 2018

simonbates Oct 30, 2018

simonbates Oct 30, 2018 •

edited

Loading

amb26 Nov 21, 2018

idrc-cms-bot commented Nov 21, 2018

cindyli Nov 30, 2018

amb26 commented Dec 1, 2018

idrc-cms-bot commented Dec 1, 2018

cindyli commented Dec 3, 2018

amb26 commented Dec 5, 2018

idrc-cms-bot commented Dec 5, 2018

cindyli Dec 5, 2018

amb26 Dec 5, 2018

cindyli commented Dec 5, 2018

amb26 commented Dec 5, 2018

cindyli commented Dec 5, 2018 •

edited

Loading

amb26 commented Dec 5, 2018

idrc-cms-bot commented Dec 5, 2018

cindyli Dec 5, 2018

amb26 Dec 5, 2018

cindyli commented Dec 5, 2018

idrc-cms-bot commented Dec 5, 2018

cindyli commented Dec 5, 2018

KETTLE-73: Allow censoring of sensitive information which may be present in URL of DataSource #49

KETTLE-73: Allow censoring of sensitive information which may be present in URL of DataSource #49

Conversation

amb26 commented Oct 17, 2018

idrc-cms-bot commented Oct 17, 2018

idrc-cms-bot commented Oct 17, 2018

simonbates Oct 30, 2018

Choose a reason for hiding this comment

simonbates Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

amb26 Nov 21, 2018

Choose a reason for hiding this comment

idrc-cms-bot commented Nov 21, 2018

cindyli Nov 30, 2018

Choose a reason for hiding this comment

amb26 commented Dec 1, 2018

idrc-cms-bot commented Dec 1, 2018

cindyli commented Dec 3, 2018

amb26 commented Dec 5, 2018

idrc-cms-bot commented Dec 5, 2018

cindyli Dec 5, 2018

Choose a reason for hiding this comment

amb26 Dec 5, 2018

Choose a reason for hiding this comment

cindyli commented Dec 5, 2018

amb26 commented Dec 5, 2018

cindyli commented Dec 5, 2018 • edited Loading

amb26 commented Dec 5, 2018

idrc-cms-bot commented Dec 5, 2018

cindyli Dec 5, 2018

Choose a reason for hiding this comment

amb26 Dec 5, 2018

Choose a reason for hiding this comment

cindyli commented Dec 5, 2018

idrc-cms-bot commented Dec 5, 2018

cindyli commented Dec 5, 2018

simonbates Oct 30, 2018 •

edited

Loading

cindyli commented Dec 5, 2018 •

edited

Loading