Auto-configuration defaults fail open: AllowAllAclResolver and in-memory stores are the silent default

[tcheeric]

Summary

Two related defaults in NapAutoConfiguration fail open. Setting nap.enabled=true and nothing else produces a server that authenticates correctly and authorizes nobody in particular: AllowAllAclResolver admits every principal who can prove key control, and InMemorySessionStore/InMemoryChallengeStore silently make sessions per-instance and process-lifetime.

Severity: Medium — neither is a bug in isolation, but the combination is the default, and both failures are silent.

AllowAllAclResolver as the default AclResolver

@Bean
@ConditionalOnMissingBean
public AclResolver aclResolver() {
    return new AllowAllAclResolver();
}

AllowAllAclResolver.resolve() returns AclDecision.allowed(List.of(), List.of()) for every principal. The effect is that anyone who holds any Nostr key gets a session. Empty roles and permissions mean @RequiresPermission handlers still 403 — but combined with #29 (an unannotated handler under a protected prefix is served), "authenticated" becomes "reachable" for any endpoint whose annotation was forgotten.

The deeper problem is that this is indistinguishable from working. An operator wires NAP, logs in with their own key, sees a session, and ships. Nothing reports that the ACL layer is a no-op. NapServerOptions.Builder.build() does the same thing (if (aclResolver == null) aclResolver = new AllowAllAclResolver()), so the fallback is in both paths.

Compare how the TypeScript side handles the analogous decision. createAudienceHostAllowlist() and createMintAllowlist() both throw at wiring time rather than accept an empty list, with the reasoning written down:

There is no empty-list escape hatch: an allowlist that allows everything is the state this exists to make unrepresentable. Throws here, at wiring time, rather than as a uniform 401 per request.

That is the right principle and it should apply to the ACL resolver, which is a broader authorization decision than either allowlist.

RegistryAclResolver has the same shape one level down: create(registry, aclStore) defaults autoProvision to true, so an unknown pubkey is silently written an ACL record carrying registry.defaultRole(). That is a reasonable behaviour for an open-signup app and a surprising one for anything else, and the two-argument overload is the one that reads as the default.

In-memory stores as the default

challengeStore() and sessionStore() default to the in-memory implementations. Consequences an operator will not be told about:

• Sessions die on restart, including a rolling deploy.
• Behind more than one instance, sessions are per-instance. A load-balanced request lands on a node that has never heard of the session, which presents as intermittent random logouts.
• Revocation is per-instance. revokeByPrincipal() on a suspension only reaches the node that served the request; the principal keeps working on every other node until their session expires. That is a security failure, not just an availability one.
• Unbounded growth — see In-memory challenge and session stores never evict: unbounded growth driven by unauthenticated /auth/init #31 (and In-memory challenge and session stores never evict: unbounded growth driven by unauthenticated /auth/init nap#35 for the TypeScript equivalent).

InMemorySessionStore's javadoc says "for testing and single-instance deployments," which is honest. Being the auto-configuration default is what makes it reachable without that sentence ever being read.

Fix

1. Refuse to start with an implicit allow-all. Replace the default bean with a startup failure that names the decision:

@Bean
@ConditionalOnMissingBean
public AclResolver aclResolver(NapProperties properties) {
    if (!properties.allowAllPrincipals()) {
        throw new IllegalStateException(
            "NAP requires an AclResolver bean. Supply RegistryAclResolver (or your own), or set "
          + "nap.allow-all-principals=true to accept every principal who proves key control — "
          + "which is what the previous default did silently.");
    }
    log.warn("nap_acl_allow_all_enabled: every principal proving key control is authorized");
    return new AllowAllAclResolver();
}

The escape hatch stays available and becomes a written decision rather than an omission. The same check belongs in NapServerOptions.Builder.build().

2. Warn loudly on the in-memory stores. These are genuinely useful for local development, so a hard failure would be the wrong trade. A startup log.warn naming both consequences is enough:

log.warn("nap_in_memory_session_store: sessions are lost on restart and are not shared "
       + "between instances — revocation reaches only this node. Supply a JdbcSessionStore "
       + "bean for any multi-instance or production deployment.");

3. Make autoProvision explicit. Deprecate the two-argument RegistryAclResolver.create(registry, aclStore) in favour of the three-argument form, so the choice is made at the call site.

Regression test

@Test
void contextFailsToStartWithoutAnAclResolver() {
    assertThatThrownBy(() -> runner.withPropertyValues("nap.enabled=true").run(ctx -> ctx.getBean(NapServer.class)))
        .hasMessageContaining("nap.allow-all-principals");
}

@Test
void allowAllStartsWhenExplicitlyRequested() {
    runner.withPropertyValues("nap.enabled=true", "nap.allow-all-principals=true")
          .run(ctx -> assertThat(ctx).hasSingleBean(NapServer.class));
}

Documentation

The README's Spring Boot setup block should show an AclResolver bean as part of the minimal configuration, and say plainly that the auto-configured stores are for development. Right now the quickstart produces a server that looks finished and authorizes everyone.

[tcheeric]

Fixed in 569df2b (branch master, not pushed)

No default AclResolver. The bean method now throws unless nap.allow-all-principals=true, with a message naming both the property and the alternative. The escape hatch remains, as a written decision rather than an omission. This matches what createAudienceHostAllowlist() and createMintAllowlist() already do on the TypeScript side.

The in-memory stores stay the default but warn at startup. They are genuinely useful for local development, so failing there would have been the wrong trade. The session-store warning names the consequence that is a security one rather than only an availability one: revokeByPrincipal() reaches a single node, so a suspended principal keeps working on every other instance until their session expires.

Three tests in NapAutoConfigurationAclTest: refuses with no opt-in, refuses when explicitly false, and builds AllowAllAclResolver when asked, with the decision asserted to still be allow-all so the behaviour is unchanged and only the route to it differs.

Tested against the bean method directly rather than through a Spring context, because this module does not depend on spring-boot-test and adding that to assert a two-branch guard would cost more than it proves. The @ConditionalOnMissingBean half is Spring's behaviour and not this class's to re-test. Flagging the tradeoff rather than burying it.

README updated. Its setup block described auto-configuration as supplying an AllowAllAclResolver, so following it produced a server that looked finished and authorized everyone. It now leads with the required AclResolver bean and an example.

Not done: RegistryAclResolver.create(registry, aclStore) still defaults autoProvision to true. The two-argument overload is the one that reads as the default, and deprecating it is a public-API change worth doing on its own rather than inside a security fix.

Full suite green: 197 tests, 0 failures, 0 skipped.

[tcheeric]

Fixed in #36 (branch security/appsec-audit-remediation, released as 0.9.0). The PR body carries the per-finding verification; this issue's earlier comments carry the detail. Will close on merge.