feat(admin): expose SPNEGO allowed realms and reject the NTLM/Basic conflict - #3220
Merged
Conversation
…onflict Two gaps on the System -> General screen for SPNEGO. spnego.allowed.realms had no field. It is the allowlist that lets an intentional cross-realm trust through the realm check in SpnegoAuthenticator, and without it the only way to set the property was to edit system.properties by hand, so the control was effectively undiscoverable. It is now an ordinary text input next to the other SPNEGO settings and round-trips through updateConfig/updateForm. "Allow Basic Auth" off together with "Prompt NTLM" on was accepted by the screen but is rejected by SpnegoFilterConfig when it builds its configuration. Because that configuration is created lazily on the first login, the save reported success and SPNEGO only failed later with "Failed to initialize SPNEGO." The combination is now a validation error on the field itself. The new label follows the surrounding SPNEGO labels, which are English in every locale except Japanese; the new validation message is translated in all 17 locales, matching how message keys are handled elsewhere. The equivalent check is not applied to /api/admin/general because that endpoint merges the request body over the stored configuration after validation runs, so guarding it correctly needs the validate/merge order to change first. Tests run: 7, Failures: 0 (AdminGeneralActionTest, including the new round trip).
The label was left in English outside Japanese, on the reasoning that the surrounding SPNEGO labels are untranslated. That was the wrong call: the project rule is to propagate a user-facing string to every fess_label_*.properties, and copying an existing gap is not a reason to widen it. Each translation reuses the realm term the locale already uses for labels.authRealm, so the two labels read consistently.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two gaps on System → General for SPNEGO, found while auditing
org.codelibs.fess.sso.spnego.spnego.allowed.realmshad no fieldSpnegoAuthenticator#isAllowedRealmrejects a client principal whose Kerberos realm is neither the server realm nor listed inspnego.allowed.realms. That allowlist is the supported way to permit an intentional cross-realm trust — but it appeared nowhere inEditForm, so the only way to set it was to hand-editapp/WEB-INF/conf/system.properties. A security control nobody can find does not get used.It is now a text input next to the other SPNEGO settings, and round-trips through
updateConfig/updateFormlike every other key on the screen.EditBody extends EditForm, so/api/admin/generalpicks the field up with no further change."Allow Basic Auth" off + "Prompt NTLM" on was silently fatal
SpnegoFilterConfig#setNtlmSupportthrowsIllegalArgumentException("If prompt ntlm is true, then allow basic auth must also be true."). Fess builds that configuration lazily — on the first login through/sso/— so the screen reported a successful save and SPNEGO then failed with the genericFailed to initialize SPNEGO., with nothing pointing back at the checkbox that caused it.Turning off Basic auth is exactly what a hardening guide tells an operator to do, and
spnego.prompt.ntlmdefaults totrue, so this is easy to hit. The combination is now a validation error attached tospnegoPromptNtlm:i18n
Message
errors.spnego_prompt_ntlm_requires_basic— translated in all 17 locales, matching how message keys are handled elsewhere in the project.Label
labels.spnego_allowed_realms— translated in all 17 locales. Each translation reuses the realm term the locale already uses forlabels.authRealm(Realm/Domaine/영역/领域/Область…) so the two labels read consistently.Note that the other SPNEGO labels on this screen (
labels.spnego_krb5_conf,labels.spnego_allow_basic, …) are still English outside_ja. That is a pre-existing gap, not something this PR introduces, and cleaning it up is a separate change.FessLabels/FessMessagesupdated with the corresponding generated members, following the shape FreeGen produces (same as fix(search): report an unsupported facet field instead of returning no hits #3199).Tests
AdminGeneralActionTest: Tests run: 7, Failures: 0, Errors: 0 — including a new test thatspnego.allowed.realmssurvivesupdateConfig→updateForm, so a subsequent save cannot silently clear it.Also green:
FessLabelsTest,LabelMessageThemeParityTest(both),FessActionDefTest.Not included
The same correlation check is not applied to
/api/admin/general.ApiAdminGeneralAction#put$indexrunsvalidateApion the raw body and only then merges it over the stored configuration, so a body carrying just one of the two fields cannot be judged at validation time. Fixing it properly means changing the validate/merge order, which is a larger change than this PR.Related: #3216 (SPNEGO authenticator fixes), plus documentation corrections in codelibs/fess-docs.