Skip to content

OnAuthenticationFailed never fires when certificate validation fails #30819

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

Closed
TheNephalim opened this issue Mar 10, 2021 · 6 comments · Fixed by #31370
Closed

OnAuthenticationFailed never fires when certificate validation fails #30819

TheNephalim opened this issue Mar 10, 2021 · 6 comments · Fixed by #31370
Assignees
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Milestone

Comments

@TheNephalim
Copy link

Description

I have a rooted certificate (created a CA and generated a test user certificate from this CA). When attempting to present this certificate, I fully expected the certificate validation to fail because it cannot check a CRL. This failure is desired. However, the handler associated with the OnAuthenticationFailed event never fires.

I stepped through the code in the CertificateAuthenticationHandler.cs and found that the certificate validation fails as expected, but reaches line 140 where the code states:

            return AuthenticateResult.Fail("Client certificate failed validation.");

and the AuthenticationFailed event and its associated event handler are never fired.

I changed the options.RevocationMode to X509RevocationMode.NoCheck. As expected, the validation of the certificate succeeds and reaches line 149 where the code states:

        await Events.CertificateValidated(certificateValidatedContext);

and the CertificateValidated event and associated handler are fired.

Configuration

.NET Core 3.1
Windows 10 Professional
x64 architecture

Regression?

I don't know if this is a regression.

Other information

N/A

@blowdart blowdart added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Mar 10, 2021
@blowdart
Copy link
Contributor

That's not what OnAuthenticationFailed is for I'm afraid. That method is for when the handler fails, due to exceptions during processing.

We updated the comments in the other middlewares, e.g. Jwt but it looks like it never made it to Certificate.

@JunTaoLuo / @HaoK could one of you update the xml doc comments to match the ones in JWT to remove the confusion?

@blowdart blowdart added this to the 6.0-preview3 milestone Mar 11, 2021
@TheNephalim
Copy link
Author

TheNephalim commented Mar 11, 2021

Ok. Thank you for the information, but I'm a little perplexed, because when the validation of the certificate fails, the request still proceeds forward rather than preventing the request from moving forward unless this is by design.

Also if you look at the JwtHandler.js, you have the following:

            if (validationFailures != null)
            {
                var authenticationFailedContext = new AuthenticationFailedContext(Context, Scheme, Options)
                {
                    Exception = (validationFailures.Count == 1) ? validationFailures[0] : new AggregateException(validationFailures)
                };

                await Events.AuthenticationFailed(authenticationFailedContext);
                if (authenticationFailedContext.Result != null)
                {
                    return authenticationFailedContext.Result;
                }

                return AuthenticateResult.Fail(authenticationFailedContext.Exception);
            }

If there are validationFailures, then the handler called Events.AuthenticationFailed which triggers the event and event handler.

Based upon the example(s) in the docs, the OnCertificateValidation seems to be designed for adding additional claims to the identity, checking the certificate against a database, etc. My application will most likely run in an environment where it cannot use the internet to check CRLs and will not have access to offline CRLs either, so setting the revocation mode to NoCheck is probably going to be the way to go anyway.

One thing I also noticed is that the checks seem to be occurring twice, is this because it runs before and after the request?

@blowdart
Copy link
Contributor

Validation failures are acceptable, because you can have multiple authentication handlers, and when one fails another may succeed, Auth failure doesn't stop the pipeline.

If no authentication handlers end up running there's no identity produced, and any authorization will fail.

@Tratcher if we look at JWT it's inconsistent with the actual code comments. What's the expected behaviour here?

@blowdart blowdart removed this from the 6.0-preview3 milestone Mar 11, 2021
@blowdart blowdart added the bug This issue describes a behavior which is not expected - a bug. label Mar 11, 2021
@blowdart blowdart added this to the 6.0-preview4 milestone Mar 11, 2021
@blowdart
Copy link
Contributor

OK we've talked about this internally, we don't have base events, so we have inconsistency.

JWT is does it, because the JWT validator isn't our code, they throw exceptions, we catch and then pass up to fail. OIDC is the same as JWT. Cookie doesn't have validation really. So, we'll change certificate for 6.0 and pass the failures up, to match JWT, and we'll fix the code comments everywhere.

@TheNephalim
Copy link
Author

Thank you very much for reviewing this and your consideration.

@blowdart
Copy link
Contributor

You're welcome.

@HaoK HaoK added ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! Done This issue has been fixed labels Mar 30, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants