Add AuthN/AuthZ metrics by MackinnonBuck · Pull Request #59557 · dotnet/aspnetcore (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation50 Commits9 Checks27 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

MackinnonBuck

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

@MackinnonBuck

@MackinnonBuck

Copilot

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 16 changed files in this pull request and generated 1 comment.

Files not reviewed (11)

@MackinnonBuck

Kahbazi

mkArtakMSFT

@davidfowl

JamesNK

JamesNK

@JamesNK

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 error.type tag. See the error.type details at https://opentelemetry.io/docs/specs/semconv/dotnet/dotnet-kestrel-metrics/#metric-kestrelconnectionduration

Applying that idea to authn/authz: common reasons for errors could be included as the error.type in some of these metrics:

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?

@halter73

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.

This is good feedback for AuthenticateAsync in particular. Normally it should be very fast because you're usually doing something like decrypting cookies with already-downloaded data protection keys or validating JWTs with already-downloaded JWKs, but this can be slowed down by things like the initial downloading of keys or user-defined callbacks.

I don't think histograms are as valuable for challenge, forbid, sign-in and sign-out though. Generally speaking, this are simply setting headers for things like redirects.

For example, in Kestrel from a developer's perspective it's hard to know why a connection was closed.

I think it's worth differentiating expected error cases like connection closed reasons in Kestrel vs. issues caused by misconfiguration.

I consider things like scheme not found and policy name not found to be misconfiguration. If someone were to run into such an error, that should be able to provide repro steps that would allow a developer to try the scenario themselves why the crank up the logging to see what's going on. I don't think we need metrics for these.

On the other hand, it would be interesting to have metrics for errors originating from calls to identity providers via the RemoteAuthenticationOptions.Backchannel HttpClient since these could be transient. Errors reported via the ?error=... query string parameter when identity providers redirect back to the remote authentication handler's CallbackPath could be transient or due to misconfiguration, but I think it'd be worth it to add metrics for these too. The known OAuth error types are defined in https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1

Both of these are specific to remote authentication handlers, so I think it could be addressed in a follow up PR.

lmolkova

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few minor naming suggestions. James share the key point in the comments - these metrics should probably be histograms. Even if it does not make sense, and some should remain counters, the counters should be reported after operation completes and should include the error.type attribute in case exception has happened.

{
if (_authorizeCount.Enabled)
{
var resultTagValue = result.Succeeded ? "success" : "failure";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it always a boolean flag? E.g. could we add a error.type or some status differentiating failures?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be multiple reasons for a given failure, and each reason is an arbitrary string, so unfortunately I'm not sure we can create distinct categories for failures.

private readonly Counter _signInCount;
private readonly Counter _signOutCount;
public AuthenticationMetrics(IMeterFactory meterFactory)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MackinnonBuck

@MackinnonBuck

@MackinnonBuck

Thanks for all the feedback, everyone!

We had a discussion offline about how to address the feedback here, and this is what we've landed on:

The latest revisions of this PR include these changes.

Debugging auth that isn't working, including exceptions, seems like it is the most valuable scenario.

The current implementation just adds an error.type attribute if an exception gets thrown, but I'm not sure how useful that will be if most reported values are just, e.g., System.InvalidOperationException. Its primary use might be determining how often exceptions are getting thrown rather than what the details are. As @halter73 mentioned, the developer could change the log level if they want to debug more deeply. Do others agree with this, or do we need to include more exception/failure info in these metrics?

JamesNK

@MackinnonBuck

@JamesNK

The current implementation just adds an error.type attribute if an exception gets thrown, but I'm not sure how useful that will be if most reported values are just, e.g., System.InvalidOperationException. Its primary use might be determining how often exceptions are getting thrown rather than what the details are. As @halter73 mentioned, the developer could change the log level if they want to debug more deeply. Do others agree with this, or do we need to include more exception/failure info in these metrics?

The exception name in error.type isn't that useful. It's basically a way to test if there is an error, e.g. a promql query like aspnetcore.authentication.request.duration{'error.type'!=''}.

Including known errors is what makes something like error.type more useful. For example, someone can see that their app which uses Facebook login has 10,000 successful logins, 100 failures because someone clicked login but decided not to give the app permission to use the Facebook profile, 50 failures because there was a transient network error calling a Facebook API, etc.

Metrics are useful when providing an aggregate system view of operations in the app. Telling people to look at the logs to get more information is a solution to someone who needs to debug an error at a particular time, but dev ops folks lose visibility of what the overall cause of errors in auth when looking at thousands of aggreated metrics. Put yourself in their shoes and think about how errors and failures can be categorized, grouped and reported.

Because so much of the cause of auth failure is in the handler, I wonder if handler's need an API to specify tags that are then reported on. The higher level AuthenticationService then uses that API to add tags to the metric.

If it turns out that categorizing and reporting known failure reasons is more work than we want to invest now then that's fine. But before we decide that we should see what kind of information we could provide, come up with a design and estimate the work it involved.

@MackinnonBuck

We discussed this offline and decided that the current implementation wouldn't block us from adding more detailed error reporting in the future.

The authentication handlers provided by ASP.NET Core already have a set of well-known failure reasons (see 1, 2, 3, for example). From these, we could define a set of values for a new aspnetcore.authentication.failure_reason attribute. This set could be something like:

(This is just an example. We would inevitably change or shorten this list in an API review.)

We could specify the failure reason either on the AuthenticateResult, AuthenticationProperties, or AuthenticationFailureException. The metrics logic would then use the specified reason populate the "failure reason" attribute.

We could also consider providing a way to allow handlers to add arbitrary attributes to report, but this may require a more elaborate design (e.g., handlers would need a way of determining whether metrics are enabled before generating custom attributes).

Anyway, the conclusion was that we could go ahead with the PR as it stands and address these further items in a follow-up.

@MackinnonBuck

@azure-pipelines Azure Pipelines

Azure Pipelines successfully started running 3 pipeline(s).

@MackinnonBuck

@MackinnonBuck

halter73

@MackinnonBuck

@MackinnonBuck

Merging for now. We can make adjustments in follow-ups if necessary.

@danmoseley

@MackinnonBuck

I've opened the following two issues to track follow-up work:
#60467
#60468

This was referenced

Apr 1, 2025

Labels

area-auth

Includes: Authn, Authz, OAuth, OIDC, Bearer

pending-ci-rerun

When assigned to a PR indicates that the CI checks should be rerun