-
Notifications
You must be signed in to change notification settings - Fork 644
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
Temp keys telemetry and auditing #3945
Changes from 10 commits
9358458
ecbe381
b2eb7b9
1898e07
b333ed8
fc0de20
f72f6d7
8f86fbc
b1bb73c
a2fc8c8
236c5f0
b8e9b12
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 |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
namespace NuGetGallery.Auditing.AuditedEntities | ||
{ | ||
/// <summary> | ||
/// Auditing details for UserSecurityPolicy entity. | ||
/// </summary> | ||
public class AuditedUserSecurityPolicy | ||
{ | ||
public string Name { get; } | ||
public string Subscription { get; } | ||
public string Value { get; } | ||
|
||
public AuditedUserSecurityPolicy(UserSecurityPolicy policy) | ||
{ | ||
Name = policy.Name; | ||
Subscription = policy.Subscription; | ||
Value = policy.Value; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,6 @@ public enum AuditedPackageAction | |
Unlist, | ||
Edit, | ||
UndoEdit, | ||
|
||
|
||
Verify | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
namespace NuGetGallery.Auditing | ||
{ | ||
public enum AuditedSecurityPolicyAction | ||
{ | ||
Create, | ||
Verify | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using NuGetGallery.Auditing.AuditedEntities; | ||
|
||
namespace NuGetGallery.Auditing | ||
{ | ||
|
@@ -17,6 +18,11 @@ public class UserAuditRecord : AuditRecord<AuditedUserAction> | |
public CredentialAuditRecord[] AffectedCredential { get; } | ||
public string AffectedEmailAddress { get; } | ||
|
||
/// <summary> | ||
/// Subset of user policies affected by the action (subscription / unsubscription). | ||
/// </summary> | ||
public AuditedUserSecurityPolicy[] AffectedPolicies { get; } | ||
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. Be sure to update shims. |
||
|
||
public UserAuditRecord(User user, AuditedUserAction action) | ||
: this(user, action, Enumerable.Empty<Credential>()) | ||
{ | ||
|
@@ -55,7 +61,18 @@ public UserAuditRecord(User user, AuditedUserAction action, string affectedEmail | |
{ | ||
AffectedEmailAddress = affectedEmailAddress; | ||
} | ||
|
||
|
||
public UserAuditRecord(User user, AuditedUserAction action, IEnumerable<UserSecurityPolicy> affectedPolicies) | ||
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. Please add tests for this constructor in UserAuditRecordTests.cs. 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. done |
||
: this(user, action, Enumerable.Empty<Credential>()) | ||
{ | ||
if (affectedPolicies == null || affectedPolicies.Count() == 0) | ||
{ | ||
throw new ArgumentException(nameof(affectedPolicies)); | ||
} | ||
|
||
AffectedPolicies = affectedPolicies.Select(p => new AuditedUserSecurityPolicy(p)).ToArray(); | ||
} | ||
|
||
public override string GetPath() | ||
{ | ||
return Username.ToLowerInvariant(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using NuGetGallery.Auditing.AuditedEntities; | ||
|
||
namespace NuGetGallery.Auditing | ||
{ | ||
/// <summary> | ||
/// Audit record for failed user security policy evaluations. | ||
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. failed and succeeded policies evaluation |
||
/// </summary> | ||
public class UserSecurityPolicyAuditRecord : AuditRecord<AuditedSecurityPolicyAction> | ||
{ | ||
public string Username { get; } | ||
|
||
public AuditedUserSecurityPolicy[] AffectedPolicies { get; } | ||
|
||
public bool Success { get; set; } | ||
|
||
public string ErrorMessage { get; } | ||
|
||
public UserSecurityPolicyAuditRecord(string username, | ||
AuditedSecurityPolicyAction action, | ||
IEnumerable<UserSecurityPolicy> affectedPolicies, | ||
bool success, string errorMessage = null) | ||
:base(action) | ||
{ | ||
if (string.IsNullOrEmpty(username)) | ||
{ | ||
throw new ArgumentNullException(nameof(username)); | ||
} | ||
if (affectedPolicies == null || affectedPolicies.Count() == 0) | ||
{ | ||
throw new ArgumentException(nameof(affectedPolicies)); | ||
} | ||
|
||
Username = username; | ||
AffectedPolicies = affectedPolicies.Select(p => new AuditedUserSecurityPolicy(p)).ToArray(); | ||
Success = success; | ||
ErrorMessage = errorMessage; | ||
} | ||
|
||
public override string GetPath() | ||
{ | ||
return Username.ToLowerInvariant(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,6 +207,10 @@ protected override void Load(ContainerBuilder builder) | |
.As<ISecurityPolicyService>() | ||
.InstancePerLifetimeScope(); | ||
|
||
builder.RegisterType<SecurePushSubscription>() | ||
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. any reason not to have it as a SingleInstance() ? Same for SecurityPolicyService. The more we use singleton, the cheaper a request will be. |
||
.AsSelf() | ||
.InstancePerLifetimeScope(); | ||
|
||
var mailSenderThunk = new Lazy<IMailSender>( | ||
() => | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
using NuGetGallery.Filters; | ||
using NuGetGallery.Infrastructure.Authentication; | ||
using NuGetGallery.Packaging; | ||
using NuGetGallery.Security; | ||
using PackageIdValidator = NuGetGallery.Packaging.PackageIdValidator; | ||
|
||
namespace NuGetGallery | ||
|
@@ -47,6 +48,7 @@ public partial class ApiController | |
public ITelemetryService TelemetryService { get; set; } | ||
public AuthenticationService AuthenticationService { get; set; } | ||
public ICredentialBuilder CredentialBuilder { get; set; } | ||
protected ISecurityPolicyService SecurityPolicyService { get; set; } | ||
|
||
protected ApiController() | ||
{ | ||
|
@@ -69,7 +71,8 @@ public ApiController( | |
IGalleryConfigurationService configurationService, | ||
ITelemetryService telemetryService, | ||
AuthenticationService authenticationService, | ||
ICredentialBuilder credentialBuilder) | ||
ICredentialBuilder credentialBuilder, | ||
ISecurityPolicyService securityPolicies) | ||
{ | ||
EntitiesContext = entitiesContext; | ||
PackageService = packageService; | ||
|
@@ -87,6 +90,7 @@ public ApiController( | |
TelemetryService = telemetryService; | ||
AuthenticationService = authenticationService; | ||
CredentialBuilder = credentialBuilder; | ||
SecurityPolicyService = securityPolicies; | ||
StatisticsService = null; | ||
} | ||
|
||
|
@@ -107,8 +111,11 @@ public ApiController( | |
IGalleryConfigurationService configurationService, | ||
ITelemetryService telemetryService, | ||
AuthenticationService authenticationService, | ||
ICredentialBuilder credentialBuilder) | ||
: this(entitiesContext, packageService, packageFileService, userService, nugetExeDownloaderService, contentService, indexingService, searchService, autoCuratePackage, statusService, messageService, auditingService, configurationService, telemetryService, authenticationService, credentialBuilder) | ||
ICredentialBuilder credentialBuilder, | ||
ISecurityPolicyService securityPolicies) | ||
: this(entitiesContext, packageService, packageFileService, userService, nugetExeDownloaderService, contentService, | ||
indexingService, searchService, autoCuratePackage, statusService, messageService, auditingService, | ||
configurationService, telemetryService, authenticationService, credentialBuilder, securityPolicies) | ||
{ | ||
StatisticsService = statisticsService; | ||
} | ||
|
@@ -224,15 +231,21 @@ public async virtual Task<ActionResult> CreatePackageVerificationKeyAsync(string | |
|
||
[HttpGet] | ||
[RequireSsl] | ||
[ApiAuthorize(SecurityPolicyAction.PackageVerify)] | ||
[ApiAuthorize] | ||
[ApiScopeRequired(NuGetScopes.PackageVerify, NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)] | ||
[ActionName("VerifyPackageKey")] | ||
public async virtual Task<ActionResult> VerifyPackageKeyAsync(string id, string version) | ||
{ | ||
var policyResult = await SecurityPolicyService.EvaluateAsync(SecurityPolicyAction.PackageVerify, HttpContext); | ||
if (!policyResult.Success) | ||
{ | ||
return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, policyResult.ErrorMessage); | ||
} | ||
|
||
var user = GetCurrentUser(); | ||
var credential = user.GetCurrentApiKeyCredential(User.Identity); | ||
|
||
var result = VerifyPackageKeyInternal(user, credential, id, version); | ||
var result = await VerifyPackageKeyInternalAsync(user, credential, id, version); | ||
|
||
// Expire and delete verification key after first use to avoid growing the database tables. | ||
if (CredentialTypes.IsPackageVerificationApiKey(credential.Type)) | ||
|
@@ -245,7 +258,7 @@ public async virtual Task<ActionResult> VerifyPackageKeyAsync(string id, string | |
return (ActionResult)result ?? new EmptyResult(); | ||
} | ||
|
||
private HttpStatusCodeWithBodyResult VerifyPackageKeyInternal(User user, Credential credential, string id, string version) | ||
private async Task<HttpStatusCodeWithBodyResult> VerifyPackageKeyInternalAsync(User user, Credential credential, string id, string version) | ||
{ | ||
// Verify that the user has permission to push for the specific Id \ version combination. | ||
var package = PackageService.FindPackageByIdAndVersion(id, version); | ||
|
@@ -254,7 +267,11 @@ private HttpStatusCodeWithBodyResult VerifyPackageKeyInternal(User user, Credent | |
return new HttpStatusCodeWithBodyResult( | ||
HttpStatusCode.NotFound, String.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, id, version)); | ||
} | ||
|
||
|
||
// Write an audit record | ||
await AuditingService.SaveAuditRecordAsync( | ||
new PackageAuditRecord(package, AuditedPackageAction.Verify, "Verified via API.")); | ||
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. is the string "Verified via API." needed? The action implies that already. Consider removing. |
||
|
||
if (!package.IsOwner(user)) | ||
{ | ||
return new HttpStatusCodeWithBodyResult(HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized); | ||
|
@@ -282,7 +299,7 @@ private HttpStatusCodeWithBodyResult VerifyPackageKeyInternal(User user, Credent | |
|
||
[HttpPut] | ||
[RequireSsl] | ||
[ApiAuthorize(SecurityPolicyAction.PackagePush)] | ||
[ApiAuthorize] | ||
[ApiScopeRequired(NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)] | ||
[ActionName("PushPackageApi")] | ||
public virtual Task<ActionResult> CreatePackagePut() | ||
|
@@ -292,7 +309,7 @@ public virtual Task<ActionResult> CreatePackagePut() | |
|
||
[HttpPost] | ||
[RequireSsl] | ||
[ApiAuthorize(SecurityPolicyAction.PackagePush)] | ||
[ApiAuthorize] | ||
[ApiScopeRequired(NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)] | ||
[ActionName("PushPackageApi")] | ||
public virtual Task<ActionResult> CreatePackagePost() | ||
|
@@ -302,6 +319,12 @@ public virtual Task<ActionResult> CreatePackagePost() | |
|
||
private async Task<ActionResult> CreatePackageInternal() | ||
{ | ||
var policyResult = await SecurityPolicyService.EvaluateAsync(SecurityPolicyAction.PackagePush, HttpContext); | ||
if (!policyResult.Success) | ||
{ | ||
return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, policyResult.ErrorMessage); | ||
} | ||
|
||
// Get the user | ||
var user = GetCurrentUser(); | ||
|
||
|
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.
Shims will need to be updated as well