Skip to content

fix(security): keep the login form entry point when X.509 may fall through to other mechanisms - #16974

Open
borinquenkid wants to merge 2 commits into
7.0.xfrom
fix/x509-entry-point-7.0.x
Open

borinquenkid wants to merge 2 commits into
7.0.xfrom
fix/x509-entry-point-7.0.x

Conversation

@borinquenkid

Copy link
Copy Markdown
Member

Fixes #15863
Fixes #15864

useX509 replaced the authenticationEntryPoint bean with a Http403ForbiddenEntryPoint unconditionally. A request without a usable certificate that needed authentication therefore got a bare 403, even with x509.continueFilterChainOnUnsuccessfulAuthentication = true, which is the default and is documented as "proceed ... to allow other authentication mechanisms to process the request". Both issues report exactly that: form login cannot coexist with X.509, and the reporters worked around it with a URL mapping for the error status.

The 403 entry point is now defined only when x509.continueFilterChainOnUnsuccessfulAuthentication is false, that is when certificates are the only way in. Otherwise the plugin's normal entry point stays, so the request goes to the login form (or its Ajax equivalent). x509.adoc now says what each value does.

Behavior change

With the default (true), an application that uses X.509 only and relied on the bare 403 for a request without a certificate now redirects that request to /login/auth. Such an application keeps the old behavior by setting grails.plugin.springsecurity.x509.continueFilterChainOnUnsuccessfulAuthentication = false, which also matches what the property says. I judged that better than a new property, but it is the point for reviewers to weigh.

Tests

  • X509EntryPointSpec (new) builds the plugin's real bean definitions with useX509 on and checks the authenticationEntryPoint bean for both values of the property. The true case fails before the change (the bean was the 403 entry point).
  • All 175 plugin tests pass; CodeNarc, Checkstyle and rat are clean.

Not covered: a request with a real client certificate through a running server; the functional-test-app has no TLS setup for it.

https://claude.ai/code/session_01Dmki4RzWwreEC2GkkyFnvL

…fall through

useX509 replaced the authenticationEntryPoint bean with a Http403ForbiddenEntryPoint
unconditionally, so a request without a usable certificate got a bare 403 even with
x509.continueFilterChainOnUnsuccessfulAuthentication = true (the default), which is
documented as letting other authentication mechanisms process the request. The 403
entry point is now defined only when that property is false, that is when
certificates are the only way in.

Claude-Session: https://claude.ai/code/session_01Dmki4RzWwreEC2GkkyFnvL
Copilot AI balanced review requested due to automatic review settings November 13, 2026 14:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@jamesfredley jamesfredley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not approving. A properties file that sets the fallthrough flag to false is read as true, so a certificate-only setup still sends the user to the login form.

}

authenticationEntryPoint(classFor('authenticationEntryPoint', Http403ForbiddenEntryPoint))
if (!conf.x509.continueFilterChainOnUnsuccessfulAuthentication) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

application.properties sets x509.continueFilterChainOnUnsuccessfulAuthentication=false. The filter receives Boolean false, because Spring converts the string. This condition treats the nonempty string false as true, so it skips the 403 entry point and keeps the login form. A request without a certificate is redirected to the form instead of being refused. Read the setting as one Boolean and use that for both.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right, the string false is truthy in Groovy, so the entry point and the filter could disagree. Fixed in c3f1d7e: the plugin parses the setting once with Boolean.parseBoolean(String.valueOf(...)) and uses that for both the filter property and the 403 entry point. X509EntryPointSpec now runs both settings as a boolean and as a string, and checks the value the filter bean definition receives.

…ion as one boolean

A value set in application.properties or on the command line is the string 'false', which Groovy truth reads as true,
so the entry point condition kept the login form while the filter, to which Spring converts the string, refused the
request. The plugin now parses the setting once and uses it for both.

Claude-Session: https://claude.ai/code/session_01Dmki4RzWwreEC2GkkyFnvL

@jamesfredley jamesfredley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not approving. The string false is now read as false. yes, on, 1, and a padded true are now read as false, so a setting that used to continue the chain stops it.

x509AuthenticationSuccessHandler(NullAuthenticationSuccessHandler)

// a value from application.properties or the command line is the string 'false', which Groovy truth reads as true
boolean continueFilterChain = Boolean.parseBoolean(String.valueOf(conf.x509.continueFilterChainOnUnsuccessfulAuthentication))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A properties value of yes, on, 1, or a padded true used to continue the chain after certificate authentication failed. Boolean.parseBoolean turns all of those into false, so the chain stops and the request is refused. Read the setting the way the filter property used to, including those spellings and surrounding space.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants