Review found Phase 1 approved with minor issues. Created cleanup tasks: - napi-identity-provider-wiring (bug: NAPI bypasses IdentityProvider) - panic-free-static-config (code smell: panic/unwrap in production path) - non-exhaustive-public-api (future-proofing public API types) - adr-doc-comments (doc convention: ADR references in new modules) - ssh-session-recv-stub-doc (documentation: mark stubs as planned)
2.5 KiB
id, name, status, depends_on, scope, risk, impact, level
| id | name | status | depends_on | scope | risk | impact | level | |
|---|---|---|---|---|---|---|---|---|
| cleanup/napi-identity-provider-wiring | Fix NapiServerHandler to use IdentityProvider and ForwardingPolicy | pending |
|
moderate | medium | component | implementation |
Description
The NapiServerHandler in crates/alknet-napi/src/serve.rs bypasses the IdentityProvider trait contract, calling config.auth.authenticate_publickey() directly instead of IdentityProvider::resolve_from_fingerprint(). This creates two problems:
-
No Identity stored on session: Without resolving through
IdentityProvider, noIdentitystruct is attached to the connection. Per-identity forwarding rules (principalsfield inForwardingRule) cannot match because there's no identity to match against. -
No forwarding policy check: The NAPI handler's
channel_open_direct_tcpip()doesn't evaluateForwardingPolicy::check()at all. It only handlesalknet-prefixed channels and rejects everything else. This means NAPI-served tunnels have no forwarding access control, defeating the purpose of ADR-031.
The core ServerHandler and SshHandler both correctly use IdentityProvider and ForwardingPolicy. The NAPI handler should be consistent.
Fix:
NapiServerHandlershould holdArc<dyn IdentityProvider>and use it inauth_publickey()NapiServerHandler.auth_publickey()should callidentity_provider.resolve_from_fingerprint()and store the resultingIdentityNapiServerHandler.channel_open_direct_tcpip()should evaluateForwardingPolicy::check()with the identity before proxying, matchingServerHandlerandSshHandlerbehavior
Acceptance Criteria
NapiServerHandlerholdsArc<dyn IdentityProvider>andArc<ArcSwap<DynamicConfig>>auth_publickey()delegates throughIdentityProvider::resolve_from_fingerprint()and storesIdentityon the sessionchannel_open_direct_tcpip()evaluatesForwardingPolicy::check()before proxying- Per-identity forwarding rules work correctly through NAPI
- Existing NAPI tests pass
- New test: forwarding policy deny blocks channel open via NAPI handler
References
- crates/alknet-napi/src/serve.rs — NapiServerHandler to be fixed
- crates/alknet-core/src/server/handler.rs — correct pattern to follow
- docs/architecture/identity.md — IdentityProvider contract
- docs/architecture/decisions/031-forwarding-policy.md — ForwardingPolicy
Notes
Identified during Phase 1 review (W1)
Summary
To be filled on completion