Add AuthN/AuthZ metrics #59557
Add AuthN/AuthZ metrics #59557
Conversation
MackinnonBuck
requested review from
BrennanConroy,
halter73 and
captainsafia
as code owners
There was a problem hiding this comment.
Copilot reviewed 5 out of 16 changed files in this pull request and generated 1 comment.
Files not reviewed (11)
- src/Http/Authentication.Core/src/Microsoft.AspNetCore.Authentication.Core.csproj: Language not supported
- src/Http/Authentication.Core/src/PublicAPI.Unshipped.txt: Language not supported
- src/Security/Authentication/test/Microsoft.AspNetCore.Authentication.Test.csproj: Language not supported
- src/Security/Authorization/Core/src/Microsoft.AspNetCore.Authorization.csproj: Language not supported
- src/Security/Authorization/Core/src/PublicAPI/net10.0/PublicAPI.Unshipped.txt: Language not supported
- src/Security/Authorization/Core/src/PublicAPI/net462/PublicAPI.Unshipped.txt: Language not supported
- src/Security/Authorization/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt: Language not supported
- src/Security/Authorization/test/Microsoft.AspNetCore.Authorization.Test.csproj: Language not supported
- src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs: Evaluated as low risk
- src/Http/Authentication.Core/src/AuthenticationMetrics.cs: Evaluated as low risk
- src/Http/Authentication.Core/src/AuthenticationService.cs: Evaluated as low risk
dotnet-policy-service
bot
added
the
pending-ci-rerun
| /// <param name="transform">The <see cref="IClaimsTransformation"/>.</param> | ||
| /// <param name="options">The <see cref="AuthenticationOptions"/>.</param> | ||
| /// <param name="services">The <see cref="IServiceProvider"/>.</param> | ||
| public AuthenticationService( |
There was a problem hiding this comment.
Question: How is the DI decide which constructor to use creating this service? Was it the order and it would pick the firs one? if it is, should the constructor with IServiceProvider placed first?
| { Succeeded: true } => "success", | ||
| { Failure: not null } => "failure", | ||
| { None: true } => "none", | ||
| _ => throw new UnreachableException($"Could not determine the result state of the {nameof(AuthenticateResult)}"), |
There was a problem hiding this comment.
Is this the expected pattern? If the meter is unable to determine this, I don't think it's up to the meter to decide if an exception should be thrown or not. Hence, I wonder if simply using a value like "unknown" is a more reasonable approach here.
My main point is that the authentication stack should be responsible for determining if a certain state is valid or not, not a meter.
There was a problem hiding this comment.
I see that no guidance has been provided in documentation either. Perhaps it's something we should provide guidance for. @JamesNK, what do you think?
There was a problem hiding this comment.
The UnreachableException is more of an assertion than anything else. It's declaring that one of the specified conditions must be true. If this exception ever gets thrown in reality, it indicates a framework bug.
I'm not sure we'd want to introduce a fourth value like "unknown" that customers need to be aware of.
There was a problem hiding this comment.
Maybe an alternative is to just make "none" the default case, although then we're not validating that the result actually is "none".
There was a problem hiding this comment.
| _ => throw new UnreachableException($"Could not determine the result state of the {nameof(AuthenticateResult)}"), | |
| _ => "_OTHER", // _OTHER is commonly used fallback for an extra or unexpected value. Shouldn't reach here. |
It doesn't look like you should ever get here, but just in case, there should be a fallback value. _OTHER is often used: https://github.com/search?q=repo%3Aopen-telemetry%2Fsemantic-conventions%20_OTHER&type=code
| @@ -77,11 +104,13 @@ public virtual async Task<AuthenticateResult> AuthenticateAsync(HttpContext cont | |||
| // Handlers should not return null, but we'll be tolerant of null values for legacy reasons. | |||
| var result = (await handler.AuthenticateAsync()) ?? AuthenticateResult.NoResult(); | |||
|
|
|||
| _metrics?.AuthenticatedRequest(scheme, result); | |||
There was a problem hiding this comment.
AuthenticationService.AuthenticateAsync can throw errors:
- No auth scheme
- Missing handler
- Can handler AuthenticateAsync throw?
- Can transform TransformAsync throw?
This metric should handle error cases.
Also, do people care how long authenticating takes? Should this be a histogram rather than a counter? A histogram can be used for both measuring count and duration. This question applies to a lot of the metrics here because they're in async methods.
| @@ -75,6 +132,9 @@ public virtual async Task<AuthorizationResult> AuthorizeAsync(ClaimsPrincipal us | |||
| } | |||
|
|
|||
| var result = _evaluator.Evaluate(authContext); | |||
|
|
|||
| _metrics?.AuthorizedRequest(policyName, result); | |||
There was a problem hiding this comment.
This should handle error situations, e.g. policy name can't be found, error from handler
Should something be recorded about the user? A flag for anonymous vs authenticated user.
|
Looking at this PR from a higher level, what are the main scenarios that people will use these? A bad outcome would be us going through the motions of adding metrics so we can say they're there. We should think about scenarios and make the metrics as useful as possible in real world scenarios. For example, will people use authn/authz metrics to:
I'm not an auth expert so I'd to hear from the folks who work with auth most often. Debugging auth that isn't working, including exceptions, seems like it is the most valable scenario. It would be useful to collect metrics on an app that runs into some of the most common errors that people have with auth and see what can be done to make the output useful for them. For example, in Kestrel from a developer's perspective it's hard to know why a connection was closed. The server knows why, so we have a set of known reasons for closing a connection. When Kestrel records that a connection is closed non-gracefully it includes that known reason as the Applying that idea to authn/authz: common reasons for errors could be included as the
Surfacing up known error reasons from auth handlers might also be useful. For example, would it be valuable for the cookie auth handler to add metadata to a metric that it wasn't able to authenticate the HTTP request because it couldn't decrypt the cookie payload? |
Add AuthN/AuthZ metrics
Adds ASP.NET Core authentication and authorization metrics.
Description
This PR adds the following metrics:
Ready to be reviewed, but the counter names, descriptions, and tags need to go through API review before this merges.
Fixes #47603