Skip to content

Commit b57ff78

Browse files
nguerreranguerrera
authored
Update Microsoft.Security.Utilities.Core from v1.17.0 to v1.18.0 (#5224)
Release Notes: https://github.com/microsoft/security-utilities/blob/release/v1.18.0/docs/ReleaseHistory.md This release includes many new granular secret masking rules and significant performance enhancements. Benchmarks show a net speedup. It also improves the case where a literal secret value is added to the masker that also matches a rule. In this case, redaction will use `***` instead of the rule-based `SECNNN/NNN:ID`. A new agent test is also added in this change to verify this behavior. The update required some refactoring to absorb the removal of `SecretMasker.Clone`. The agent secret masker no longer implements the server `ISecretMasker` interface that has a `Clone` method. There was only one place where the agent masker was passed to server `ISecretMasker` interface, but this was deemed to be unnecessary as we also pass an `ITrace` that handles secret masking. We therefore now pass `null` in this case with a detailed comment explaining why this is correct. A new test is added to verify that masking is still performed in this code path. There is also some minor refactoring in tests to provide a shared `TestHostContext.GetTraceContent` method. A handful of tests with duplicated code are refactored to use this and a new test takes advantage of it. Finally, `TestHostContext` is updated to use the non-legacy `OssSecretMasker`.
1 parent 07778e4 commit b57ff78

17 files changed

+419
-218
lines changed

‎src/Agent.Sdk/Agent.Sdk.csproj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
</PropertyGroup>
88

99
<ItemGroup>
10-
<PackageReference Include="Microsoft.Security.Utilities.Core" Version="1.17.0" />
10+
<PackageReference Include="Microsoft.Security.Utilities.Core" Version="1.18.0" />
1111
<PackageReference Include="Microsoft.Win32.Registry" Version="5.0.0" />
1212
<PackageReference Include="System.IO.FileSystem.AccessControl" Version="6.0.0-preview.5.21301.5" />
1313
<PackageReference Include="System.Management" Version="4.7.0" />

‎src/Agent.Sdk/SecretMasking/ILoggedSecretMasker.cs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,21 @@
33

44
using System;
55

6-
using Microsoft.TeamFoundation.DistributedTask.Logging;
7-
86
namespace Agent.Sdk.SecretMasking
97
{
108
/// <summary>
119
/// Extended ISecretMasker interface that adds support for logging the origin of
1210
/// regexes, encoders and literal secret values.
1311
/// </summary>
14-
public interface ILoggedSecretMasker : ISecretMasker, IDisposable
12+
public interface ILoggedSecretMasker : IDisposable
1513
{
16-
static int MinSecretLengthLimit { get; }
14+
int MinSecretLength { get; set; }
1715

18-
void AddRegex(String pattern, string origin);
19-
void AddValue(String value, string origin);
20-
void AddValueEncoder(ValueEncoder encoder, string origin);
16+
void AddRegex(string pattern, string origin);
17+
void AddValue(string value, string origin);
18+
void AddValueEncoder(Func<string, string> encoder, string origin);
19+
string MaskSecrets(string input);
20+
void RemoveShortSecretsFromDictionary();
2121
void SetTrace(ITraceWriter trace);
2222
}
2323
}

‎src/Agent.Sdk/SecretMasking/IRawSecretMasker.cs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
using System;
4+
5+
namespace Agent.Sdk.SecretMasking
6+
{
7+
/// <summary>
8+
/// Rerpresents a raw secret masker without the features that <see
9+
/// cref="ILoggedSecretMasker"/> adds.
10+
/// </summary>
11+
public interface IRawSecretMasker : IDisposable
12+
{
13+
int MinSecretLength { get; set; }
14+
15+
void AddRegex(string pattern);
16+
void AddValue(string value);
17+
void AddValueEncoder(Func<string, string> encoder);
18+
string MaskSecrets(string input);
19+
void RemoveShortSecretsFromDictionary();
20+
}
21+
}

‎src/Agent.Sdk/SecretMasking/LegacySecretMasker.cs

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
using System;
4+
using Microsoft.TeamFoundation.DistributedTask.Logging;
5+
6+
namespace Agent.Sdk.SecretMasking
7+
{
8+
/// <summary>
9+
/// Legacy secret masker that dispatches to <see cref="SecretMasker"/> from
10+
/// 'Microsoft.TeamFoundation.DistributedTask.Logging'.
11+
/// </summary>
12+
public sealed class LegacySecretMasker : IRawSecretMasker
13+
{
14+
private ISecretMasker _secretMasker;
15+
16+
public LegacySecretMasker()
17+
{
18+
_secretMasker = new SecretMasker();
19+
}
20+
21+
private LegacySecretMasker(ISecretMasker secretMasker)
22+
{
23+
_secretMasker = secretMasker;
24+
}
25+
26+
public int MinSecretLength
27+
{
28+
get => _secretMasker.MinSecretLength;
29+
set => _secretMasker.MinSecretLength = value;
30+
}
31+
32+
public void AddRegex(string pattern)
33+
{
34+
_secretMasker.AddRegex(pattern);
35+
}
36+
37+
public void AddValue(string value)
38+
{
39+
_secretMasker.AddValue(value);
40+
}
41+
42+
public void AddValueEncoder(Func<string, string> encoder)
43+
{
44+
_secretMasker.AddValueEncoder(x => encoder(x));
45+
}
46+
47+
public void Dispose()
48+
{
49+
(_secretMasker as IDisposable)?.Dispose();
50+
_secretMasker = null;
51+
}
52+
53+
public string MaskSecrets(string input)
54+
{
55+
return _secretMasker.MaskSecrets(input);
56+
}
57+
58+
public void RemoveShortSecretsFromDictionary()
59+
{
60+
_secretMasker.RemoveShortSecretsFromDictionary();
61+
}
62+
63+
public LegacySecretMasker Clone()
64+
{
65+
return new LegacySecretMasker(_secretMasker.Clone());
66+
}
67+
}
68+
}

‎src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs

Lines changed: 57 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,49 +1,52 @@
11

22
// Copyright (c) Microsoft Corporation.
33
// Licensed under the MIT License.
4-
using System;
54

5+
using System;
66
using Microsoft.TeamFoundation.DistributedTask.Logging;
77

88
namespace Agent.Sdk.SecretMasking
99
{
1010
/// <summary>
1111
/// Extended secret masker service that allows specifying the origin of any
12-
/// masking operation. It works by wrapping an existing ISecretMasker
12+
/// masking operation. It works by wrapping an existing IRawSecretMasker
1313
/// implementation and an optionally settable ITraceWriter instance for
1414
/// secret origin logging operations. In the agent today, this class can be
15-
/// initialized with two distinct ISecretMasker implementations, the one
15+
/// initialized with two distinct IRawSecretMasker implementations, the one
1616
/// that ships in VSO itself, and the official Microsoft open source secret
1717
/// masker, implemented at https://github/microsoft/security-utilities.
1818
/// </summary>
1919
public class LoggedSecretMasker : ILoggedSecretMasker
2020
{
21-
private ISecretMasker _secretMasker;
21+
private IRawSecretMasker _secretMasker;
2222
private ITraceWriter _trace;
2323

24-
2524
private void Trace(string msg)
2625
{
2726
this._trace?.Info(msg);
2827
}
2928

30-
public LoggedSecretMasker(ISecretMasker secretMasker)
29+
private LoggedSecretMasker(IRawSecretMasker secretMasker)
3130
{
32-
this._secretMasker = secretMasker;
31+
_secretMasker = secretMasker;
3332
}
3433

35-
public void SetTrace(ITraceWriter trace)
34+
public static LoggedSecretMasker Create(IRawSecretMasker secretMasker)
3635
{
37-
this._trace = trace;
36+
return secretMasker switch
37+
{
38+
LegacySecretMasker lsm => new LegacyLoggedSecretMasker(lsm),
39+
_ => new LoggedSecretMasker(secretMasker),
40+
};
3841
}
3942

40-
public void AddValue(string pattern)
43+
public void SetTrace(ITraceWriter trace)
4144
{
42-
this._secretMasker.AddValue(pattern);
45+
this._trace = trace;
4346
}
4447

4548
/// <summary>
46-
/// Overloading of AddValue method with additional logic for logging origin of provided secret
49+
/// AddValue method with additional logic for logging origin of provided secret
4750
/// </summary>
4851
/// <param name="value">Secret to be added</param>
4952
/// <param name="origin">Origin of the secret</param>
@@ -57,18 +60,14 @@ public void AddValue(string value, string origin)
5760
return;
5861
}
5962

60-
AddValue(value);
61-
}
62-
public void AddRegex(string pattern)
63-
{
64-
this._secretMasker.AddRegex(pattern);
63+
_secretMasker.AddValue(value);
6564
}
6665

6766
/// <summary>
68-
/// Overloading of AddRegex method with additional logic for logging origin of provided secret
67+
/// AddRegex method with additional logic for logging origin of provided secret
6968
/// </summary>
70-
/// <param name="pattern"></param>
71-
/// <param name="origin"></param>
69+
/// <param name="pattern">Regex to be added</param>
70+
/// <param name="origin">Origin of the regex</param>
7271
public void AddRegex(string pattern, string origin)
7372
{
7473
// WARNING: Do not log the pattern here, it could be very specifc and contain a secret!
@@ -79,7 +78,7 @@ public void AddRegex(string pattern, string origin)
7978
return;
8079
}
8180

82-
AddRegex(pattern);
81+
_secretMasker.AddRegex(pattern);
8382
}
8483

8584
// We don't allow to skip secrets longer than 5 characters.
@@ -111,18 +110,17 @@ public void RemoveShortSecretsFromDictionary()
111110
_secretMasker.RemoveShortSecretsFromDictionary();
112111
}
113112

114-
public void AddValueEncoder(ValueEncoder encoder)
113+
public void AddValueEncoder(Func<string, string> encoder)
115114
{
116115
this._secretMasker.AddValueEncoder(encoder);
117116
}
118117

119-
120118
/// <summary>
121119
/// Overloading of AddValueEncoder method with additional logic for logging origin of provided secret
122120
/// </summary>
123121
/// <param name="encoder"></param>
124122
/// <param name="origin"></param>
125-
public void AddValueEncoder(ValueEncoder encoder, string origin)
123+
public void AddValueEncoder(Func<string, string> encoder, string origin)
126124
{
127125
this.Trace($"Setting up value for origin: {origin}");
128126
if (encoder == null)
@@ -134,18 +132,11 @@ public void AddValueEncoder(ValueEncoder encoder, string origin)
134132
AddValueEncoder(encoder);
135133
}
136134

137-
public LoggedSecretMasker Clone()
138-
{
139-
return new LoggedSecretMasker(this._secretMasker.Clone());
140-
}
141-
142135
public string MaskSecrets(string input)
143136
{
144137
return this._secretMasker.MaskSecrets(input);
145138
}
146139

147-
ISecretMasker ISecretMasker.Clone() => this.Clone();
148-
149140
public void Dispose()
150141
{
151142
Dispose(true);
@@ -156,12 +147,43 @@ protected virtual void Dispose(bool disposing)
156147
{
157148
if (disposing)
158149
{
159-
if (_secretMasker is IDisposable disposable)
160-
{
161-
disposable.Dispose();
162-
}
150+
_secretMasker?.Dispose();
163151
_secretMasker = null;
164152
}
165153
}
154+
155+
// When backed by legacy secret masker, we can still implement Clone and
156+
// the server ISecretMasker interface. This is done to minimize churn
157+
// when running without the feature flag that enables the new secret
158+
// masker.
159+
private sealed class LegacyLoggedSecretMasker : LoggedSecretMasker, ISecretMasker
160+
{
161+
public LegacyLoggedSecretMasker(LegacySecretMasker secretMasker) : base(secretMasker) { }
162+
163+
void ISecretMasker.AddRegex(string pattern)
164+
{
165+
_secretMasker.AddRegex(pattern);
166+
}
167+
168+
void ISecretMasker.AddValue(string value)
169+
{
170+
_secretMasker.AddValue(value);
171+
}
172+
173+
void ISecretMasker.AddValueEncoder(ValueEncoder encoder)
174+
{
175+
_secretMasker.AddValueEncoder(x => encoder(x));
176+
}
177+
178+
ISecretMasker ISecretMasker.Clone()
179+
{
180+
// NOTE: It has always been the case that trace does not flow to
181+
// clones and this code path exists to preserve legacy behavior
182+
// in the absence of a feature flag, so that behavior is
183+
// retained here.
184+
var lsm = (LegacySecretMasker)_secretMasker;
185+
return new LegacyLoggedSecretMasker(lsm.Clone());
186+
}
187+
}
166188
}
167189
}

‎src/Agent.Sdk/SecretMasking/OssSecretMasker.cs

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,9 @@
55
using System.Text.RegularExpressions;
66
using Microsoft.Security.Utilities;
77

8-
using ISecretMasker = Microsoft.TeamFoundation.DistributedTask.Logging.ISecretMasker;
9-
using ValueEncoder = Microsoft.TeamFoundation.DistributedTask.Logging.ValueEncoder;
10-
118
namespace Agent.Sdk.SecretMasking;
129

13-
public sealed class OssSecretMasker : ISecretMasker, IDisposable
10+
public sealed class OssSecretMasker : IRawSecretMasker
1411
{
1512
private SecretMasker _secretMasker;
1613

@@ -24,10 +21,6 @@ public OssSecretMasker(IEnumerable<RegexPattern> patterns)
2421
_secretMasker.DefaultRegexRedactionToken = "***";
2522
}
2623

27-
private OssSecretMasker(OssSecretMasker copy)
28-
{
29-
_secretMasker = copy._secretMasker.Clone();
30-
}
3124

3225
/// <summary>
3326
/// This property allows to set the minimum length of a secret for masking
@@ -70,13 +63,11 @@ public void AddValue(string test)
7063
/// <summary>
7164
/// This implementation assumes no more than one thread is adding regexes, values, or encoders at any given time.
7265
/// </summary>
73-
public void AddValueEncoder(ValueEncoder encoder)
66+
public void AddValueEncoder(Func<string, string> encoder)
7467
{
7568
_secretMasker.AddLiteralEncoder(x => encoder(x));
7669
}
7770

78-
public OssSecretMasker Clone() => new OssSecretMasker(this);
79-
8071
public void Dispose()
8172
{
8273
_secretMasker?.Dispose();
@@ -152,6 +143,4 @@ public void RemoveShortSecretsFromDictionary()
152143
}
153144
}
154145
}
155-
156-
ISecretMasker ISecretMasker.Clone() => this.Clone();
157146
}

‎src/Agent.Worker/ContainerOperationProvider.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -298,7 +298,7 @@ private async Task<string> GetAcrPasswordFromAADToken(IExecutionContext executio
298298
}
299299

300300
// Mark retrieved password as secret
301-
HostContext.SecretMasker.AddValue(AcrPassword);
301+
HostContext.SecretMasker.AddValue(AcrPassword, origin: "AcrPassword");
302302

303303
return AcrPassword;
304304
}

‎src/Agent.Worker/ExpressionManager.cs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using Microsoft.TeamFoundation.DistributedTask.WebApi;
88
using Microsoft.VisualStudio.Services.Agent.Util;
99
using Microsoft.TeamFoundation.DistributedTask.Expressions;
10+
using Microsoft.TeamFoundation.DistributedTask.Logging;
1011
using System.Text;
1112

1213
namespace Microsoft.VisualStudio.Services.Agent.Worker
@@ -52,7 +53,13 @@ public ConditionResult Evaluate(IExecutionContext executionContext, IExpressionN
5253
ConditionResult result = new ConditionResult();
5354
var expressionTrace = new TraceWriter(Trace, hostTracingOnly ? null : executionContext);
5455

55-
result.Value = tree.Evaluate<bool>(trace: expressionTrace, secretMasker: HostContext.SecretMasker, state: executionContext);
56+
// NOTE: When the non-legacy secret masker is enabled via feature
57+
// flag, this conversion will fail and we will pass null here. This
58+
// is deliberate and OK because the trace that we pass will handle
59+
// secret masking as will upstream exception handlers.
60+
var secretMasker = HostContext.SecretMasker as ISecretMasker;
61+
62+
result.Value = tree.Evaluate<bool>(trace: expressionTrace, secretMasker, state: executionContext);
5663
result.Trace = expressionTrace.Trace;
5764

5865
return result;

0 commit comments

Comments
 (0)