diff --git a/src/porchlight/app.py b/src/porchlight/app.py index e07d51f..d399923 100644 --- a/src/porchlight/app.py +++ b/src/porchlight/app.py @@ -8,7 +8,6 @@ from urllib.parse import urlparse from fastapi import FastAPI from fastapi.staticfiles import StaticFiles from fastapi.templating import Jinja2Templates -from fido2.webauthn import UserVerificationRequirement from slowapi.errors import RateLimitExceeded from starlette.middleware.sessions import SessionMiddleware from starlette.requests import Request @@ -56,7 +55,6 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: rp_id=rp_id, rp_name=app.title, origin=settings.issuer, - user_verification=UserVerificationRequirement(settings.webauthn_user_verification), ) app.state.magic_link_service = MagicLinkService( @@ -82,8 +80,8 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: } oidc_server.keyjar.add_symmetric(client_id, client_cfg.client_secret) - # Register management client (stable secret tied to the session secret) - manage_secret = app.state.session_secret + # Register management client + manage_secret = settings.session_secret or secrets.token_hex(32) oidc_server.context.cdb[settings.manage_client_id] = { "client_id": settings.manage_client_id, "client_secret": manage_secret, @@ -101,21 +99,6 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: raise NotImplementedError("MongoDB backend not yet implemented") -def _resolve_session_secret(settings: Settings) -> str: - """Return the session signing secret, requiring one in production. - - A random per-process secret silently invalidates sessions on restart and - rotates the management client secret, so it is only acceptable for local - development (debug or a localhost issuer). - """ - if settings.session_secret: - return settings.session_secret - host = urlparse(settings.issuer).hostname or "" - if settings.debug or host in ("localhost", "127.0.0.1", "::1"): - return secrets.token_hex(32) - raise RuntimeError("OIDC_OP_SESSION_SECRET must be set in production (non-debug, non-localhost issuer).") - - def create_app(settings: Settings | None = None) -> FastAPI: if settings is None: settings = Settings() @@ -131,8 +114,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: app.state.settings = settings # Session middleware - session_secret = _resolve_session_secret(settings) - app.state.session_secret = session_secret + session_secret = settings.session_secret or secrets.token_hex(32) app.add_middleware( CSRFMiddleware, # ty: ignore[invalid-argument-type] exempt_paths={"/token", "/userinfo"}, diff --git a/src/porchlight/authn/routes.py b/src/porchlight/authn/routes.py index 465ebde..606b315 100644 --- a/src/porchlight/authn/routes.py +++ b/src/porchlight/authn/routes.py @@ -23,19 +23,6 @@ def _login_redirect_target(request: Request) -> str: return "/manage/credentials" -def _is_sign_count_rollback(stored: int, presented: int) -> bool: - """Detect a WebAuthn signature-counter rollback (cloned authenticator/replay). - - Authenticators that don't maintain a counter (incl. synced passkeys) report - 0; when both stored and presented are 0 the counter is meaningless and no - rollback can be inferred. Otherwise the presented counter must strictly - exceed the stored one. - """ - if stored == 0 and presented == 0: - return False - return presented <= stored - - def _establish_authenticated_session(request: Request, user: User) -> None: """Reset the session before recording the authenticated identity. @@ -157,7 +144,7 @@ async def register_magic_link(request: Request, token: str) -> Response: return RedirectResponse("/manage/credentials?setup=1", status_code=303) -@router.post("/login/webauthn/begin") +@router.get("/login/webauthn/begin") async def login_webauthn_begin(request: Request) -> Response: webauthn_service = request.app.state.webauthn_service @@ -169,7 +156,7 @@ async def login_webauthn_begin(request: Request) -> Response: @router.post("/login/webauthn/complete") @limiter.limit("10/minute") -async def login_webauthn_complete(request: Request) -> Response: # noqa: PLR0911 +async def login_webauthn_complete(request: Request) -> Response: webauthn_service = request.app.state.webauthn_service user_repo = request.app.state.user_repo cred_repo = request.app.state.credential_repo @@ -207,9 +194,6 @@ async def login_webauthn_complete(request: Request) -> Response: # noqa: PLR091 stored = await cred_repo.get_webauthn_by_credential_id(matched_credential_id) if stored is not None: - if _is_sign_count_rollback(stored.sign_count, new_counter): - # Counter went backwards — likely a cloned authenticator or replay. - return JSONResponse({"error": "Authentication failed"}, status_code=400) stored.sign_count = new_counter await cred_repo.update_webauthn(stored) diff --git a/src/porchlight/authn/webauthn.py b/src/porchlight/authn/webauthn.py index 5c4c4e2..1c9d8ce 100644 --- a/src/porchlight/authn/webauthn.py +++ b/src/porchlight/authn/webauthn.py @@ -18,16 +18,9 @@ from fido2.webauthn import ( class WebAuthnService: """FIDO2 WebAuthn registration and authentication.""" - def __init__( - self, - rp_id: str, - rp_name: str, - origin: str, - user_verification: UserVerificationRequirement = UserVerificationRequirement.PREFERRED, - ) -> None: + def __init__(self, rp_id: str, rp_name: str, origin: str) -> None: rp = PublicKeyCredentialRpEntity(name=rp_name, id=rp_id) self._server = Fido2Server(rp, verify_origin=lambda o: o == origin) - self._user_verification = user_verification def begin_registration( self, @@ -46,7 +39,7 @@ class WebAuthnService: user=user, credentials=existing_credentials, resident_key_requirement=ResidentKeyRequirement.REQUIRED, - user_verification=self._user_verification, + user_verification=UserVerificationRequirement.PREFERRED, ) return dict(options), state @@ -71,7 +64,7 @@ class WebAuthnService: """ options, state = self._server.authenticate_begin( credentials=credentials, - user_verification=self._user_verification, + user_verification=UserVerificationRequirement.PREFERRED, ) return dict(options), state diff --git a/src/porchlight/config.py b/src/porchlight/config.py index a8225d2..dff55a5 100644 --- a/src/porchlight/config.py +++ b/src/porchlight/config.py @@ -49,10 +49,6 @@ class Settings(BaseSettings): session_secret: str | None = None # If None, a random secret is generated per process session_https_only: bool = True - # WebAuthn user verification requirement: "preferred" (default), "required", - # or "discouraged". Identity providers may want "required". - webauthn_user_verification: str = "preferred" - # Magic links invite_ttl: int = 86400 # seconds diff --git a/src/porchlight/static/webauthn.js b/src/porchlight/static/webauthn.js index 9052d0f..0ed2a50 100644 --- a/src/porchlight/static/webauthn.js +++ b/src/porchlight/static/webauthn.js @@ -19,16 +19,6 @@ function getCsrfToken() { return meta ? meta.getAttribute('content') : ''; } -// Render an alert with the message as text (never as HTML) to avoid XSS. -function showAlert(el, message) { - if (!el) return; - el.replaceChildren(); - const div = document.createElement('div'); - div.setAttribute('role', 'alert'); - div.textContent = message; - el.appendChild(div); -} - async function beginRegistration() { const statusEl = document.getElementById('webauthn-status'); @@ -39,7 +29,7 @@ async function beginRegistration() { headers: { 'Content-Type': 'application/json', 'X-CSRF-Token': getCsrfToken() }, }); if (!beginRes.ok) { - showAlert(statusEl, 'Failed to start registration'); + if (statusEl) statusEl.innerHTML = '
Failed to start registration
'; return; } const options = await beginRes.json(); @@ -84,7 +74,7 @@ async function beginRegistration() { if (statusEl) statusEl.innerHTML = text; } } catch (err) { - showAlert(statusEl, 'Registration failed: ' + err.message); + if (statusEl) statusEl.innerHTML = '
Registration failed: ' + err.message + '
'; } } @@ -93,13 +83,10 @@ async function beginAuthentication() { try { // Step 1: Get options from server (no username needed) - const beginRes = await fetch('/login/webauthn/begin', { - method: 'POST', - headers: { 'X-CSRF-Token': getCsrfToken() }, - }); + const beginRes = await fetch('/login/webauthn/begin'); if (!beginRes.ok) { const data = await beginRes.json(); - showAlert(statusEl, data.error || 'Failed to start authentication'); + if (statusEl) statusEl.innerHTML = '
' + (data.error || 'Failed to start authentication') + '
'; return; } const options = await beginRes.json(); @@ -142,10 +129,10 @@ async function beginAuthentication() { window.location.href = data.redirect || '/manage/credentials'; } else { const data = await completeRes.json().catch(function () { return {}; }); - showAlert(statusEl, data.error || 'Authentication failed'); + if (statusEl) statusEl.innerHTML = '
' + (data.error || 'Authentication failed') + '
'; } } catch (err) { - showAlert(statusEl, 'Authentication failed: ' + err.message); + if (statusEl) statusEl.innerHTML = '
Authentication failed: ' + err.message + '
'; } } diff --git a/src/porchlight/store/sqlite/migrations/003_webauthn_credential_id_unique.sql b/src/porchlight/store/sqlite/migrations/003_webauthn_credential_id_unique.sql deleted file mode 100644 index 23eaa20..0000000 --- a/src/porchlight/store/sqlite/migrations/003_webauthn_credential_id_unique.sql +++ /dev/null @@ -1,5 +0,0 @@ --- A WebAuthn credential_id must be globally unique: lookups by credential_id --- alone (during usernameless authentication) must never resolve to the wrong --- user. The table PK is (user_id, credential_id), which does not prevent the --- same credential_id under two users, so add an explicit unique index. -CREATE UNIQUE INDEX ix_webauthn_credential_id ON webauthn_credentials (credential_id); diff --git a/tests/test_app.py b/tests/test_app.py index 14c3aca..82f1358 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -1,10 +1,7 @@ from unittest.mock import MagicMock -import pytest from httpx import AsyncClient -from porchlight.app import create_app -from porchlight.config import Settings from porchlight.dependencies import ( get_credential_repo, get_magic_link_repo, @@ -59,19 +56,3 @@ async def test_dependency_functions() -> None: assert get_user_repo(request) == "user_repo_sentinel" assert get_credential_repo(request) == "credential_repo_sentinel" assert get_magic_link_repo(request) == "magic_link_repo_sentinel" - - -def test_create_app_requires_session_secret_in_production() -> None: - settings = Settings(issuer="https://op.example.com", sqlite_path=":memory:", debug=False) - with pytest.raises(RuntimeError, match="SESSION_SECRET"): - create_app(settings) - - -def test_create_app_allows_missing_secret_on_localhost() -> None: - settings = Settings(issuer="http://localhost:8000", sqlite_path=":memory:") - assert create_app(settings) is not None - - -def test_create_app_allows_missing_secret_in_debug() -> None: - settings = Settings(issuer="https://op.example.com", sqlite_path=":memory:", debug=True) - assert create_app(settings) is not None diff --git a/tests/test_auth_routes/test_webauthn_login.py b/tests/test_auth_routes/test_webauthn_login.py index 4959045..5952633 100644 --- a/tests/test_auth_routes/test_webauthn_login.py +++ b/tests/test_auth_routes/test_webauthn_login.py @@ -46,12 +46,10 @@ async def _setup_user_with_webauthn( async def test_webauthn_login_begin_returns_options(client: AsyncClient) -> None: - """Begin is a POST (mutates session) with no username — returns options - with empty allowCredentials.""" + """Begin is now GET with no username — returns options with empty allowCredentials.""" await _setup_user_with_webauthn(client) - token = await get_csrf_token(client) - res = await client.post("/login/webauthn/begin", headers={"X-CSRF-Token": token}) + res = await client.get("/login/webauthn/begin") assert res.status_code == 200 data = res.json() assert "publicKey" in data @@ -61,8 +59,7 @@ async def test_webauthn_login_begin_returns_options(client: AsyncClient) -> None async def test_webauthn_login_begin_has_user_verification_preferred(client: AsyncClient) -> None: - token = await get_csrf_token(client) - res = await client.post("/login/webauthn/begin", headers={"X-CSRF-Token": token}) + res = await client.get("/login/webauthn/begin") assert res.status_code == 200 data = res.json() assert data["publicKey"]["userVerification"] == "preferred" @@ -84,9 +81,10 @@ async def test_webauthn_login_complete_returns_json_redirect(client: AsyncClient _userid, _pk, _credential_id, _att = await _setup_user_with_webauthn(client) # Begin to get state into session - token = await get_csrf_token(client) - res1 = await client.post("/login/webauthn/begin", headers={"X-CSRF-Token": token}) + res1 = await client.get("/login/webauthn/begin") assert res1.status_code == 200 + + token = await get_csrf_token(client) # We can't easily complete the full assertion without browser interaction, # but we verify the endpoint returns 400 JSON (not HTML) for bad assertions res2 = await client.post( diff --git a/tests/test_authn/test_sign_count.py b/tests/test_authn/test_sign_count.py deleted file mode 100644 index 5c8a65d..0000000 --- a/tests/test_authn/test_sign_count.py +++ /dev/null @@ -1,22 +0,0 @@ -from porchlight.authn.routes import _is_sign_count_rollback - - -def test_increasing_counter_is_not_rollback() -> None: - assert _is_sign_count_rollback(stored=5, presented=6) is False - - -def test_equal_counter_is_rollback() -> None: - assert _is_sign_count_rollback(stored=5, presented=5) is True - - -def test_lower_counter_is_rollback() -> None: - assert _is_sign_count_rollback(stored=5, presented=3) is True - - -def test_both_zero_sync_passkey_is_allowed() -> None: - # Sync passkeys (and counter-less authenticators) always report 0. - assert _is_sign_count_rollback(stored=0, presented=0) is False - - -def test_first_increment_from_zero_is_allowed() -> None: - assert _is_sign_count_rollback(stored=0, presented=1) is False diff --git a/tests/test_authn/test_webauthn.py b/tests/test_authn/test_webauthn.py index 339d5a5..9c3da22 100644 --- a/tests/test_authn/test_webauthn.py +++ b/tests/test_authn/test_webauthn.py @@ -17,7 +17,6 @@ from fido2.webauthn import ( PublicKeyCredentialDescriptor, PublicKeyCredentialType, RegistrationResponse, - UserVerificationRequirement, ) from porchlight.authn.webauthn import WebAuthnService @@ -195,19 +194,6 @@ def test_begin_authentication_prefers_user_verification() -> None: assert pub_key["userVerification"] == "preferred" -def test_user_verification_is_configurable() -> None: - service = WebAuthnService( - rp_id=RP_ID, - rp_name=RP_NAME, - origin=ORIGIN, - user_verification=UserVerificationRequirement.REQUIRED, - ) - reg, _ = service.begin_registration(user_id=b"user-123", username="alice") - assert reg["publicKey"]["authenticatorSelection"]["userVerification"] == "required" - auth, _ = service.begin_authentication() - assert auth["publicKey"]["userVerification"] == "required" - - def test_begin_authentication_returns_options_and_state() -> None: service = _make_service() _, cred_id, _attested = _generate_credential() diff --git a/tests/test_client_registration.py b/tests/test_client_registration.py index ea6ba37..d1d7e2b 100644 --- a/tests/test_client_registration.py +++ b/tests/test_client_registration.py @@ -19,7 +19,7 @@ scope = ["openid", "profile"] toml_file = tmp_path / "test.toml" toml_file.write_text(toml_content) - settings = Settings(_toml_file=str(toml_file), session_secret="x" * 32) + settings = Settings(_toml_file=str(toml_file)) app = create_app(settings) async with ( @@ -41,7 +41,7 @@ scope = ["openid", "profile"] async def test_manage_app_always_registered() -> None: """The internal manage-app client is always registered, even without config file clients.""" - settings = Settings(issuer="https://test.example.com", session_secret="x" * 32) + settings = Settings(issuer="https://test.example.com") app = create_app(settings) async with ( diff --git a/tests/test_store/test_migrations.py b/tests/test_store/test_migrations.py index 4466d76..386aa5a 100644 --- a/tests/test_store/test_migrations.py +++ b/tests/test_store/test_migrations.py @@ -13,7 +13,7 @@ async def test_run_migrations_applies_initial() -> None: async with aiosqlite.connect(":memory:") as db: await db.execute("PRAGMA foreign_keys=ON") count = await run_migrations(db, MIGRATIONS_DIR) - assert count == 3 + assert count == 2 async with db.execute("SELECT name FROM sqlite_master WHERE type='table' AND name='users'") as cursor: row = await cursor.fetchone() assert row is not None @@ -24,7 +24,7 @@ async def test_run_migrations_skips_already_applied() -> None: await db.execute("PRAGMA foreign_keys=ON") first_count = await run_migrations(db, MIGRATIONS_DIR) second_count = await run_migrations(db, MIGRATIONS_DIR) - assert first_count == 3 + assert first_count == 2 assert second_count == 0 diff --git a/tests/test_store/test_sqlite_credential_repo.py b/tests/test_store/test_sqlite_credential_repo.py index 3b54b61..1c3396a 100644 --- a/tests/test_store/test_sqlite_credential_repo.py +++ b/tests/test_store/test_sqlite_credential_repo.py @@ -227,21 +227,3 @@ async def test_delete_webauthn_if_not_last_allows_when_others_exist( assert await credential_repo.delete_webauthn_if_not_last(alice.userid, b"\x01") is True assert len(await credential_repo.get_webauthn_by_user(alice.userid)) == 0 - - -async def test_webauthn_credential_id_is_globally_unique( - credential_repo: SQLiteCredentialRepository, user_repo: SQLiteUserRepository -) -> None: - # The same credential_id must not be registrable for two different users, - # otherwise lookup-by-credential-id could resolve to the wrong account. - await user_repo.create(User(userid="u-alice", username="alice2")) - await user_repo.create(User(userid="u-bob", username="bob2")) - cred_id = b"\xde\xad\xbe\xef" - - await credential_repo.create_webauthn( - WebAuthnCredential(user_id="u-alice", credential_id=cred_id, public_key=b"\x01") - ) - with pytest.raises(DuplicateError): - await credential_repo.create_webauthn( - WebAuthnCredential(user_id="u-bob", credential_id=cred_id, public_key=b"\x02") - )