-
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?
Conversation
…e the secrete manager command line operations to allow for passing in a service tree id associated with the service execuring change operations.
Changing variable names to be more consistent with use Altering some comments
…l and base command settings when not using the command line.
{ | ||
_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 comment
The reason will be displayed to describe this comment to others. Learn more.
// Provides a curtisy warning message if the ServiceTreeId option is set to a empty guid | |
// Provides a courtesy warning message if the ServiceTreeId option is set to a empty guid |
{ | ||
{"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 comment
The 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.
var auditFactory = AuditLoggerFactory.Create(options => | ||
{ | ||
// We use ETW as the destination for the audit logs becsue the application is not gurenteed to run on windows | ||
options.Destination = AuditLogDestination.ETW; |
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.
ETW is available on Linux?
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.
+1. We also don't typically capture ETW data. Where does audit information ultimately end up?
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary?
var createdSecret = await client.SetSecretAsync(name, value.Value ?? ""); | ||
var properties = createdSecret.Value.Properties; | ||
foreach (var (k, v) in value.Tags) | ||
// The default audit state should alwasy be failure and overwritten with success at the end of the operation. |
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.
// The default audit state should alwasy be failure and overwritten with success at the end of the operation. | |
// The default audit state should always be failure and overwritten with success at the end of the operation. |
// The hope is that app insights will also catch the base exception for debugging. | ||
catch (Exception ex) | ||
{ | ||
Console.WriteLine($"Failed to add audit log for secret update!: <{ex.Message}>"); |
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.
Will the audit message always be safe for public view in a console?
/// </summary> | ||
public class SecurityAuditLogger | ||
{ | ||
private ILogger ControlPanelLogger; |
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.
private ILogger ControlPanelLogger; | |
private ILogger ControlPlaneLogger; |
I think that's what you meant?
{ | ||
LogSecretAction(OperationType.Update, operationName, credentialProvider, secretName, secretStoreType, secretLocation, result, resultMessage); | ||
} | ||
// Audit logging is a 'volitile' operation meaning it can throw exceptions if logging fails. |
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.
// Audit logging is a 'volitile' operation meaning it can throw exceptions if logging fails. | |
// Audit logging is a 'volatile' operation meaning it can throw exceptions if logging fails. |
return; | ||
} | ||
|
||
// The token applicatoin id of the client running the assembly. |
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.
// The token applicatoin id of the client running the assembly. | |
// The token application id of the client running the assembly. |
// NOTE: The user identity here should be something 'dynamic'. | ||
// If you are hard coding this value you should question if this Audit Log is useful | ||
// as it is likly redundant to lower level permission change logging that is already occuring. |
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.
Should these comments be here? They look like copy-paste from an example?
<configuration> | ||
<packageSources> | ||
<clear /> | ||
<add key="AzureGenevaMonitoring" value="https://msblox.pkgs.visualstudio.com/_packaging/AzureGenevaMonitoring/nuget/v3/index.json" /> |
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.
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 comment
The 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.
- I need to investigate if the 'public' feed version of the package has the same class and methods defined. If so, we can do something where the 'internal feed' is different than the feed used in the 'public' processes.
- I also have open questions to the Audit team to verify if the package from 'this' feed CAN be exposed publicly. If so we may be able to just manually move target versions into one of the feeds we manage directly.
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 comment
The 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.
/// </summary> | ||
public SecurityAuditLogger(Guid serviceTreeId) | ||
{ | ||
var auditFactory = AuditLoggerFactory.Create(options => |
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.
What does this logger do if the Service ID is Guid.Empty
? Does it still try to log or is that enough to make it silent?
@@ -26,6 +24,11 @@ private async Task TestCommand(DateTimeOffset now, string manifestText, string l | |||
var cancellationToken = cts.Token; | |||
|
|||
var services = new ServiceCollection(); | |||
// Dependency injection instruction needed to support properties used for Geneval Logging operations | |||
services.AddSingleton(new GlobalCommand()); | |||
services.AddSingleton(new SecurityAuditLogger(Guid.Empty)); |
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.
Since these are test classes, would it be better to make a do-nothing version of SecurityAuditLogger? (I'm not sure what SecurityAuditLogger does.)
services.AddSingleton(new GlobalCommand()); | ||
services.AddSingleton(new SecurityAuditLogger(Guid.Empty)); | ||
|
||
// Original dependency injection instructions |
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 don't thin kthis comment (or the previous comment) is necessary. The code shouldn't have a sense of "human time". That's what version control handles. The code should always just be "this is what it is right now".
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 comment
The reason will be displayed to describe this comment to others. Learn more.
What's bringing this in?
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
/// The applicatoin ID for the credential provider. | |
/// The application ID for the credential provider. |
@@ -57,15 +57,15 @@ | |||
<PackageVersion Include="Microsoft.DotNet.SwaggerGenerator.MSBuild" Version="$(MicrosoftDotNetSwaggerGeneratorMSBuildVersion)" /> | |||
<PackageVersion Include="Microsoft.DotNet.VersionTools" Version="$(MicrosoftDotNetVersionToolsVersion)" /> | |||
<PackageVersion Include="Microsoft.DotNet.Web.Authentication" Version="$(MicrosoftDotNetWebAuthenticationVersion)" /> | |||
<PackageVersion Include="Microsoft.Extensions.Configuration" Version="6.0.1" /> | |||
<PackageVersion Include="Microsoft.Extensions.Configuration.Json" Version="6.0.0" /> | |||
<PackageVersion Include="Microsoft.Extensions.Configuration" Version="8.0.0" /> |
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?
{ | ||
|
||
/// <summary> | ||
/// Indictes if the global option for 'quiet' is set |
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.
/// Indictes if the global option for 'quiet' is set | |
/// Indicates if the global option for 'quiet' is set |
|
||
/// <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 comment
The reason will be displayed to describe this comment to others. Learn more.
/// The ID is a goid and is set to Guid.Empty if not set | |
/// The ID is a guid and is set to Guid.Empty if not set |
} | ||
|
||
/// <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 comment
The reason will be displayed to describe this comment to others. Learn more.
/// Overides the GetOptions method from the base class to add a cusotm option for the ServiceTreeId | |
/// Overrides the GetOptions method from the base class to add a custom option for the ServiceTreeId |
@@ -45,6 +47,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 comment
The reason will be displayed to describe this comment to others. Learn more.
// Provides a curtisy warning message if the ServiceTreeId option is set to a empty guid | |
// Provides a courtesy warning message if the ServiceTreeId option is set to an empty guid |
var ipAddress = host.AddressList.FirstOrDefault(ip => ip.AddressFamily == AddressFamily.InterNetwork); | ||
if (ipAddress == null) | ||
{ | ||
throw new Exception("No network adapters with an IPv4 address in the system!"); |
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.
No IPv6?
_auditLogger.LogSecretUpdate( | ||
credentialProvider: _tokenCredentialProvider, | ||
secretName: name, | ||
secretStoreType: "AzureKeyVault", |
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.
secretStoreType: "AzureKeyVault", | |
secretStoreType: nameof(AzureKeyVault), |
Compiler fanciness.
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.
Marking to hold at least until the questions about how CI will handle the new packaging are answered.
Add Audit Logging To SecreteManager.exe
Part Of GitHub Issue https://github.com/dotnet/arcade/issues/13217