Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
rrodewald wants to merge 4 commits into from
Closed

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

rrodewald wants to merge 4 commits into from

Conversation

@rrodewald
Copy link

@rrodewald rrodewald commented Aug 10, 2021

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

Refactored check for preemptive authentication
08cfac4 
- new protected method isPreemptiveAuthRequest() in AuthenticatorBase
which is overridden in some authenticators
- moved getRequestCertificates() from AuthenticatorBase to
SSLAuthenticator
michael-o
michael-o reviewed Aug 10, 2021
View reviewed changes
michael-o
michael-o reviewed Aug 11, 2021
View reviewed changes
@rrodewald rrodewald force-pushed the delegate-preemptive branch from 46c7905 to d2e8c89 Compare August 12, 2021 09:12
@michael-o
Copy link
Member

michael-o commented Aug 12, 2021

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

rrodewald commented Aug 12, 2021

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

michael-o commented Aug 12, 2021

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
michael-o
michael-o approved these changes Aug 12, 2021
View reviewed changes
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 •
edited
Loading

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

michael-o commented Aug 12, 2021

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

@rrodewald
Copy link
Author

rrodewald commented Aug 12, 2021

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

markt-asf commented Aug 17, 2021

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

michael-o commented Aug 17, 2021

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

markt-asf commented Aug 17, 2021

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

Reviewers

@michael-o michael-o michael-o approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rrodewald @michael-o @markt-asf