Add auditing for all user actions #3078 #3083
Conversation
| @@ -26,6 +26,7 @@ public AuditActor(string machineName, string machineIP, string userName, string | |||
| public AuditActor(string machineName, string machineIP, string userName, string authenticationType, DateTime timeStampUtc, AuditActor onBehalfOf) | |||
| { | |||
| MachineName = machineName; | |||
| MachineIP = machineIP; // TODO: can we store this information? | |||
There was a problem hiding this comment.
None issue for server IPs. If client IP, this is PII, which means "to investigate" :)
There was a problem hiding this comment.
@yishaigalatzer do you know if we can store client IP for auditing purposes?
There was a problem hiding this comment.
Lets follow up offline with privacy/security folk
There was a problem hiding this comment.
Seems we can store it if we obfuscate the last octet. Updated the code to reflect this. (replace last octet with 0)
I'm confused.. do we save the Api Key value? Why? Are we allowed? Refers to: src/NuGetGallery.Core/Auditing/CredentialAuditRecord.cs:19 in d1d291b. [](commit_id = d1d291b, deletion_comment = False) |
|
Also add failed login attempts (if not there) and failed publish (because of ApiKey) |
| @@ -71,8 +68,8 @@ private class NullAuditingService : AuditingService | |||
| { | |||
| protected override Task<Uri> SaveAuditRecord(string auditData, string resourceType, string filePath, string action, DateTime timestamp) | |||
| { | |||
| var uriString = $"http://auditing.local/{resourceType}/{filePath}/{timestamp:s}-{action.ToLowerInvariant()}"; | |||
| var uri = new Uri(uriString); | |||
| var uri = new Uri($"http://auditing.local/{resourceType}/{filePath}/{timestamp:s}-{action.ToLowerInvariant()}"); | |||
There was a problem hiding this comment.
why the change? also can you break line 70 so it fits on the screen without scrolling?
|
@skofman RE:#3083 (comment) - the value is only stored when: 1) it is an API key and 2) it has just been replaced. Audits will never contain API keys that are in use. That said, this used to be the behavior and we're free to update if this makes no sense. |
|
@yishaigalatzer Added additional audits re: #3083 |
|
Think we are good to merge. Can you have another pass? |
* Remaining actions for Package: list/unlist, edit metadata
Thuis PR ensures the following operations are audited:
Please have a look @skofman1 @xavierdecoster