Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jan 10, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2a885cb
Fix for issue 1043 - obfuscate the AI data on request redirect.
cristinamanum Jan 4, 2018
53311e8
Clean-up
cristinamanum Jan 4, 2018
85b1b48
PR feedback
cristinamanum Jan 8, 2018
b0c018b
Remove regex usage and add obfuscation paths on routes.
cristinamanum Jan 9, 2018
81bb4e2
Merge branch 'dev' into cmanu1043
cristinamanum Jan 9, 2018
7b1d98c
PR feedback
cristinamanum Jan 9, 2018
62e8928
pr feedback
cristinamanum Jan 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 6 additions & 25 deletions src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.ApplicationInsights.Extensibility;
Expand All @@ -30,29 +27,13 @@ private void ModifyItem(ITelemetry item)
var requestTelemetryItem = item as RequestTelemetry;
if(requestTelemetryItem != null)
{
requestTelemetryItem.Url = ObfuscateUri(requestTelemetryItem);
requestTelemetryItem.Url = Obfuscator.ObfuscateUrl(requestTelemetryItem.Url);
requestTelemetryItem.Name = Obfuscator.Obfuscate(requestTelemetryItem.Name);
if(requestTelemetryItem.Context.Operation != null)
{
requestTelemetryItem.Context.Operation.Name = Obfuscator.Obfuscate(requestTelemetryItem.Context.Operation.Name);
}
}
}

private Uri ObfuscateUri(RequestTelemetry telemetryItem)
{
if(IsPIIOperation(telemetryItem.Context.Operation.Name))
{
// The new url form will be: https://nuget.org/ObfuscatedUserName
return new Uri(Obfuscator.DefaultObfuscatedUrl(telemetryItem.Url));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be updated if needed in a different change.

}
return telemetryItem.Url;
}

protected virtual bool IsPIIOperation(string operationName)
{
if(string.IsNullOrEmpty(operationName))
{
return false;
}
// Remove the verb from the operation name.
// An example of operationName : GET Users/Profiles
return Obfuscator.ObfuscatedActions.Contains(operationName.Split(' ').Last());
}
}
}
74 changes: 73 additions & 1 deletion src/NuGetGallery/Telemetry/Obfuscator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,37 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

namespace NuGetGallery
{
internal struct ObfuscateMetadata
{
/// <summary>
/// A <see cref="System.Text.RegularExpressions.Regex" to be used for matching the value to be obfuscated./>
/// </summary>
public string ObfuscateTemplate
{
get;
}

/// <summary>
/// The obfuscation value.
/// </summary>
public string ObfuscateValue
{
get;
}

public ObfuscateMetadata(string obfuscateTemplate, string obfuscateValue)
{
ObfuscateTemplate = obfuscateTemplate;
ObfuscateValue = obfuscateValue;
}
}

internal static class Obfuscator
{
/// <summary>
Expand All @@ -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")},
Copy link
Member

Choose a reason for hiding this comment

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

We should probably compile these regular expressions.

Copy link
Contributor

@loic-sharma loic-sharma Jan 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Nice read, good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{@"/packages/(.+)/owners/(.+)/reject/(.+)", new ObfuscateMetadata(@"/owners/(.+)/reject", $"/owners/{DefaultTelemetryUserName}/reject") },
{@"/packages/(.+)/owners/(.+)/cancel/(.+)", new ObfuscateMetadata(@"/owners/(.+)/cancel", $"/owners/{DefaultTelemetryUserName}/cancel") },
{@"/account/confirm/(.+)/(.+)", new ObfuscateMetadata(@"/account/confirm/(.+)/", $"/account/confirm/{DefaultTelemetryUserName}/") },
{@"/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}/") }
Copy link
Member

Choose a reason for hiding this comment

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

Why @?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};

internal static string DefaultObfuscatedUrl(Uri url)
{
return url == null ? string.Empty : $"{url.Scheme}://{url.Host}/{DefaultTelemetryUserName}";
}

internal static Uri ObfuscateUrl(Uri url)
{
if (url == null)
{
return url;
}
return new Uri($"{url.Scheme}://{url.Host}{Obfuscate(url.AbsolutePath)}");
Copy link
Member

Choose a reason for hiding this comment

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

Do we lose port?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the port.

}

internal static string Obfuscate(string value)
{
string obfuscatedTemplateKey = null;
if (value == null || !NeedsObfuscation(value, out obfuscatedTemplateKey))
{
return value;
}
string obfuscatedValue = Regex.Replace(value,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added case insensitive option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add timeouts to all regex matches: #4038

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the timeouts.

ObfuscatedTemplates[obfuscatedTemplateKey].ObfuscateTemplate,
ObfuscatedTemplates[obfuscatedTemplateKey].ObfuscateValue);
return obfuscatedValue;
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Use case insensitive regex instead of ToLower()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (match.Key == null)
{
return false;
}
obfuscatedTemplateKey = match.Key;
return true;
}
}
}
110 changes: 52 additions & 58 deletions tests/NuGetGallery.Facts/Telemetry/ClientTelemetryPIIProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,68 +25,59 @@ public class ClientTelemetryPIIProcessorTests
public void NullTelemetryItemDoesNotThorw()
{
// Arange
string userName = "user1";
var piiProcessor = CreatePIIProcessor(false, userName);
var piiProcessor = CreatePIIProcessor();

// Act
piiProcessor.Process(null);
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public void UrlIsUpdatedOnPIIAction(bool actionIsPII)
[MemberData(nameof(PIIUrlDataGenerator))]
public void UrlIsUpdatedOnPIIAction(string inputUrl, string expectedOutputUrl)
{
// Arange
string userName = "user1";
var piiProcessor = CreatePIIProcessor(actionIsPII, userName);
var piiProcessor = CreatePIIProcessor();
RequestTelemetry telemetryItem = new RequestTelemetry();
telemetryItem.Url = new Uri("https://localhost/user1");
telemetryItem.Url = new Uri(inputUrl);
telemetryItem.Name = $"GET {telemetryItem.Url.AbsolutePath}";

// Act
piiProcessor.Process(telemetryItem);

// Assert
string expected = actionIsPII ? $"https://localhost/{Obfuscator.DefaultTelemetryUserName}" : telemetryItem.Url.ToString();
Assert.Equal(expected, telemetryItem.Url.ToString());
Assert.Equal(expectedOutputUrl, telemetryItem.Url.ToString());
Assert.Equal($"GET {(new Uri(expectedOutputUrl)).AbsolutePath}", $"GET {telemetryItem.Url.AbsolutePath}");
}

[Theory]
[MemberData(nameof(PIIOperationDataGenerator))]
public void TestValidPIIOperations(string operation)
{
// Arange
var piiProcessor = (TestClientTelemetryPIIProcessor)CreatePIIProcessor(false, "user");

// Act and Assert
Assert.True(piiProcessor.IsPIIOperationBase(operation));
}

[Theory]
[MemberData(nameof(InvalidPIIOPerationDataGenerator))]
public void TestInvalidPIIOperations(string operation)
[Fact]
public void ValidatePIIActions()
{
// Arange
var piiProcessor = (TestClientTelemetryPIIProcessor)CreatePIIProcessor(false, "user");
HashSet<string> existentPIIOperations = Obfuscator.ObfuscatedActions;
List<string> piiOperationsFromRoutes = GetPIIOperationsFromRoute();

// Act and Assert
Assert.False(piiProcessor.IsPIIOperationBase(operation));
Assert.True(existentPIIOperations.SetEquals(piiOperationsFromRoutes));
}

[Fact]
public void ValidatePIIRoutes()
{
// Arange
HashSet<string> existentPIIOperations = Obfuscator.ObfuscatedActions;
List<string> piiOperationsFromRoutes = GetPIIOperationsFromRoute();
List<string> piiUrlRoutes = GetPIIRoutesUrls();

// Act and Assert
Assert.True(existentPIIOperations.SetEquals(piiOperationsFromRoutes));
string templateKey;
foreach (var route in piiUrlRoutes)
{
var match = Obfuscator.NeedsObfuscation($"/{route}", out templateKey);
Assert.True(match, $"Route {route} did not match.");
}
}

private ClientTelemetryPIIProcessor CreatePIIProcessor(bool isPIIOperation, string userName)
private ClientTelemetryPIIProcessor CreatePIIProcessor()
{
return new TestClientTelemetryPIIProcessor(new TestProcessorNext(), isPIIOperation, userName);
return new ClientTelemetryPIIProcessor(new TestProcessorNext());
}

private class TestProcessorNext : ITelemetryProcessor
Expand All @@ -96,29 +87,25 @@ public void Process(ITelemetry item)
}
}

private class TestClientTelemetryPIIProcessor : ClientTelemetryPIIProcessor
private static List<string> GetPIIOperationsFromRoute()
{
private User _testUser;
private bool _isPIIOperation;

public TestClientTelemetryPIIProcessor(ITelemetryProcessor next, bool isPIIOperation, string userName) : base(next)
{
_isPIIOperation = isPIIOperation;
_testUser = new User(userName);
}
var currentRoutes = new RouteCollection();
NuGetGallery.Routes.RegisterApiV2Routes(currentRoutes);
NuGetGallery.Routes.RegisterUIRoutes(currentRoutes);

protected override bool IsPIIOperation(string operationName)
var piiRoutes = currentRoutes.Where((r) =>
{
return _isPIIOperation;
}
Route webRoute = r as Route;
return webRoute != null ? IsPIIUrl(webRoute.Url.ToString()) : false;
}).Select((r) => {
var dd = ((Route)r).Defaults;
return $"{dd["controller"]}/{dd["action"]}";
}).Distinct().ToList();

public bool IsPIIOperationBase(string operationName)
{
return base.IsPIIOperation(operationName);
}
return piiRoutes;
}

private static List<string> GetPIIOperationsFromRoute()
private static List<string> GetPIIRoutesUrls()
{
var currentRoutes = new RouteCollection();
NuGetGallery.Routes.RegisterApiV2Routes(currentRoutes);
Expand All @@ -128,10 +115,7 @@ private static List<string> GetPIIOperationsFromRoute()
{
Route webRoute = r as Route;
return webRoute != null ? IsPIIUrl(webRoute.Url.ToString()) : false;
}).Select((r)=> {
var dd = ((Route)r).Defaults;
return $"{dd["controller"]}/{dd["action"]}";
}).Distinct().ToList();
}).Select((r) => ((Route)r).Url).Distinct().ToList();

return piiRoutes;
}
Expand All @@ -141,16 +125,26 @@ private static bool IsPIIUrl(string url)
return url.ToLower().Contains("username") || url.ToLower().Contains("accountname");
}

public static IEnumerable<string[]> PIIOperationDataGenerator()
public static IEnumerable<string[]> PIIUrlDataGenerator()
{
return GetPIIOperationsFromRoute().Select(o => new string[]{$"GET {o}"});
foreach (var user in GenerateUserNames())
{
yield return new string[] { $"https://localhost/packages/pack1/owners/{user}/confirm/sometoken", $"https://localhost/packages/pack1/owners/ObfuscatedUserName/confirm/sometoken" };
yield return new string[] { $"https://localhost/packages/pack1/owners/{user}/reject/sometoken", $"https://localhost/packages/pack1/owners/ObfuscatedUserName/reject/sometoken" };
yield return new string[] { $"https://localhost/packages/pack1/owners/{user}/cancel/sometoken", $"https://localhost/packages/pack1/owners/ObfuscatedUserName/cancel/sometoken" };

yield return new string[] { $"https://localhost/account/confirm/{user}/sometoken", $"https://localhost/account/confirm/ObfuscatedUserName/sometoken" };
yield return new string[] { "https://localhost/account/delete/{user}", $"https://localhost/account/delete/ObfuscatedUserName" };

yield return new string[] { $"https://localhost/profiles/{user}", $"https://localhost/profiles/ObfuscatedUserName" };
yield return new string[] { $"https://localhost/account/setpassword/{user}/sometoken", $"https://localhost/account/setpassword/ObfuscatedUserName/sometoken" };
yield return new string[] { $"https://localhost/account/forgotpassword/{user}/sometoken", $"https://localhost/account/forgotpassword/ObfuscatedUserName/sometoken" };
}
}

public static IEnumerable<string[]> InvalidPIIOPerationDataGenerator()
public static List<string> GenerateUserNames()
{
yield return new string[]{ null };
yield return new string[]{ string.Empty };
yield return new string[]{"Some random data" };
return new List<string>{ "user1", "user.1", "user_1", "user-1"};
}
}
}