Fix issue 1043: Obfuscate AI data on request redirect #5236

cristinamanum · 2018-01-04T22:48:49Z

The AI data can contain user names. The user names are in general on the request url field but in request redirect case it can be found on the name and operation_Name fields as well.

With this change:

The redirect requests data will be obfuscated
A name like: GET /profiles/cristinamanu will be changed in GET /profiles/ObfuscatedUserName
An url like: http://localhost/profiles/cristinamanu will be changed in http://localhost/profiles/ObfuscatedUserName
A operation_Name like: GET /profiles/cristinamanu will be changed in GET /profiles/ObfuscatedUserName
The not redirected requests will be obfuscated and kept in sync with the above structure. A snapshot of the obfuscated values:

joelverhagen · 2018-01-04T23:58:58Z

src/NuGetGallery/Telemetry/Obfuscator.cs

+        internal static bool NeedsObfuscation(string valueToBeVerified, out string obfuscatedTemplateKey)
+        {
+            obfuscatedTemplateKey = string.Empty;
+            var match = ObfuscatedTemplates.Where(template => Regex.IsMatch(valueToBeVerified.ToLower(), template.Key)).FirstOrDefault();


Please add an OrderBy to the static so that order is defined. Dictionaries don't have a defined order AFAIK

Use case insensitive regex instead of ToLower()

Updated to use case insensitive.
Why to orderby? The return is only one element.

joelverhagen · 2018-01-05T00:00:51Z

src/NuGetGallery/Telemetry/Obfuscator.cs

@@ -25,9 +51,55 @@ internal static class Obfuscator
            "Users/Profiles",
            "Users/ResetPassword"};

+        internal static readonly Dictionary<string, ObfuscateMetadata> ObfuscatedTemplates = new Dictionary<string, ObfuscateMetadata>
+        {
+            {@"/packages/(.+)/owners/(.+)/confirm/(.+)", new ObfuscateMetadata(@"/owners/(.+)/confirm", $"/owners/{DefaultTelemetryUserName}/confirm")},


We should probably compile these regular expressions.

Do we already use compiled regex expressions in the gallery? .NET only caches up to 15 compiled regex expressions by default (source here), and compiling a regex is rather expensive.

TLDR: Compiling regex may result in worse performance, we should do some due diligence before enabling it.

Nice read, good point.

I did simple perf tests with: static regex and compiled instance regex . The results show similar perf characteristics.
For invocation of 1000 https://localhost/profiles/cristinamanu
Static - 525.6479932 seconds
Instance Compiled - 525.0719887 seconds

joelverhagen · 2018-01-05T00:01:32Z

src/NuGetGallery/Telemetry/Obfuscator.cs

+            {@"/account/delete/(.+)", new ObfuscateMetadata(@"/account/delete/(.+)", $"/account/delete/{DefaultTelemetryUserName}") },
+            {@"/profiles/(.+)", new ObfuscateMetadata(@"/profiles/(.+)", $"/profiles/{DefaultTelemetryUserName}") },
+            {@"/account/setpassword/(.+)/(.+)", new ObfuscateMetadata(@"/setpassword/(.+)/", $"/setpassword/{DefaultTelemetryUserName}/") },
+            {@"/account/forgotpassword/(.+)/(.+)", new ObfuscateMetadata(@"/forgotpassword/(.+)/", $"/forgotpassword/{DefaultTelemetryUserName}/") }


I tend to use verbatim string when working with regex expressions. For these regex there is no difference.

joelverhagen · 2018-01-05T00:02:44Z

src/NuGetGallery/Telemetry/Obfuscator.cs

+            {
+                return value;
+            }
+            string obfuscatedValue = Regex.Replace(value,


Is this case sensitive? I think our URLs are:
https://www.nuget.org/PROFILES/jamesnk

I added case insensitive option.

joelverhagen

⌚️

joelverhagen · 2018-01-05T00:04:21Z

src/NuGetGallery/Telemetry/Obfuscator.cs

+            {
+                return url;
+            }
+            return new Uri($"{url.Scheme}://{url.Host}{Obfuscate(url.AbsolutePath)}");


Do we lose port?

(e.g. if a customer is running gallery on a non-standard port)

Added the port.

chenriksson · 2018-01-05T20:40:49Z

@cristinamanum string regex for obfuscating urls seems fragile and error-prone. Would it be possible instead to just use the existing request's RouteData? Maybe we could even inject logic when mapping routes to specify what should be obfuscated?

Very rough idea of what I'm thinking...

class Routes {
    routes.MapRoute(
        name: RouteName.Profile,
        url: "profiles/{username}",
        defaults: new { controller = "Users", action = "Profiles" },
        obfuscations: new { username = 'ObfuscatedUsername' }
}

static class NuGetRouteExtensions {

    public static Dictionary<string, Dictionary<string, string>> UrlObfuscations;

    // using Dictionary, or could use object ViewBag to be consistent with MVC routing APIs
    public static Route MapRoute(this RouteCollection routes, string name, string url, object defaults, Dictionary<string, string> obfuscations) {
        if (obfuscations != null) {
            Obfuscator.UrlObfuscations[url] = obfuscations;
        }
    }

    public static string ObfuscateUri(this Route route, string actualUri) {
        if (UrlObfuscations.ContainsKey(route.Uri) {
            var obfuscations = UrlObfuscations[route.Uri];
            var parts = routeUri.Split('/');
            var actualParts = actualUri.Split('/');
            for (int i = 0; i < parts.length; i++) {
                if (obfuscations.ContainsKey(parts[i])) {
                    actualParts[i] = obfuscations[parts[i]];
                }
            }
            return actualParts.Join('/');
        }
    }
}

skofman1 · 2018-01-05T21:10:51Z

src/NuGetGallery/Telemetry/Obfuscator.cs

+            {
+                return value;
+            }
+            string obfuscatedValue = Regex.Replace(value,


Add timeouts to all regex matches: #4038

Added the timeouts.

chenriksson · 2018-01-09T05:39:22Z

src/NuGetGallery/Extensions/RouteExtensions.cs

+
+        public static string ObfuscateUrlPath(this Route route, string urlPath)
+        {
+            var path = route.Url;


nit: extra single whitespace? Or maybe it's the if indentation below?

chenriksson · 2018-01-09T05:39:50Z

src/NuGetGallery/Extensions/RouteExtensions.cs

+
+namespace NuGetGallery
+{
+    public static class RouteExtensions


nit: class could be internal (tests already have internalsVisibleTo)

chenriksson · 2018-01-09T05:42:29Z

src/NuGetGallery/Extensions/RouteExtensions.cs

+            }
+        }
+
+        internal static Dictionary<string, ObfuscatedMetadata> ObfuscatedRouteMap = new Dictionary<string, ObfuscatedMetadata>();


nit: maybe name RoutePartsToObfuscateByUrl is clearer?

I think I like more the current name.

chenriksson · 2018-01-09T05:45:59Z

src/NuGetGallery/Extensions/RouteExtensions.cs

+            var metadata = ObfuscatedRouteMap[path];
+            string[] segments = urlPath.Split('/');
+            segments[metadata.ObfuscatedSegment] = metadata.ObfuscateValue;
+            return segments.Aggregate((x, y) => { return x + "/" + y; });


Is this equivalent to String.Join('/', segments)? Is Join simpler?

Yep - join has better perf than aggregate - updated.

chenriksson · 2018-01-09T05:46:32Z

src/NuGetGallery/Helpers/RegexEx.cs

@@ -8,7 +8,7 @@ namespace NuGetGallery.Helpers
 {
    public static class RegexEx
    {
-        private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(2);
+        internal static readonly TimeSpan Timeout = TimeSpan.FromSeconds(2);


still necessary?

no - updated

chenriksson · 2018-01-09T05:49:19Z

src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs

            {
-                // The new url form will be: https://nuget.org/ObfuscatedUserName
-                return new Uri(Obfuscator.DefaultObfuscatedUrl(telemetryItem.Url));
+                return RouteTable.Routes.GetRouteData(new System.Web.HttpContextWrapper(HttpContext.Current)).Route as Route;


nit: namespace unnecessary since 'System.Web' is imported?
nit: maybe this is complex enough to be method like GetCurrentRouteData?

Comment 1 : it was not needed - updated
Comment 2 : I agree method is better suited - updated

chenriksson · 2018-01-09T05:51:31Z

src/NuGetGallery/Telemetry/Obfuscator.cs

 using System.Collections.Generic;
+using System.Linq;
+using System.Text.RegularExpressions;
+using NuGetGallery.Helpers;


nit: can revert?

yes - updated

chenriksson · 2018-01-09T05:58:37Z

src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs

            {
-                // The new url form will be: https://nuget.org/ObfuscatedUserName
-                return new Uri(Obfuscator.DefaultObfuscatedUrl(telemetryItem.Url));


DefaultObfuscatedUrl was also called by QuietLog.GetObfuscatedServerVariables. Can this use the same technique?

It will be updated if needed in a different change.

chenriksson · 2018-01-09T06:00:26Z

@cristinamanum Looks really nice!

skofman1 · 2018-01-09T22:54:56Z

src/NuGetGallery/Extensions/RouteExtensions.cs

+        public static void MapRoute(this RouteCollection routes, string name, string url, object defaults, ObfuscatedMetadata obfuscationMetadata)
+        {
+            routes.MapRoute(name, url, defaults);
+            if (!ObfuscatedRouteMap.ContainsKey(url)) { ObfuscatedRouteMap.Add(url, obfuscationMetadata); }


nit: { should be in a new line

skofman1

shishirx34 · 2018-01-09T23:00:08Z

src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs

+                    return;
+                }
+                // Removes the first /
+                var requestPath = requestTelemetryItem.Url.AbsolutePath.Remove(0,1);


probably a good idea to test if the first char is '/' before removal?

Maybe use requestTelemetryItem.Url.AbsolutePath.TrimStart('/')?

I tested
updated with TrimStart

shishirx34 · 2018-01-09T23:02:51Z

src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs

+                {
+                    requestTelemetryItem.Url = new Uri(requestTelemetryItem.Url.ToString().Replace(requestPath, obfuscatedPath));
+                    requestTelemetryItem.Name = requestTelemetryItem.Name.Replace(requestPath, obfuscatedPath);
+                    if(requestTelemetryItem.Context.Operation != null && requestTelemetryItem.Context.Operation.Name != null)


you can simplify this: if (requestTelemetryItem.Context.Operation?.Name != null)

nicer - updated

cristinamanum added 2 commits January 4, 2018 14:10

Fix for issue 1043 - obfuscate the AI data on request redirect.

2a885cb

Clean-up

53311e8

cristinamanum requested review from chenriksson, agr, shishirx34, joelverhagen, ryuyu, scottbommarito, skofman1, loic-sharma and xavierdecoster January 4, 2018 22:49

joelverhagen reviewed Jan 4, 2018

View reviewed changes

joelverhagen reviewed Jan 5, 2018

View reviewed changes

joelverhagen requested changes Jan 5, 2018

View reviewed changes

joelverhagen reviewed Jan 5, 2018

View reviewed changes

skofman1 reviewed Jan 5, 2018

View reviewed changes

cristinamanum added 2 commits January 7, 2018 18:27

PR feedback

85b1b48

Remove regex usage and add obfuscation paths on routes.

b0c018b

chenriksson approved these changes Jan 9, 2018

View reviewed changes

cristinamanum added 2 commits January 9, 2018 11:50

Merge branch 'dev' into cmanu1043

81bb4e2

PR feedback

7b1d98c

skofman1 reviewed Jan 9, 2018

View reviewed changes

skofman1 approved these changes Jan 9, 2018

View reviewed changes

joelverhagen approved these changes Jan 9, 2018

View reviewed changes

shishirx34 approved these changes Jan 9, 2018

View reviewed changes

pr feedback

62e8928

cristinamanum merged commit 18bc2a1 into dev Jan 10, 2018

chenriksson mentioned this pull request Jan 10, 2018

Obfuscate new transform account route #5260

Merged

cristinamanum deleted the cmanu1043 branch January 11, 2018 18:09

Fix issue 1043: Obfuscate AI data on request redirect #5236

Fix issue 1043: Obfuscate AI data on request redirect #5236

Conversation

cristinamanum commented Jan 4, 2018

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

loic-sharma Jan 5, 2018 • edited Loading

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

joelverhagen left a 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

chenriksson commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson Jan 9, 2018 • edited Loading

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

chenriksson commented Jan 9, 2018

Choose a reason for hiding this comment

skofman1 left a 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

loic-sharma Jan 5, 2018 •

edited

Loading

chenriksson Jan 9, 2018 •

edited

Loading