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.

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

* NEW: Add --disable-telemetry argument to disable telemetry collection #975

Merged
merged 11 commits into from
Jan 10, 2024
Merged
1 change: 1 addition & 0 deletions ReleaseHistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
- NEW => new feature
## UNRELEASED
* DEP: Update `Sarif.Sdk` submodule from [bc8cb57 to fd6e615](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...fd6e615). Reference [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/fd6e615/ReleaseHistory.md).
* NEW: Add `--disable-telemetry` argument to disable telemetry collection.
* BUG: Fix `ERR998.ExceptionInAnalyze`: `InvalidOperationException: Unrecognized crypto HRESULT: 0x80096011` for check `BA2022.SignSecurely` when the signature is malformed, by adding missing error code to error description mappings. [969](https://github.com/microsoft/binskim/pull/969)

## **v4.2.1**
Expand Down
5 changes: 5 additions & 0 deletions src/BinSkim.Driver/AnalyzeOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ public class AnalyzeOptions : AnalyzeOptionsBase
HelpText = "If enabled, BinSkim won't break if we have a 'PdbLoadingException'.")]
public bool? IgnorePdbLoadError { get; set; }

[Option(
"disable-telemetry",
HelpText = "If enabled, BinSkim will disable telemetry.")]
public bool? DisableTelemetry { get; set; }

[Option(
's',
"statistics",
Expand Down
8 changes: 7 additions & 1 deletion src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(Analyze
// Update context object based on command-line parameters.
context.SymbolPath = options.SymbolsPath ?? context.SymbolPath;
context.IgnorePdbLoadError = options.IgnorePdbLoadError != null ? options.IgnorePdbLoadError.Value : context.IgnorePdbLoadError;
context.DisableTelemetry = options.DisableTelemetry != null ? options.DisableTelemetry.Value : context.DisableTelemetry;
context.LocalSymbolDirectories = options.LocalSymbolDirectories ?? context.LocalSymbolDirectories;
context.TracePdbLoads = options.Trace.Contains(nameof(Traces.PdbLoad));

Expand Down Expand Up @@ -172,6 +173,11 @@ public override int Run(AnalyzeOptions analyzeOptions)
{
Stopwatch stopwatch = null;

if (analyzeOptions.DisableTelemetry == true)
{
this.Telemetry = null;
Copy link

@scottoneil-ms scottoneil-ms Dec 1, 2023

Choose a reason for hiding this comment

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

This worries me a bit. I see that if it's null, we just don't add a certain logger to an aggregate logger. That looks great.

But then a little bit later, we pass this thing into a method. Does that call flow lead to uses? Is it already nullchecked? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it is,
in fact our application is checking if it is null to see enabled or not,
if null, no Telemetry will be performed.

Copy link
Member

Choose a reason for hiding this comment

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

Scott's right, you haven't gotten this change quite right, which is to enable telemetry collection by default but allow someone to override that via this argument.

Where is the code that makes telemetry work out of box with no special configuration?

}

if (analyzeOptions.Trace.Where(s => s == nameof(DefaultTraces.ScanTime)).Any())
{
stopwatch = Stopwatch.StartNew();
Expand Down Expand Up @@ -243,6 +249,6 @@ public override int Run(AnalyzeOptions analyzeOptions)

internal Sarif.SarifVersion UnitTestOutputVersion { get; set; }

private Sdk.Telemetry Telemetry { get; }
private Sdk.Telemetry Telemetry { get; set; }
}
}
9 changes: 9 additions & 0 deletions src/BinSkim.Sdk/BinSkim.Sdk.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@
</ItemGroup>

<ItemGroup>
<Compile Update="EnvironmentResources.Designer.cs">
<DesignTime>True</DesignTime>
<AutoGen>True</AutoGen>
<DependentUpon>EnvironmentResources.resx</DependentUpon>
</Compile>
<Compile Update="SdkResources.Designer.cs">
<DesignTime>True</DesignTime>
<AutoGen>True</AutoGen>
Expand All @@ -28,6 +33,10 @@
</ItemGroup>

<ItemGroup>
<EmbeddedResource Update="EnvironmentResources.resx">
<Generator>ResXFileCodeGenerator</Generator>
<LastGenOutput>EnvironmentResources.Designer.cs</LastGenOutput>
</EmbeddedResource>
<EmbeddedResource Update="SdkResources.resx">
<Generator>ResXFileCodeGenerator</Generator>
<LastGenOutput>SdkResources.Designer.cs</LastGenOutput>
Expand Down
6 changes: 6 additions & 0 deletions src/BinSkim.Sdk/BinaryAnalyzerContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ public bool IgnorePdbLoadError
set => this.Policy.SetProperty(BinaryParsersProperties.IgnorePdbLoadError, value);
}

public bool DisableTelemetry
{
get => this.Policy?.GetProperty(BinaryParsersProperties.DisableTelemetry) == true;
set => this.Policy.SetProperty(BinaryParsersProperties.DisableTelemetry, value);
}

public bool IncludeWixBinaries
{
get => this.Policy?.GetProperty(BinaryParsersProperties.IncludeWixBinaries) == true;
Expand Down
72 changes: 72 additions & 0 deletions src/BinSkim.Sdk/EnvironmentResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

123 changes: 123 additions & 0 deletions src/BinSkim.Sdk/EnvironmentResources.resx
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link

@scottoneil-ms scottoneil-ms Dec 1, 2023

Choose a reason for hiding this comment

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

I do not know how to review this, you might need a second set of eyes for it. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the .net resource file is auto gen,
the only thing that is worth looking is the DefaultTelemetryConnectionString at the end.

<root>
<!--
Microsoft ResX Schema

Version 2.0

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
<resheader name="reader">System.Resources.ResXResourceReader, System.Windows.Forms, ...</resheader>
<resheader name="writer">System.Resources.ResXResourceWriter, System.Windows.Forms, ...</resheader>
<data name="Name1"><value>this is my long string</value><comment>this is a comment</comment></data>
<data name="Color1" type="System.Drawing.Color, System.Drawing">Blue</data>
<data name="Bitmap1" mimetype="application/x-microsoft.net.object.binary.base64">
<value>[base64 mime encoded serialized .NET Framework object]</value>
</data>
<data name="Icon1" type="System.Drawing.Icon, System.Drawing" mimetype="application/x-microsoft.net.object.bytearray.base64">
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>

There are any number of "resheader" rows that contain simple
name/value pairs.

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
<xsd:schema id="root" xmlns="" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:msdata="urn:schemas-microsoft-com:xml-msdata">
<xsd:import namespace="http://www.w3.org/XML/1998/namespace" />
<xsd:element name="root" msdata:IsDataSet="true">
<xsd:complexType>
<xsd:choice maxOccurs="unbounded">
<xsd:element name="metadata">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" />
</xsd:sequence>
<xsd:attribute name="name" use="required" type="xsd:string" />
<xsd:attribute name="type" type="xsd:string" />
<xsd:attribute name="mimetype" type="xsd:string" />
<xsd:attribute ref="xml:space" />
</xsd:complexType>
</xsd:element>
<xsd:element name="assembly">
<xsd:complexType>
<xsd:attribute name="alias" type="xsd:string" />
<xsd:attribute name="name" type="xsd:string" />
</xsd:complexType>
</xsd:element>
<xsd:element name="data">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
<xsd:element name="comment" type="xsd:string" minOccurs="0" msdata:Ordinal="2" />
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="required" msdata:Ordinal="1" />
<xsd:attribute name="type" type="xsd:string" msdata:Ordinal="3" />
<xsd:attribute name="mimetype" type="xsd:string" msdata:Ordinal="4" />
<xsd:attribute ref="xml:space" />
</xsd:complexType>
</xsd:element>
<xsd:element name="resheader">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="required" />
</xsd:complexType>
</xsd:element>
</xsd:choice>
</xsd:complexType>
</xsd:element>
</xsd:schema>
<resheader name="resmimetype">
<value>text/microsoft-resx</value>
</resheader>
<resheader name="version">
<value>2.0</value>
</resheader>
<resheader name="reader">
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="DefaultTelemetryConnectionString" xml:space="preserve">
<value />
</data>
</root>
7 changes: 7 additions & 0 deletions src/BinSkim.Sdk/Telemetry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,13 @@ private static void ConfigureTelemetryContext(TelemetryContext context)
return "InstrumentationKey=" + appInsightsKey;
}

// Fall back to DefaultTelemetryConnectionString.
string defaultTelemetryConnectionString = EnvironmentResources.DefaultTelemetryConnectionString;
if (!string.IsNullOrWhiteSpace(defaultTelemetryConnectionString))
{
return defaultTelemetryConnectionString;
}

return null;
}

Expand Down
6 changes: 6 additions & 0 deletions src/BinaryParsers/BinaryParsersProperties.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public IEnumerable<IOption> GetOptions()
{
ComprehensiveBinaryParsing,
IgnorePdbLoadError,
DisableTelemetry,
IncludeWixBinaries,
LocalSymbolDirectories,
SymbolPath
Expand All @@ -34,6 +35,11 @@ public IEnumerable<IOption> GetOptions()
"BinaryParsers", nameof(IgnorePdbLoadError), defaultValue: () => false,
"Set this value to 'true' to don't break if we have a 'PdbLoadingException'.");

public static PerLanguageOption<bool> DisableTelemetry { get; } =
new PerLanguageOption<bool>(
"BinaryParsers", nameof(DisableTelemetry), defaultValue: () => false,
"Set this value to 'true' to disable telemetry.");

public static PerLanguageOption<bool> IncludeWixBinaries { get; } =
new PerLanguageOption<bool>(
"BinaryParsers", nameof(IncludeWixBinaries), defaultValue: () => false,
Expand Down
37 changes: 36 additions & 1 deletion src/Test.FunctionalTests.BinSkim.Driver/CommandLineTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
Expand All @@ -10,7 +11,6 @@
using FluentAssertions;

using Microsoft.CodeAnalysis.IL;
using Microsoft.CodeAnalysis.Sarif.Driver;

using Xunit;

Expand Down Expand Up @@ -70,5 +70,40 @@ public void MostlyFunctionlessCommandlineTest()
builder.Length.Should().Be(0,
$"all test cases should pass, but the following test cases failed:\n{builder}");
}

[Fact]
public void DisableTelemetryCommandlineTest()
Copy link

@scottoneil-ms scottoneil-ms Dec 1, 2023

Choose a reason for hiding this comment

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

I don't know what the rest of this suite looks like, and I'm not 100% sure I'm following this test. So tell me if this is here and I'm not seeing it. But: I think you need a test that disables telemetry, runs a scan against a well-known file (I see this much) and then verifies that the scan results are the same as if you had not passed in disable-telemetry (I don't see this part.)
#Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good input. Added another test.

Copy link
Member

Choose a reason for hiding this comment

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

DisableTelemetryCommandlineTest

i don't see any tests that prove that telemetry isn't collected in a tool run where this setting is specified. how do we know it works?

{
var testCases = new List<Tuple<string, bool?>>()
{
new Tuple<string, bool?>(@"analyze C:\Native.exe -o C:\result.sarif", null),
new Tuple<string, bool?>(@"analyze C:\Native.exe -o C:\result.sarif --disable-telemetry true", true),
new Tuple<string, bool?>(@"analyze C:\Native.exe -o C:\result.sarif --disable-telemetry false", false)
};

var builder = new StringBuilder();

foreach (Tuple<string, bool?> testCase in testCases)
{
string[] args = testCase.Item1.Split(' ');
bool parser = new Parser(cfg => cfg.CaseInsensitiveEnumValues = true).ParseArguments<AnalyzeOptions>(args)
.MapResult(
options =>
{
if (options.DisableTelemetry != testCase.Item2)
{
builder.AppendLine($"\u2022 {testCase.Item1}");
}
return true;
},
err =>
{
builder.AppendLine($"\u2022 {testCase.Item1}");
return true;
});
}
builder.Length.Should().Be(0,
$"all test cases should pass, but the following test cases failed:\n{builder}");
}
}
}
Loading
Loading