Skip to content
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

Delegate check for preemptive authentication from AuthenticatorBase to affected Authenticators #444

Closed
wants to merge 4 commits into from

Conversation

rrodewald
Copy link

The main purpose of the proposed refactoring is to give an individual Authenticator the possibility to decide if preemptive authentication is possible (e.g. if a completely different header is used for authentication).

In addition it yields cleaner code as the certificate handling code and the header name for basic, digest and spnego auth can now be moved to the relevant Authenticators and does not pollute the AuthenicatorBase. FormAuthenticator and NonLoginAuthenticator don't need to override isPreemptiveAuthRequest() as preemptive is not supported/needed.

Main changes:

  • new protected method isPreemptiveAuthRequest() in AuthenticatorBase
    which is overridden in some authenticators
  • moved getRequestCertificates() from AuthenticatorBase to
    SSLAuthenticator

- new protected method isPreemptiveAuthRequest() in AuthenticatorBase
which is overridden in some authenticators
- moved getRequestCertificates() from AuthenticatorBase to
SSLAuthenticator
@michael-o
Copy link
Member

One more nit: I think the check in the header-based authenticators is too generic. Shouldn't they check for a value for their auth scheme only? Basic for Basic <value>, etc.?

@rrodewald
Copy link
Author

That's a good point IMHO. Now that the check is in the individual Authenticators it can easily be made more specific. I'm not too familiar with Digest and SPNEGO but I'll try.

@michael-o
Copy link
Member

That's a good point IMHO. Now that the check is in the individual Authenticators it can easily be made more specific. I'm not too familiar with Digest and SPNEGO but I'll try.

With SPNEGO is like with Basic: Base64 token.

@michael-o michael-o self-requested a review August 12, 2021 10:29
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Makes sense now to me.

@rrodewald
Copy link
Author

rrodewald commented Aug 12, 2021

The removal of the preemptive capability of the SSLAuthenticator makes multiple tests fail:

    [junit] Test org.apache.catalina.valves.rewrite.TestResolverSSL FAILED
    [junit] Test org.apache.tomcat.util.net.TestClientCertTls13 FAILED
    [junit] Test org.apache.tomcat.util.net.TestClientCert FAILED
    [junit] Test org.apache.tomcat.util.net.TestCustomSsl FAILED

Before I change all these tests I'd like to confirm that it is worth it.

@michael-o
Copy link
Member

This needs to analyzed whether the tests are invalid or not.

@rrodewald
Copy link
Author

Will have to look at that in detail, which will take some time. I have to postpone this for 2 weeks because I am on vacation.

@markt-asf
Copy link
Contributor

Preemptive authentication for TLS needs to be retained. There are a few edge cases where it still has an effect. For example when certificateVerification="optional" is used.

@michael-o
Copy link
Member

Preemptive authentication for TLS needs to be retained. There are a few edge cases where it still has an effect. For example when certificateVerification="optional" is used.

Can you explain how?

@markt-asf
Copy link
Contributor

Applied manually so I could:

  • add a change log entry
  • tweak the code formatting
  • retain SSL preemptive auth

@markt-asf markt-asf closed this Aug 17, 2021
@rrodewald rrodewald deleted the delegate-preemptive branch September 6, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants