Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Add Audit Logging To SecreteManager.exe #4183
Changes from 37 commits
05b8a68
cd9b948
7134128
e661780
87c81eb
7f74cda
80629f8
f71bee0
cc6d25e
db2f714
6676fbe
45d061c
8c4a7d3
665ec4c
e4e3ee6
8cb0a82
e7c01fa
f92854a
8764fe8
4e52cfe
10e3150
c964d13
9204ef1
09eb38a
d6a9dfc
683509f
00b4f16
21c7bfe
f752944
c7e56da
507ce96
394d13c
b53ab8e
5ad6fe2
b2ff903
846b088
0b204be
0a93297
c341bf7
b5995a7
40c421f
5bd1293
b5c497e
b208ff4
2fa7882
939cbef
0bbfd80
032f375
b70116d
f2253b6
13199b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
Yes. I don't think we can merge (validly) until this is address as it build breaks at the NuGet restore step. So, it has to be fixed in some way in order to pass the PR validation build.
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 expressions
throw
instead of setting the properties to invalid values❓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 are a couple of reasons not to throw.
Looking at this code again throw I think it should also be wrapped in a try catch block to ensure that even if the process of extracting clams value throws an exception the process does not cause the service to fail.
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.)
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.
If I understand correctly I think you would be describing a Stub class which I think would be fine. However, the SecurityAutidLogger class is supposed to be non-volatile meaning it should not throw even if logging fails. So, if this does throw in any test it would mean that there is a problem with the class.
NOTES:
SecurityAutidLogger is a wrapper class used to simplify calls to the OpenTelemetry.Audit.Geneva methods. This is because the OpenTelemetry.Audit.Geneva operation have heavy setup costs and are volatile. So, this wrapper reduces the setup costs of audit logging and ensure the logging operations are non-volatile.
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.
Please confirm if you want me to make a stub for this class. My take is that creating a stub is an extra thing we would have to manage, and this class is supposed to be non-volatile so if it is throwing unexpectedly in test cases that is a real problem not a false positive. But I am fine making a stub if you prefer it that way.
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 will try to use the Moq lib to resolve this based on our offline conversation.
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".
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 would like to have an offline conversation with you about commenting in general. I was thinking about trying to start a team wide discussion but I would like to get some individual views first.
In my view the first set of comments should exist because it is not reasonable to assume a new Dev will intuitively know 'why' injection instruction were added. Meaning what are the objects used for and why are they needed. Yes you can 'figure it out' if you go back though history looking for when the lines were added and then examine the other code changes (hoping that it was a small well scoped change) to determine what caused the injection to be added or do global search fining where the objects are used and figuring out what they do in those classes. However, that is an investigation delay that can be avoided if you have a comment.
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 prefer we leave historical comments out of the code though they sometimes make sense in commit descriptions. investigations that go back in history tend to be about blame or providing more descriptive information for a fix e.g., "this partially reverts {some commit} to avoid {some problem}". but the history doesn't usually help figuring the fix out in the first place. put another way
git log
and similar for history, not commentsThere 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 will remove the historical comment.
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.
suggest removing
_ServiceTreeId
andThere 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 enforces validity in Service Tree❓
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.
Nothing other than it being a 'Guid'. The audit logging process from does not verify that a ID is valid just that it is passed and the value meets the data type criteria.
If you pass an empty Guid or a valid Guid for a non-existent service tree ID it would still 'log' to Geneva successfully.
I don't think we have an additional obligation to make this process have to communicate with an external Service Tree API to ensure IDs are valid.
If the 'service' does not use a correct ID then it will still report to Geneva but Geneva will not close any SFI KPI work items associated with the service so in that sense you are incentivized to use a valid and correct ID.
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.
put another way, very few non-empty
Guid
s are not valid service tree IDs. looks like those are ignored until the option is used. that is, this checks forGuid
validity and not service tree validityThere 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 do not think there is any reasonable way for this library to ensure it is a valid service tree ID. I think about this instead as being sort of "tagging" information or metadata that gets passed to Geneva, then Geneva happens to do something interesting with it.
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'm fine w/ the level of validation implemented here. my main concern is the option's description implies something more. how about the following❓
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 get what you mean about the description for input but 'should be' implies that something else is OK to use here.
It is not. The value is only meaningful if it is a valid service tree ID and specifically the service tree ID for your service.
Any other Guid even one for a valid service tree ID for a service other than the one running the process would be invalid.
The value 'would' be accepted by Geneva and recorded but it is not correct and does not benefit the actual service that needs to report so it can close it's KPIs.
However, the way this option is being provided allows for service we do not own to also use this tool and generate a log that can go to Geneva.
We cannot predict what those possible IDs would be or how they will change in the future.
What we do know is that the ID is meant to be a Guid structure that comes from https://aka.ms/servicetree and relates to the specific service owned by the 'caller'.
I will change the comment to be something less directive so it does state must or should and instead just provides the link back to the location where valid IDs can be found.
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.
again, this doesn't actually validate the service tree ID. it only validates the value is a non-empty
Guid
. suggest using a different nameor actually confirming the ID is validThere 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.
this comment would not be necessary if the
ValidateServiceTreeIdOption
name were updated