-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add Audit Logging To SecreteManager.exe #4183
base: main
Are you sure you want to change the base?
Changes from 7 commits
05b8a68
cd9b948
7134128
e661780
87c81eb
7f74cda
80629f8
f71bee0
cc6d25e
db2f714
6676fbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<configuration> | ||
<packageSources> | ||
<clear /> | ||
<add key="AzureGenevaMonitoring" value="https://msblox.pkgs.visualstudio.com/_packaging/AzureGenevaMonitoring/nuget/v3/index.json" /> | ||
<add key="dotnet-public" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What permissions are required to access this feed? Will there be issues with CI and individual users getting access? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is currently an issue with this feed and it blocked the PR build from completing. I have been discussing it with Matt Mitchell to see how it can be resolved. As you observed this feed requires external permissions that are not granted so the package restore is failing. The basic issue is that the Audit team says we have to use the version of the package that comes from 'this' feed and not the public version of the package.
Matt said these two options have been used in the past to deal with other packages that have similar issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good good, Matt should have good ideas here. Just to say it explicitly, do not merge this PR until there's a good solution for public CI and for local dev. |
||
<add key="dotnet-eng" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json" /> | ||
<add key="dotnet-tools" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json" /> | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,23 +1,27 @@ | ||||||
using Microsoft.DncEng.CommandLineLib; | ||||||
using Microsoft.DncEng.SecretManager.Commands; | ||||||
using System.Diagnostics; | ||||||
using System.Reflection; | ||||||
using System.Threading; | ||||||
using System.Threading.Tasks; | ||||||
using Microsoft.DncEng.CommandLineLib; | ||||||
|
||||||
namespace Microsoft.DncEng.SecretManager; | ||||||
|
||||||
[Command("info")] | ||||||
class InfoCommand : Command | ||||||
class InfoCommand : ProjectBaseCommand | ||||||
{ | ||||||
private readonly IConsole _console; | ||||||
|
||||||
public InfoCommand(IConsole console) | ||||||
public InfoCommand(GlobalCommand globalCommand, IConsole console): base(globalCommand) | ||||||
{ | ||||||
_console = console; | ||||||
} | ||||||
|
||||||
public override Task RunAsync(CancellationToken cancellationToken) | ||||||
{ | ||||||
// Provides a curtisy warning message if the ServiceTreeId option is set to a empty guid | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
ValidateServiceTreeIdOption(); | ||||||
|
||||||
var exeName = Process.GetCurrentProcess().ProcessName; | ||||||
var version = Assembly.GetEntryAssembly() | ||||||
?.GetCustomAttribute<AssemblyInformationalVersionAttribute>() | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,94 @@ | ||||||
| ||||||
using Microsoft.DncEng.CommandLineLib; | ||||||
using Mono.Options; | ||||||
using System; | ||||||
|
||||||
namespace Microsoft.DncEng.SecretManager.Commands | ||||||
{ | ||||||
/// <summary> | ||||||
/// This class is used to extend the CommandLineLib.GlobalCommand class to add a global options spacific to this project | ||||||
/// </summary> | ||||||
public class ProjectBaseCommand : GlobalCommand | ||||||
{ | ||||||
|
||||||
/// <summary> | ||||||
/// Indictes if the global option for 'quiet' is set | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
/// </summary> | ||||||
public bool Quiet { get { return Verbosity == VerbosityLevel.Quiet; } } | ||||||
|
||||||
/// <summary> | ||||||
/// Check for local environment values to indicate you are running for Azure DevOps | ||||||
/// SYSTEM_COLLECTIONURI is a default environment variable in Azure DevOps | ||||||
/// </summary> | ||||||
private bool RunningInAzureDevOps = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("SYSTEM_COLLECTIONURI")); | ||||||
|
||||||
/// <summary> | ||||||
/// Provides the ServiceTreeId set with global options | ||||||
/// The ID is a goid and is set to Guid.Empty if not set | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
/// </summary> | ||||||
public Guid ServiceTreeId = Guid.Empty; | ||||||
|
||||||
/// <summary> | ||||||
/// Base constructor for the ProjectBaseCommand class | ||||||
/// </summary> | ||||||
public ProjectBaseCommand(GlobalCommand globalCommand) | ||||||
{ | ||||||
Verbosity = globalCommand.Verbosity; | ||||||
Help = globalCommand.Help; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Overides the GetOptions method from the base class to add a cusotm option for the ServiceTreeId | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
/// </summary> | ||||||
public override OptionSet GetOptions() | ||||||
{ | ||||||
return new OptionSet() | ||||||
{ | ||||||
{"servicetreeid=", "The service tree ID (must be a valid GUID id from aka.ms/servicetree)", id => | ||||||
{ | ||||||
if (Guid.TryParse(id, out var guid)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since someone took the time to specify a service tree ID here, I would just error out if it's not a valid GUID. |
||||||
{ | ||||||
ServiceTreeId = guid; | ||||||
} | ||||||
// If running in Azure DevOps use VSO tagging in the console output to the warning message will be handled by the Azure DevOps build system | ||||||
else if (RunningInAzureDevOps) | ||||||
{ | ||||||
WriteWarningMessage($"##vso[task.logissue type=warning]Failed to parse a valid Guid value from ServiceTreeId value '{id}'! Security Audit logging will be suppressed!"); | ||||||
} | ||||||
// Else write a general warning messgae to console | ||||||
else | ||||||
{ | ||||||
WriteWarningMessage($"Failed to parse a valid Guid value from ServiceTreeId value '{id}'! Security Audit logging will be suppressed!"); | ||||||
} | ||||||
} | ||||||
} | ||||||
}; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Provides a non-volitie warning message if the ServiceTreeId option is set to a empty guid value and argments have been parsed | ||||||
internal void ValidateServiceTreeIdOption() | ||||||
{ | ||||||
if (!Quiet && ServiceTreeId == Guid.Empty) | ||||||
{ | ||||||
// If running in Azure DevOps use VSO tagging in the console output to the warning message will be handled by the Azure DevOps build system | ||||||
if (RunningInAzureDevOps) | ||||||
{ | ||||||
WriteWarningMessage("##vso[task.logissue type=warning]ServiceTreeId is set to an Empty Guid! Security Audit logging will be suppressed!"); | ||||||
} | ||||||
// Else write a general warning messgae to console | ||||||
else | ||||||
{ | ||||||
WriteWarningMessage("ServiceTreeId is set to an Empty Guid! Security Audit logging will be suppressed!"); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
internal void WriteWarningMessage(string message) | ||||||
{ | ||||||
Console.ForegroundColor = ConsoleColor.Yellow; | ||||||
Console.WriteLine(message); | ||||||
Console.ResetColor(); | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,6 +6,7 @@ | |||||
using System.Threading.Tasks; | ||||||
using Microsoft.DncEng.CommandLineLib; | ||||||
using Microsoft.DncEng.SecretManager.StorageTypes; | ||||||
using Microsoft.VisualStudio.Services.Common; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's bringing this in? |
||||||
using Mono.Options; | ||||||
using Newtonsoft.Json; | ||||||
using Newtonsoft.Json.Linq; | ||||||
|
@@ -14,15 +15,15 @@ | |||||
namespace Microsoft.DncEng.SecretManager.Commands; | ||||||
|
||||||
[Command("validate-all")] | ||||||
public class ValidateAllCommand : Command | ||||||
public class ValidateAllCommand : ProjectBaseCommand | ||||||
{ | ||||||
private readonly IConsole _console; | ||||||
private readonly SettingsFileValidator _settingsFileValidator; | ||||||
private readonly StorageLocationTypeRegistry _storageLocationTypeRegistry; | ||||||
private readonly List<string> _manifestFiles = new List<string>(); | ||||||
private string _basePath; | ||||||
|
||||||
public ValidateAllCommand(IConsole console, SettingsFileValidator settingsFileValidator, StorageLocationTypeRegistry storageLocationTypeRegistry) | ||||||
public ValidateAllCommand(GlobalCommand globalCommand, IConsole console, SettingsFileValidator settingsFileValidator, StorageLocationTypeRegistry storageLocationTypeRegistry) : base(globalCommand) | ||||||
{ | ||||||
_console = console; | ||||||
_settingsFileValidator = settingsFileValidator; | ||||||
|
@@ -31,11 +32,11 @@ public ValidateAllCommand(IConsole console, SettingsFileValidator settingsFileVa | |||||
|
||||||
public override OptionSet GetOptions() | ||||||
{ | ||||||
return new OptionSet | ||||||
return base.GetOptions().AddRange(new OptionSet() | ||||||
{ | ||||||
{"m|manifest-file=", "A secret manifest file. Can be specified more than once.", m => _manifestFiles.Add(m)}, | ||||||
{"b|base-path=", "The base path to search for settings files.", b => _basePath = b}, | ||||||
}; | ||||||
}); | ||||||
} | ||||||
|
||||||
public override bool AreRequiredOptionsSet() | ||||||
|
@@ -45,6 +46,9 @@ public override bool AreRequiredOptionsSet() | |||||
|
||||||
public override async Task RunAsync(CancellationToken cancellationToken) | ||||||
{ | ||||||
// Provides a curtisy warning message if the ServiceTreeId option is set to a empty guid | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
ValidateServiceTreeIdOption(); | ||||||
|
||||||
bool haveErrors = false; | ||||||
var manifestFiles = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||||||
foreach (string manifestFile in _manifestFiles) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,33 @@ | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.DncEng.CommandLineLib; | ||
using Microsoft.VisualStudio.Services.Common; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Necessary? |
||
using Mono.Options; | ||
using Command = Microsoft.DncEng.CommandLineLib.Command; | ||
|
||
namespace Microsoft.DncEng.SecretManager.Commands; | ||
|
||
[Command("validate")] | ||
public class ValidateCommand : Command | ||
public class ValidateCommand : ProjectBaseCommand | ||
{ | ||
private readonly SettingsFileValidator _settingsFileValidator; | ||
private string _manifestFile; | ||
private string _baseSettingsFile; | ||
private string _envSettingsFile; | ||
|
||
public ValidateCommand(SettingsFileValidator settingsFileValidator) | ||
public ValidateCommand(GlobalCommand globalCommand, SettingsFileValidator settingsFileValidator) : base(globalCommand) | ||
{ | ||
_settingsFileValidator = settingsFileValidator; | ||
} | ||
|
||
public override OptionSet GetOptions() | ||
{ | ||
return new OptionSet | ||
return base.GetOptions().AddRange(new OptionSet() | ||
{ | ||
{"m|manifest-file=", "The secret manifest file", f => _manifestFile = f}, | ||
{"e|env-settings-file=", "The environment settings file to validate", f => _envSettingsFile = f}, | ||
{"b|base-settings-file=", "The base settings file to validate", f => _baseSettingsFile = f}, | ||
}; | ||
}); | ||
} | ||
|
||
public override bool AreRequiredOptionsSet() | ||
|
@@ -38,6 +39,8 @@ public override bool AreRequiredOptionsSet() | |
|
||
public override async Task RunAsync(CancellationToken cancellationToken) | ||
{ | ||
// Provides a curtisy warning message if the ServiceTreeId option is set to a empty guid | ||
ValidateServiceTreeIdOption(); | ||
bool foundError = !await _settingsFileValidator.ValidateFileAsync(_envSettingsFile, _baseSettingsFile, _manifestFile, cancellationToken); | ||
|
||
if (foundError) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,5 +5,15 @@ namespace Microsoft.DncEng.SecretManager; | |||||
|
||||||
public interface ITokenCredentialProvider | ||||||
{ | ||||||
/// <summary> | ||||||
/// The applicatoin ID for the credential provider. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
/// </summary> | ||||||
public string ApplicationId { get; } | ||||||
|
||||||
/// <summary> | ||||||
/// The tenant ID that provided the token from the credential provider. | ||||||
/// </summary> | ||||||
public string TenantId { get; } | ||||||
|
||||||
public Task<TokenCredential> GetCredentialAsync(); | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recognize that these updates are safe because they all implement netstandard2.0, but I'm wondering what motivates the bump in this PR?