Skip to content

Commit 5be598a

Browse files
joelverhagenjoelverhagen
authored
Do not audit values for removed/revoked API keys (#10272)
1 parent 315ab74 commit 5be598a

6 files changed

Lines changed: 32 additions & 53 deletions

File tree

‎src/NuGetGallery.Core/Auditing/CredentialAuditRecord.cs‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ public class CredentialAuditRecord
2121
public string TenantId { get; }
2222
public string RevocationSource { get; }
2323

24-
public CredentialAuditRecord(Credential credential, bool removedOrRevoked)
24+
public CredentialAuditRecord(Credential credential)
2525
{
2626
if (credential == null)
2727
{
@@ -34,23 +34,12 @@ public CredentialAuditRecord(Credential credential, bool removedOrRevoked)
3434
Identity = credential.Identity;
3535
TenantId = credential.TenantId;
3636

37-
// Track the value for credentials that are external (object id) or definitely revocable (API Key, etc.) and have been removed
37+
// Track the value for credentials that are external (object id)
38+
// Do not track the credential valid for API keys or passwords, even if they are revoked.
3839
if (credential.IsExternal())
3940
{
4041
Value = credential.Value;
4142
}
42-
else if (removedOrRevoked)
43-
{
44-
if (Type == null)
45-
{
46-
throw new ArgumentNullException(nameof(credential.Type));
47-
}
48-
49-
if (!credential.IsPassword())
50-
{
51-
Value = credential.Value;
52-
}
53-
}
5443

5544
Created = credential.Created;
5645
Expires = credential.Expires;
@@ -65,10 +54,10 @@ public CredentialAuditRecord(Credential credential, bool removedOrRevoked)
6554
}
6655
}
6756

68-
public CredentialAuditRecord(Credential credential, bool removedOrRevoked, string revocationSource)
69-
: this(credential, removedOrRevoked)
57+
public CredentialAuditRecord(Credential credential, string revocationSource)
58+
: this(credential)
7059
{
7160
RevocationSource = revocationSource;
7261
}
7362
}
74-
}
63+
}

‎src/NuGetGallery.Core/Auditing/FailedAuthenticatedOperationAuditRecord.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public FailedAuthenticatedOperationAuditRecord(
3131

3232
if (attemptedCredential != null)
3333
{
34-
AttemptedCredential = new CredentialAuditRecord(attemptedCredential, removedOrRevoked: false);
34+
AttemptedCredential = new CredentialAuditRecord(attemptedCredential);
3535
}
3636
}
3737

@@ -40,4 +40,4 @@ public override string GetPath()
4040
return Path; // store in <auditpath>/failedauthenticatedoperation/all
4141
}
4242
}
43-
}
43+
}

‎src/NuGetGallery.Core/Auditing/UserAuditRecord.cs‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
@@ -56,7 +56,7 @@ public UserAuditRecord(User user, AuditedUserAction action)
5656
Roles = user.Roles.Select(r => r.Name).ToArray();
5757

5858
Credentials = user.Credentials.Where(CredentialTypes.IsSupportedCredential)
59-
.Select(c => new CredentialAuditRecord(c, removedOrRevoked: false)).ToArray();
59+
.Select(c => new CredentialAuditRecord(c)).ToArray();
6060

6161
AffectedCredential = Array.Empty<CredentialAuditRecord>();
6262
AffectedPolicies = Array.Empty<AuditedUserSecurityPolicy>();
@@ -70,8 +70,7 @@ public UserAuditRecord(User user, AuditedUserAction action, Credential affected,
7070
public UserAuditRecord(User user, AuditedUserAction action, IEnumerable<Credential> affected, string revocationSource)
7171
: this(user, action)
7272
{
73-
AffectedCredential = affected.Select(c => new CredentialAuditRecord(c,
74-
removedOrRevoked: action == AuditedUserAction.RemoveCredential || action == AuditedUserAction.RevokeCredential, revocationSource: revocationSource)).ToArray();
73+
AffectedCredential = affected.Select(c => new CredentialAuditRecord(c, revocationSource: revocationSource)).ToArray();
7574
}
7675

7776
public UserAuditRecord(User user, AuditedUserAction action, Credential affected)
@@ -82,8 +81,7 @@ public UserAuditRecord(User user, AuditedUserAction action, Credential affected)
8281
public UserAuditRecord(User user, AuditedUserAction action, IEnumerable<Credential> affected)
8382
: this(user, action)
8483
{
85-
AffectedCredential = affected.Select(c => new CredentialAuditRecord(c,
86-
removedOrRevoked: action == AuditedUserAction.RemoveCredential || action == AuditedUserAction.RevokeCredential)).ToArray();
84+
AffectedCredential = affected.Select(c => new CredentialAuditRecord(c)).ToArray();
8785
}
8886

8987
public UserAuditRecord(User user, AuditedUserAction action, string affectedEmailAddress)

‎tests/NuGetGallery.Core.Facts/Auditing/CredentialAuditRecordTests.cs‎

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
@@ -13,31 +13,23 @@ public class CredentialAuditRecordTests
1313
[Fact]
1414
public void Constructor_ThrowsForNullCredential()
1515
{
16-
Assert.Throws<ArgumentNullException>(() => new CredentialAuditRecord(credential: null, removedOrRevoked: true));
16+
Assert.Throws<ArgumentNullException>(() => new CredentialAuditRecord(credential: null));
1717
}
1818

1919
[Fact]
20-
public void Constructor_ThrowsForRemovalWithNullType()
21-
{
22-
var credential = new Credential();
23-
24-
Assert.Throws<ArgumentNullException>(() => new CredentialAuditRecord(credential, removedOrRevoked: true));
25-
}
26-
27-
[Fact]
28-
public void Constructor_RemovalOfNonPasswordSetsValue()
20+
public void Constructor_RemovalOfNonPasswordDoesNotSetValue()
2921
{
3022
var credential = new Credential(type: "a", value: "b");
31-
var record = new CredentialAuditRecord(credential, removedOrRevoked: true);
23+
var record = new CredentialAuditRecord(credential);
3224

33-
Assert.Equal("b", record.Value);
25+
Assert.Null(record.Value);
3426
}
3527

3628
[Fact]
3729
public void Constructor_RemovalOfPasswordDoesNotSetValue()
3830
{
3931
var credential = new Credential(type: CredentialTypes.Password.V3, value: "a");
40-
var record = new CredentialAuditRecord(credential, removedOrRevoked: true);
32+
var record = new CredentialAuditRecord(credential);
4133

4234
Assert.Null(record.Value);
4335
}
@@ -46,7 +38,7 @@ public void Constructor_RemovalOfPasswordDoesNotSetValue()
4638
public void Constructor_NonRemovalOfNonPasswordDoesNotSetsValue()
4739
{
4840
var credential = new Credential(type: "a", value: "b");
49-
var record = new CredentialAuditRecord(credential, removedOrRevoked: false);
41+
var record = new CredentialAuditRecord(credential);
5042

5143
Assert.Null(record.Value);
5244
}
@@ -57,7 +49,7 @@ public void Constructor_NonRemovalOfNonPasswordDoesNotSetsValue()
5749
public void Constructor_ExternalCredentialSetsValue(string externalType)
5850
{
5951
var credential = new Credential(type: externalType, value: "b");
60-
var record = new CredentialAuditRecord(credential, removedOrRevoked: false);
52+
var record = new CredentialAuditRecord(credential);
6153

6254
Assert.Equal("b", record.Value);
6355
}
@@ -66,7 +58,7 @@ public void Constructor_ExternalCredentialSetsValue(string externalType)
6658
public void Constructor_NonRemovalOfPasswordDoesNotSetValue()
6759
{
6860
var credential = new Credential(type: CredentialTypes.Password.V3, value: "a");
69-
var record = new CredentialAuditRecord(credential, removedOrRevoked: false);
61+
var record = new CredentialAuditRecord(credential);
7062

7163
Assert.Null(record.Value);
7264
}
@@ -90,7 +82,7 @@ public void Constructor_SetsProperties()
9082
Type = "e",
9183
Value = "f"
9284
};
93-
var record = new CredentialAuditRecord(credential, removedOrRevoked: true);
85+
var record = new CredentialAuditRecord(credential);
9486

9587
Assert.Equal(created, record.Created);
9688
Assert.Equal("a", record.Description);
@@ -104,19 +96,19 @@ public void Constructor_SetsProperties()
10496
Assert.Equal("c", scope.Subject);
10597
Assert.Equal("d", scope.AllowedAction);
10698
Assert.Equal("e", record.Type);
107-
Assert.Equal("f", record.Value);
99+
Assert.Null(record.Value);
108100
}
109101

110102
[Fact]
111103
public void Constructor_WithRevocationSource_Properties()
112104
{
113105
var testRevocationSource = "TestRevocationSource";
114106
var credential = new Credential(type: "a", value: "b");
115-
var record = new CredentialAuditRecord(credential, removedOrRevoked: true, revocationSource: testRevocationSource);
107+
var record = new CredentialAuditRecord(credential, revocationSource: testRevocationSource);
116108

117109
Assert.Equal(testRevocationSource, record.RevocationSource);
118110
Assert.Equal("a", record.Type);
119-
Assert.Equal("b", record.Value);
111+
Assert.Null(record.Value);
120112
}
121113
}
122-
}
114+
}

‎tests/NuGetGallery.Core.Facts/Auditing/UserAuditRecordTests.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
@@ -108,7 +108,7 @@ public void Constructor_WithRevocationSource_SetsProperties()
108108
Assert.Single(record.AffectedCredential);
109109
Assert.Equal(testRevocationSource, record.AffectedCredential[0].RevocationSource);
110110
Assert.Equal("b", record.AffectedCredential[0].Type);
111-
Assert.Equal("c", record.AffectedCredential[0].Value);
111+
Assert.Null(record.AffectedCredential[0].Value);
112112
}
113113

114114
[Fact]
@@ -127,4 +127,4 @@ public void GetPath_ReturnsLowerCasedUserName()
127127
Assert.Equal("a", actualPath);
128128
}
129129
}
130-
}
130+
}

‎tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
@@ -1605,7 +1605,7 @@ public async Task WritesAuditRecordRemovingTheOldCredential()
16051605
ar.AffectedCredential.Length == 1 &&
16061606
ar.AffectedCredential[0].Type == existingCred.Type &&
16071607
ar.AffectedCredential[0].Identity == existingCred.Identity &&
1608-
ar.AffectedCredential[0].Value == existingCred.Value &&
1608+
ar.AffectedCredential[0].Value is null &&
16091609
ar.AffectedCredential[0].Created == existingCred.Created &&
16101610
ar.AffectedCredential[0].Expires == existingCred.Expires));
16111611
}
@@ -2633,4 +2633,4 @@ public static bool VerifyPasswordHash(string hash, string algorithm, string pass
26332633
return canAuthenticate && !confidenceCheck;
26342634
}
26352635
}
2636-
}
2636+
}

0 commit comments

Comments
 (0)