azure-activedirectory-identitymodel-extensions-for-dotnet
azure-activedirectory-identitymodel-extensions-for-dotnet copied to clipboard
AzureAD
[Feature Request] Improving Microsoft.IdentityModel Observability
Improving Microsoft.IdentityModel Observability
There have been some internal asks for Microsoft.IdentityModel and dependent libraries to offer a better observability surface. Currently logging is possible but good hooks for metrics and thus alerting aren't as readily available.
Observability areas to consider
- Logging signals to monitor
- Additional logs that can clarify behavior for consumers
- Metrics/Counters which can hopefully represent some dimensional slices
- System Resources to Monitor
- While the library won't be able to monitor its own system resource usage, it may be able to emit information that will allow consumers to better correlate operations with resource consumption.
- Anything which could assist in making Safe Deployment Practices more straightforward for services which consume the library
- Alerting
- Addition of metrics/counters will likely make this easier however anything else that can make alerting easier should be considered
Areas of focus for observability
Some metrics/counters around time taken in validation and its dependencies (crypto calls etc.) as well as information about internal caching crypto cache/LRU info.
Means of communicating the logs/metrics
Microsoft.IdentityModel.Tokens.CallContext would appear the most promising place since it already contains the log collection.
Another possibility would be to extend scenario specific return values, e.g. TokenValidationResult so they could be decorated with metrics/logs specific to the scenario. I'm currently partial to using CallContext.
@kevinchalet , @leastprivilege -- I was hoping either of you might share your feedback ideas on anything you'd like to see improved w.r.t. observability in the library. Additionally if you know of anyone else who you feel would be useful to solicit feedback for improvements on observability in the library, it would be much appreciated if you could tag them for their attention. Thanks.
@TimHannMSFT thank you for reaching out!
One of the biggest complaints I have with IdentityModel regarding logging is the fact that the error messages became useless OOTB since the introduction of the PII protection (that is enabled by default). Messages like IDX10205: Issuer validation failed. Issuer: 'System.String'. Did not match: validationParameters.ValidIssuer: 'System.String' or validationParameters.ValidIssuers: 'System.String' are cryptic for users trying to debug them. It's a huge source of bug reports for libraries built on top of IdentityModel, like OpenIddict.
Potential ways to improve that:
- https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/1741 helped a bit but most exception messages are still crude and don't clearly mention that "System.String" values are actually redacted data that can be logged by using the static
ShowPIIproperty. I'd recommend making the exception messages more explicit about this option. - Making PII logging configurable per token handler instance would be a great way to allow libraries leveraging IM to enable or disable it in a non-static way. In OpenIddict, this is typically the kind of thing that could be leveraged to include PII in log messages when
IHostEnvironment.EnvironmentNameindicates a development environment.
Also, IIRC, the assumption is that the Trace level of logging can contain sensitive values -- so just always include the details at that level.
I agree that the PII model is very confusing for users, IDX10205: Issuer validation failed. Issuer: 'System.String'. Did not match: validationParameters.ValidIssuer: 'System.String' or validationParameters.ValidIssuers: 'System.String' looks like a bug, not a feature. Even something like IDX10205: Issuer validation failed. Issuer: '[PII redacted]'. Did not match: validationParameters.ValidIssuer: '[PII redacted]' or validationParameters.ValidIssuers: '[PII redacted]' would at least better communicate the intent.
Is there any integration available with Microsoft.Extensions.Logging? That would be helpful to ASP.NET Core consumers, and others.
@Tratcher we currently use System.Diagnostics.Tracing.EventSource, does Microsoft.Extensions.Logging play well with that OR would we need to re-tool?
@brockallen yes one has to assume sensitive info in log messages. Are you suggesting that different levels should expose more or less details?
@Tratcher we currently use System.Diagnostics.Tracing.EventSource, does Microsoft.Extensions.Logging play well with that OR would we need to re-tool?
@shirhatti?
@Tratcher / @kevinchalet , was #1725 helpful to that end. Agreed we still have improvements to be made around this but hopefully that was a step forward on this journey, there's a message which points folks towards https://aka.ms/IdentityModel/PII as well as no longer stripping the arguments that really aren't PII.
Are you suggesting that different levels should expose more or less details?
ASP.NET Core documents that trace level can contain sensitive info, so don't enable it in prod.
@Tratcher / @kevinchalet , was #1725 helpful to that end. Agreed we still have improvements to be made around this but hopefully that was a step forward on this journey, there's a message which points folks towards https://aka.ms/IdentityModel/PII as well as no longer stripping the arguments that really aren't PII.
It's a start. I think some visual changes like those suggested above would help make this more understandable.
https://aka.ms/IdentityModel/PII should also give an example of an error message with PII on vs off so people understand what to expect.
For the obfuscation, I am linking an issue I filed on MSAL. There are opportunities to provide even more obfuscation levers into the customers hands.
Some of the 1P libraries for example do the job of categorizing the data and then the lib user might decide that tenant metadata will never be obfuscated, only data relevant to user.
