Repository navigation
Conversation
Moving user accounts out of SQLite and into MongoDB changes the storage model for authentication, so this is a minor bump rather than a patch.
Groundwork for moving authentication off SQLite. Nothing is wired up yet --
app.py still uses the Flask-SQLAlchemy User model -- so this commit is inert
and independently revertable.
package/user_store.py holds the new store. Accounts live in the collection
named by MONGODB_USERS_COLLECTION (default "users") inside MONGODB_DATABASE,
with the username as the document _id. That choice does real work:
- Uniqueness comes free from _id, so there is no index to build at startup and
no build to fail, and no window in which two registrations of the same name
both succeed. create_user() insert_one()s and lets DuplicateKeyError be the
duplicate check.
- _id matching is binary, which preserves the case-sensitive uniqueness the
SQLite "unique" column gave. No collation is set, deliberately: adding one
would change semantics and could make an existing Bob/bob pair unmigratable.
Accounts carry a "disabled" flag and are never deleted. Deleting a document
would free the username, and since a username is also the MongoDB collection
name and the data/<username> directory name, the next person to register it
would inherit the previous holder's configs, snapshots and SSH keys. The field
is "disabled" rather than "enabled" so that an absent value means usable --
documents written before the field existed keep working. User.is_active
returns not disabled, which Flask-Login consults in login_user(), and
get_user_by_session_id() rejects disabled accounts itself so that disabling
takes effect on the user's next request rather than at their next login.
get_id() returns "u:<username>" rather than the bare name. is_valid_username
permits all-digit usernames, so an unprefixed token would be indistinguishable
from the integer primary key that pre-2.5.0 sessions carry, and a stale
session holding "1" could resolve to the account named "1".
Queries deliberately do not swallow exceptions. Rendering a MongoDB outage as
"no such user" would be the worst available diagnostic.
Supporting changes:
- data_file_functions gains _get_mongo_db(), which defaults MONGODB_DATABASE
to "fwgui_database" (matching app.py's session-store default). pymongo
raises "TypeError: name must be an instance of str" on client[None], and
once accounts live in MongoDB an unset variable would take the login page
down rather than just the config routes. The eight existing
client[os.environ.get("MONGODB_DATABASE")] sites now go through it;
mongo_dump() reads the name back off the handle.
- validators gains is_reserved_username(), hardcoding "users" and "sessions".
A username is a collection name, so a user holding one of those would be
handed an application collection as their config collection: the ordinary
config routes would let them list, read and delete other users' accounts or
session documents. Matching is lowercased and stripped.
MONGODB_USERS_COLLECTION is honoured in addition to -- never instead of --
the hardcoded pair, since an install that renamed the collection may still
hold a "users"-named leftover. is_valid_username() now rejects reserved
names, so every existing caller inherits the guard.
The cutover. User accounts now come from the MongoDB users collection added
in the previous commit; Flask-SQLAlchemy and data/database/auth.db are no
longer read or written. Migrating the accounts of an existing install is the
next commit -- this one changes where the app looks.
app.py:
- Drops the flask_sqlalchemy import, db_location, the SQLALCHEMY_* config,
db = SQLAlchemy(app) and the User model. UserMixin is no longer imported
here either; user_store.User carries it.
- load_user() delegates to user_store.get_user_by_session_id(), which rejects
unknown and disabled accounts.
- /user_logout took a name for its log line by loading the user from the
database. It now reads session["username"], which is already there. One
fewer round-trip and one fewer reason to keep a lookup helper.
package/auth_functions.py loses the db and User parameters rather than keeping
them as dead arguments, so any call site missed while rewriting fails loudly as
a TypeError. bcrypt stays a parameter: it keeps the module free of an app
import and keeps the test suite's MockBcrypt usable.
change_password(bcrypt, db, User, username, request)
-> change_password(bcrypt, username, request)
process_login(bcrypt, db, request, User) -> process_login(bcrypt, request)
register_user(bcrypt, db, request, User) -> register_user(bcrypt, request)
query_user_by_id / query_user_by_username -> deleted (user_store)
Behaviour changes that come with it:
- generate_password_hash() returns bytes and both write paths stored it
unconverted, which is why the old auth.db holds a mix of TEXT and BLOB
password columns. Both now .decode("utf-8"), so no BSON Binary hash is ever
written.
- Registration no longer pre-checks the username. create_user() inserts and
DuplicateKeyError is the answer, closing the query-then-insert window in
which two simultaneous registrations of one name could both succeed.
- Registration rejects reserved usernames with "That username is reserved."
rather than the misleading character-set message.
- change_password() guards a missing account instead of dereferencing None: a
session can outlive the account it belongs to, and that was a 500.
- Disabled accounts are refused at login and at password change. Login gives
the same "Login incorrect." as a bad password -- saying "disabled" would hand
out account enumeration -- and logs a WARNING so an operator can tell the
difference.
- The query helpers' bare "except Exception: return None" is gone. Under
pymongo it would have rendered a MongoDB outage as "Login incorrect.".
- New _password_matches() traps ValueError from check_password_hash, which
bcrypt raises on an empty or malformed stored hash. That failed the request
with a 500 before; it now fails the login and logs which account needs a
password reset.
data_file_functions.initialize_data_dir() no longer creates auth.db, which
removes the deferred "from app import app, db" that existed only to work
around the resulting import cycle. data/database is still created for the
telemetry instance id.
Test suite:
- The MockDB fixture is gone; user_model now builds a real user_store.User so
the "u:<username>" get_id() contract and the disabled/is_active mapping are
under test rather than mocked.
- New session-scoped mongo_client fixture installs mongomock before flask_app,
which seeds testuser into the users collection with a real bcrypt hash at
rounds=4 so auth_client's real POST /user_login still works offline. Two
side benefits: the suite no longer writes testuser into the developer's real
data/database/auth.db, and dropping from 12 rounds to 4 takes the full run
from ~31s to ~2s.
- MockBcrypt now returns bytes from generate_password_hash, mirroring
flask_bcrypt, so the new .decode() calls are exercised.
- test_auth_functions.py asserts against seeded mongomock documents instead of
db.session mock calls, and adds coverage for the disabled flag, an absent
disabled field, an unusable stored hash, reserved usernames, a vanished
account, and a MongoDB error propagating instead of being swallowed.
Existing installs keep working with no operator action: pre-2.5.0 accounts in data/database/auth.db are copied into the MongoDB users collection at startup. package/user_migration.py. The migration is idempotent two independent ways, because the file marker alone is not trustworthy -- a data/ volume restored from an older snapshot, or a curious operator, can make it disappear: 1. auth.db is renamed to auth.db.migrated once the copy succeeds, so the next boot has nothing to find. The rename is the marker; there is no separate flag file or sentinel document. (A sentinel document would have shown up in list_usernames(), and "_migration" is itself a valid username.) 2. Every write is $setOnInsert. This is the important one. With $set, a re-run against already-migrated accounts would restore each user's pre-cutover password hash -- including one rotated precisely because it leaked -- and re-enable an account the operator had since disabled. Two tests cover exactly that. The renamed file is deliberately left in place as the rollback path for an operator who downgrades the image. It never overwrites an existing auth.db.migrated, which would destroy an earlier migration's snapshot; the second one gets a timestamp suffix instead. Other details that matter: - The legacy password column holds a mix of TEXT and BLOB, because generate_password_hash returns bytes and the pre-2.5.0 write paths stored it unconverted. Bytes are decoded to str (bcrypt hashes are ASCII, so this is lossless), and an undecodable value is skipped with a warning rather than written as BSON Binary. - Rows with no username or no password are skipped: bcrypt raises ValueError on an empty hash, which would have been a 500 on that user's login. - SQLite is opened read-only via a file: URI, so no -wal/-shm sidecars appear next to a database the app otherwise no longer touches. The table is queried as "user" -- SQLAlchemy's implicit name -- rather than mongo_converter's old "FROM User", which only worked because SQLite identifiers are case-insensitive. - An unreadable or corrupt auth.db is logged and skipped *without* renaming, so the next boot retries. - Two abort paths log CRITICAL and exit rather than corrupting anything: a target collection already holding non-account documents (i.e. it is some user's config collection -- set MONGODB_USERS_COLLECTION), and a legacy row whose username is reserved (rename the account and restart). - Server-side sessions are purged, both the MongoDB sessions collection and the filesystem session directory. Flask-Login's token format changed, so every pre-cutover session is stale; deleting them logs everyone out once at the cutover instead of leaving tokens that cannot resolve. - DuplicateKeyError on the upsert is treated as already-migrated, which is what a concurrent boot with replicaCount > 1 looks like. Wiring: app.py runs migrate_sqlite_users() before mongo_converter(), which now takes its user list from user_store.list_usernames() instead of querying auth.db directly -- hence the ordering. process_login() also calls it, guarded by the same missing-file short-circuit, because app.py's __main__ block is the shipped entrypoint but a WSGI server (gunicorn app:app) never executes it and every account would appear to have vanished. mongo_converter keeps reading per-user directories rather than globbing data/*/*.json: the glob would match data/uploads/*.json, the transient upload staging area, and import a leftover upload into a collection named "uploads". Test suite: conftest gains a session-scoped autouse fixture pointing LEGACY_AUTH_DB at a path that does not exist. Without it the new process_login fallback migrates and renames the developer's real data/database/auth.db and purges their real session store as soon as any test logs in -- which is exactly what happened the first time the suite ran. Verified against a live MongoDB with the real mixed TEXT/BLOB auth.db: both hashes migrate byte-identically as str, the file is renamed, a re-run after a password change preserves the new hash, and register/login/disable/re-enable behave correctly through the real routes.
Nothing imports it since authentication moved to MongoDB. Removing it also drops sqlalchemy and greenlet from the resolved set -- 96 lines out of uv.lock. Kept as its own commit so the lockfile churn does not drown the review, and because the Dockerfile's "uv sync" fails outright if pyproject.toml and uv.lock disagree -- a mismatch that would only surface at image build time. Verified: the full suite passes and app.py imports with sqlalchemy uninstalled from the venv.
POST /download read any file under data/ and returned it to any logged-in user.
The containment check was correct as far as it went -- os.path.commonpath, no
sibling-prefix bypass -- but containment within data/ was never a meaningful
boundary. Reachable that way: data/database/auth.db (and, after the migration,
auth.db.migrated), every full-backup zip, other users' generated .conf files
and per-user backup zips, and data/log/app.log.
Moving accounts into MongoDB made that worse rather than better:
mongo_dump() sweeps every collection, so a full backup now contains
mongo_dumps/<ts>/<db>/users.bson -- the whole bcrypt hash set, in a format
that is trivially greppable where auth.db at least needed sqlite.
Rather than scope the route, delete it. It has no caller: no url_for('download')
and no action="/download" in any template or static file, and the admin page
tells operators outright that backups "must be accessed via operating system
level or from S3", listing them as plain text with no link. The only consumers
were its own two tests. The UI's real download endpoints, /download_config and
/download_json, build their response from the session's own configuration and
take no caller-supplied path; they are untouched.
Kept: the create_backup exclusion work is separate (next commit). The zips
still leave the host for S3, and the Helm initContainer chmods the data
directory 777 inside the pod, so removing the in-app read path is necessary but
not sufficient.
Release note: any operator scripting a POST to /download loses it.
create_backup() zips all of data/, excluding only backups/, tmp/, uploads/ and *.key. That swept up data/database/auth.db, and now sweeps up the auth.db.migrated the user migration retains -- a complete set of bcrypt hashes that nothing in the application reads any more, shipped into every full-backup zip and from there to S3 unencrypted when BUCKET_NAME is set. Exclude auth.db* from the walk. Current accounts are already captured by mongo_dump()'s users.bson, so nothing is lost; the legacy file stays on the host as the downgrade path but no longer travels. Test covers the new exclusion alongside the existing *.key and backups/tmp/uploads ones, running against a temp data/ tree rather than the real one.
docs/data-architecture.md keeps both architectures rather than replacing one with the other, so it stays usable for an operator running an older image. A version note up front, and §1 (Overview) and §4 (Authentication store) each split into "2.5.0+" and "Pre-2.5.0" subsections with their own tables and diagrams. New material in §4: - The users document shape, and why _id is the username: uniqueness with no index to build, DuplicateKeyError as the registration check, and binary matching preserving SQLite's case-sensitive semantics (hence no collation). - Reserved usernames, with the reason -- a username is a collection name, so a user named "users" or "sessions" would get an application collection as their config collection and could read and delete other people's accounts or sessions through the ordinary routes. - The disabled flag: the operator recipe, why it is "disabled" and not "enabled", the three enforcement points, and an explicit warning that deleting the document instead frees the username and hands the next registrant the previous holder's configs, snapshots and SSH keys. - The "u:<username>" session token and why the prefix exists. - Pre-2.5.0 marked historical, including the TEXT/BLOB password-column mix and what auth.db.migrated is -- specifically that a downgrade loses every account created and password changed after the cutover. Elsewhere: §2 documents _get_mongo_db()'s default and that no application code creates indexes; §5 covers the changed _user_id format and the one-time session purge; §6 relabels the database directory and records the /download removal; §7 notes users.bson now appears in every dump and that auth.db* is excluded from the zip; §8 becomes "Startup migrations", documenting the new one (including why $setOnInsert rather than $set) alongside the JSON converter and the ordering between them; §11 gains MONGODB_USERS_COLLECTION and corrects the MONGODB_DATABASE default. Stale file:line references throughout are fixed. Deployment config: both compose files and chart/fw-gui/values.yaml now set MONGODB_DATABASE explicitly, so behaviour no longer depends on the .env baked into the image. The compose security comments and the chart README say plainly that this MongoDB is now the credential store -- previously the hashes needed filesystem access inside the fw-gui container, and the chart's initContainer chmods the data directory 777 in-pod. MONGODB_USERS_COLLECTION is documented, commented out, in .env and examples/.env.prod. CLAUDE.md: single-database tech stack, user_store.py and user_migration.py in the module table, the users collection in Data Storage with the disable-don't-delete rule, and the new env vars. README.md's doc index line updated.
The migration retains data/database/auth.db as auth.db.migrated so that downgrading the image still has accounts, but the procedure for using it was never written down and two of its consequences were neither documented nor signalled. docs/data-architecture.md §4 gains a "Downgrading and re-upgrading" subsection: - The procedure, and why the order matters: stop the app, rename the file back, then start the old image. Pre-2.5.0 initialize_data_dir() runs db.create_all() when auth.db is absent, so starting the old image first leaves an empty database -- no accounts and an open registration page. - What the downgrade costs, including the one that is easy to miss: pre-2.5.0 has no `disabled` concept, so **disabled accounts become active again**. Plus the already-documented losses (post-cutover accounts absent, post-cutover password changes reverted), the one-time logout because the "u:<username>" token matches no integer primary key, and /download returning. - Re-upgrading, which is where data is actually lost. The users collection survives the downgraded period and $setOnInsert makes the MongoDB document win, so a plain re-upgrade discards every password change and disable applied while downgraded; only genuinely new users are inserted. That default is correct -- overwriting live accounts from a stale SQLite file would be worse -- but it has to be visible. Documents db.users.drop() as the alternative, and states what that costs in turn: it makes SQLite the whole truth, so post-cutover accounts vanish and disabled accounts re-enable. - That the rollback data is on the volume, not in a backup: post-cutover zips exclude auth.db* and carry users.bson instead. §8.1's "to re-run deliberately" note now says what a re-run does *not* do -- it read as though it re-syncs existing accounts. user_migration.py collects the usernames it skipped because a MongoDB document already existed and logs one WARNING naming them. On a first migration nothing is skipped, so this fires only in the re-upgrade / restored-volume case. WARNING rather than ERROR: nothing has gone wrong, but which copy won should not go unsaid. DuplicateKeyError stays a bare continue -- that is a concurrent-boot insert, not an already-present account, and should not inflate the warning on a multi-replica boot. Two tests, both mutation-checked (each fails under the mutation it targets): the warning fires and names the account on a re-run, and does not fire on a clean first migration. Verified end to end with real images and containers rather than only in-process: registered two accounts on 2.4.3 (both stored BLOB, as expected), upgraded to 2.5.0 and confirmed both still log in with their original passwords, created a post-cutover account and disabled one, then followed the documented rollback to 2.4.3 -- the original password worked, the disabled account was usable again, the post-cutover account was gone, and /download answered 400 (CSRF) rather than 2.5.0's 404. Changed a password and registered a user while rolled back, then re-upgraded: the WARNING named both pre-existing accounts, the MongoDB hashes won, the disable was restored, and the rollback-era user was inserted. Finally db.users.drop() plus a restart adopted the SQLite state, including the rollback-era password, and lost the post-cutover account -- exactly as the new text says.
data/database/instance.id was the last live file in data/database/; with accounts
already in MongoDB, moving it there lets that directory be retired in one later
cleanup alongside auth.db.migrated.
New package/instance_id.py owns the id. Collection "instance", one document:
{"_id": "instance_id", "value": "<uuid4>", "created": datetime}
Resolution order: FWGUI_INSTANCE_ID env var, then a process cache, then MongoDB.
Two properties drive the whole design, and both are mutation-tested:
1. **Nothing here may raise.** telemetry_commit() and telemetry_diff() are called
from napalm_ssh_functions:128 and :199 *outside* the try blocks that guard a
firewall push, and get_instance_id() previously caught only OSError. A
PyMongoError would have 500'd a commit. The MongoDB path is wrapped in a broad
except; every route returns a string, and an unavailable id is "".
2. **The lookup is time-bounded.** The shared client sets no
serverSelectionTimeoutMS, so pymongo's 30s default applied per event -- and
telemetry_instance() is in the login path. Measured: four telemetry calls
against an unreachable database took 121s. Wrapped in pymongo.timeout(2.0),
the same four take 8s. A regression test asserts the bound is applied, since a
wall-clock assertion would be flaky.
Other deliberate choices:
- Seeding happens lazily in the getter, not in initialize_data_dir(). That
removes the startup-ordering coupling entirely and works under any entrypoint,
including "gunicorn app:app", which never runs __main__ -- the gap the auth
migration needed a process_login fallback for.
- The process cache exists because get_instance_id() fires on every telemetry
event and used to be a free file read. Only a non-empty value is cached, so a
transient outage does not pin "" for the life of the process.
- $setOnInsert plus a read-back, so two concurrent callers converge on one value
instead of racing to overwrite each other.
- A pre-2.5.0 instance.id is adopted so an upgraded install keeps its telemetry
identity, then renamed to instance.id.migrated (never clobbering an existing
one). Retiring the file is best effort: the id is already stored and the stored
value wins on every later read, so a read-only data directory costs a leftover
file rather than the id.
"instance" joins users and sessions in _RESERVED_USERNAMES -- a username is a
collection name, so otherwise a user could be handed the telemetry collection as
their config collection. But the severity differs, so the notion is split:
is_auth_critical_username() (users/sessions only) is what the startup user
migration aborts on. Refusing to boot over a telemetry UUID would be
disproportionate, and would newly break an install that has a user named
"instance" and upgrades fine today. That collision instead logs an ERROR and
degrades telemetry.
telemetry_instance() moves after the validate_mongodb_connection() block in
app.py's __main__ so it has a real id and cannot stall the boot. Trade-off worth
naming: a broken-MongoDB install used to post here and *then* exit, so it
reported; now it dies silently. Recovering that signal needs a deliberate
"startup failed" event, out of scope.
CI's data/database/instance.id seeding step is replaced by
FWGUI_INSTANCE_ID: github-actions-ci, which keeps the marker without a file. The
env var is documented (commented out) in .env and examples/.env.prod.
conftest gains no_real_instance_id_migration, mirroring
no_real_sqlite_migration. This is not hypothetical: the auth migration silently
renamed the developer's real auth.db the first time that suite ran, and the same
hazard applied here. Verified afterwards that data/database/instance.id is
untouched by a full run.
Verified end to end with real containers: a volume carrying a pre-migration
instance.id was adopted (same UUID now in the instance collection,
migrated_from_file true, file renamed), restart was a no-op with one document,
register and login still work with telemetry in the login path, FWGUI_INSTANCE_ID
short-circuited without the collection ever being created, and registering
"instance" was refused.
docs/data-architecture.md §10 Telemetry is rewritten with the same 2.5.0+ / Pre-2.5.0 split §4 already uses: the document shape, the three-step resolution order (FWGUI_INSTANCE_ID, process cache, MongoDB) and why each step exists, the reserved-name situation and why an "instance" collision degrades telemetry rather than aborting startup like users/sessions, and the two mutation-tested invariants -- that nothing in instance_id.py raises, and that the lookup is bounded by pymongo.timeout(2.0) with the measured 121s-to-8s figure that motivated it. It also records that telemetry_instance() now runs after the MongoDB check, and what that costs: an install which cannot reach MongoDB used to post and then exit, so it reported; it now dies silently. Elsewhere in the same doc: the version note covers both stores that moved rather than just auth; §1's 2.5.0+ table and diagram fold the telemetry id into the application database and drop it from the filesystem row (the Pre-2.5.0 table is left alone -- it was on disk then); §4's "Downgrading and re-upgrading" gains the second rename a downgrade needs, with the consequence that skipping it makes the install report as brand new; §6's filesystem diagram and prose say data/database/ now holds nothing current code writes; §11 gains FWGUI_INSTANCE_ID. The stale initialize_data_dir() line reference is corrected. CLAUDE.md: instance_id.py in the module table, the instance collection in Data Storage with the adoption/retirement note and the reserved-name rule, the telemetry row noting it cannot break a config push, and the new env var. Also corrected the filesystem bullet, which did not mention that the per-user directories hold the encrypted SSH keys -- the reason the volume is still required at all.
…efault
pymongo waits 30 seconds for a reachable server. That is not a useful wait: a
database on the same Docker network either answers in milliseconds or is not
coming. Meanwhile every stalled request holds a waitress thread, so during an
outage a handful of retrying browsers wedges a finite pool rather than getting
fast errors.
SERVER_SELECTION_TIMEOUT_MS = 5000 in data_file_functions, applied to the shared
client and to the Flask-Session client in app.py. The session client matters most
-- it is touched by every single request, including the login page.
Measured with the app already running and MongoDB then killed, which is the
realistic outage:
GET /user_login before: 60.4s after: 10.1s
Two server-selection attempts per request, because Flask-Session reads on the
request and writes on the response, so a request pays the timeout twice.
socketTimeoutMS is deliberately left at its default: capping it would kill
legitimately long-running queries, which is a different problem from an
unreachable server.
Also found while measuring, and worth recording: with SESSION_TYPE=mongodb (the
default) and MongoDB unreachable, app.py cannot be imported at all --
Flask-Session's MongoDBSessionInterface creates the "expiration" TTL index in its
constructor and nothing catches the failure. That is long-standing behaviour, not
new here; verified against the pre-change tree it took 30.8s, and now takes 5.8s.
instance_id's own pymongo.timeout(2.0) stays, and is now explicitly documented as
deliberately tighter than the client bound rather than as a workaround for the
30s default -- that comment would otherwise have become false.
Tests assert the shared client is constructed with the bound, that the bound is
meaningfully below 30s, and that the client is still reused rather than rebuilt
per call. Note they capture _get_mongo_client at import time: conftest's
session-scoped mongo_client fixture replaces that module attribute with a
mongomock lambda, so a late lookup would have tested the stub -- which is exactly
what happened on the first attempt, passing in isolation and failing in the full
suite.
validate_mongodb_connection() passed serverSelectionTimeoutMS=1. One millisecond is shorter than a real connection takes, so this is the opposite failure from the 30s default fixed in the previous commit: a mongod that is up but still starting -- the normal case behind Compose's `depends_on`, which ships without a healthcheck -- could fail the probe and sys.exit(), dropping the container into a restart loop. It now shares SERVER_SELECTION_TIMEOUT_MS, so there is one number to reason about instead of three. Costs at most a few seconds on a genuinely unreachable database, and the probe still exits: verified with an unroutable address, the container exits 1 after ~9s. Verified the case this actually fixes by starting mongo and the app in the same instant: 0 restarts, "MongoDB connection successful", serving. Worth noting from that unreachable run: there was no "MongoDB connection failed!" line, because with SESSION_TYPE=mongodb the app dies at Flask-Session's TTL index creation before it ever reaches this probe (documented in §2 of docs/data-architecture.md). So in the default configuration this probe mostly guards the non-MongoDB-session case. Also corrected a comment in app.py's __main__ that still claimed the shared client sets no serverSelectionTimeoutMS -- true when written one commit ago, false now. Test asserts the probe uses the shared constant and that the constant sits in a sane range (long enough for a starting mongod, short enough not to hang a boot).
The per-user data/<user>/<firewall_name>.conf file existed only to give NAPALM a file path for load_merge_candidate(). It was rewritten before every push, was never a source of truth (MongoDB is), and was never deleted -- files accumulated per firewall name, outlived the configs that produced them, and shipped in every full backup zip and S3 upload. napalm-vyos accepts load_merge_candidate(config=<str>), and internally writes that string to its own NamedTemporaryFile, so both paths converge on identical code before any device I/O. The temp file is not eliminated so much as relocated: 0600 and auto-unlinked in TMPDIR, rather than a predictable, never-deleted path inside the mounted data volume. Adds build_merge_config() to render the command list into that string, and threads it through commit_to_firewall() / get_diffs_from_firewall() as a required third argument. The operational-command action no longer pays for a write it never read. Fixes a latent bug in passing. generate_config() emits "#" banner lines and blank separators into the command list for the human-facing download and snapshot diff. Those were landing in the .conf and being sent to VyOS as literal commands -- on the shipped example config, 58 of 146 elements. VyOS rejects them, but the driver only greps its output for "Set failed" / "Delete failed", so every rejection was swallowed silently. build_merge_config drops them at the push seam rather than in generate_config, which would break the diff view's section headings. Comments and blanks are filtered by joining the list before splitting it, not element by element: elements can hold several lines (the banners are multi-line, and add_extra_items splits the textarea on "\r" alone, so an LF-only POST collapses typed lines into one element). Filtering per element would discard a real set command sitting behind a comment. An all-comments config now renders an empty string, which napalm-vyos treats as a caller error and raises MergeConfigException for -- surfacing as a red "Error in diff" banner. Reachable via the seeded extra-items placeholder, which is three comment lines. Both entry points guard on it before assembling the driver, so there is no SSH connect and no staged key to clean up. sweep_legacy_conf_files() removes the leftovers on the first startup after upgrade. It takes the account list as a parameter rather than globbing data/*/*.conf, which would also delete from stray directories that never belonged to a user, and rather than importing user_store, which imports this module. It runs after the MongoDB gate because it needs that list, and never raises -- housekeeping must not stop startup. Also corrects stale file:line references in both docs that predate this change.
mongo_converter imports pre-1.4.0 on-disk JSON configs and renames each source
file to <name>.old so it is not re-imported. Nothing ever deleted them: they sat
on the data volume indefinitely and, unlike the .key files, shipped in every
full-backup zip and S3 upload.
Rather than a near-duplicate function, this generalises the .conf sweep added in
the previous commit -- the glob/remove/log/never-raise body was already right and
only the pattern differed. sweep_legacy_conf_files is renamed to
sweep_legacy_user_files, since leaving a _conf_ name on something that also
deletes .old would be actively misleading, and the suffixes move into
_LEGACY_USER_FILE_SUFFIXES with the reason recorded per entry.
Two things make deleting .old safe, both checked in the code rather than assumed:
- The rename at mongo_converter.py:62 runs only after write_user_data_file() at
:59 returns, and there is no try/except around that loop, so a failed import
leaves the file as .json to be retried. **A .old file therefore always means
that config is already in MongoDB.**
- Nothing reads .old. list_user_keys filters .key, list_user_backups filters
.zip, mongo_converter looks for .json, and allowed_file() permits only .json
and .key on upload, so a user cannot create one either.
The call moves from after mongo_converter() to between migrate_sqlite_users() and
it. Both halves of that position matter: after the user migration because
list_usernames() needs the accounts in MongoDB (a pre-2.5.0 upgrade would
otherwise return an empty list and sweep nothing), and before the converter
because the converter is what creates .old files -- sweeping afterwards would
delete a file created seconds earlier and make the rename pointless. As placed, a
.old produced by one boot survives until the next, so it is a one-restart safety
net.
json is deliberately absent from the suffix list and the constant says so; the
pre-existing test asserting .json survives the sweep is the load-bearing one, and
mongo_converter still needs those files.
Verified end to end with real containers, seeding a migrated account's directory
with .conf, two .old, a .json, a .key, a backup zip, and a .old in a non-account
directory:
boot 1: removed 3 (.conf + both .old); .json/.key/.zip and the stray .old
survive; mongo_converter then imports to-import.json in the same boot,
leaving to-import.old -- which is what proves the ordering
boot 2: removed 1 (to-import.old); the config is still in MongoDB
boot 3: removed 0
Mutation-checked: globbing "*" instead of the suffix list fails the survival
test, and dropping "old" from the list fails two.
data/<username>/<name>.key was the last durable state on the data volume.
Accounts moved to MongoDB in 2.5.0, the telemetry id followed, generated .conf
files stopped being written and .old leftovers are now swept; keys are what still
made the volume load-bearing.
The encryption design is unchanged and is the part worth protecting:
process_upload() generates a fresh Fernet key, shows it to the user once, and
never persists it. The server holds ciphertext it cannot read, and the user
supplies the Fernet key again at connect time. Only the storage location moves,
so an unauthenticated MongoDB still yields no usable key.
New package/ssh_key_store.py owns the keys collection:
{"_id": "alice/id_rsa", "user": ..., "name": ..., "key": Binary(...), ...}
_id as f"{user}/{name}" gives uniqueness per (user, name) with no index to build,
the same reasoning as user_store. It is unambiguous because usernames pass an
allowlist excluding "/" and key names have been through secure_filename(). user
and name are kept as fields so listing is a projection.
A dedicated collection is required, not just tidier: list_user_files() selects
current configs with {"firewall": {"$exists": False}, "snapshot": {"$exists":
False}}, and a key document has neither field, so keys stored in the user's own
collection would be listed and rendered as firewall configs.
"keys" therefore joins _RESERVED_USERNAMES, and unlike "instance" it also joins
_AUTH_CRITICAL_RESERVED so a legacy account of that name aborts the startup
migration. That user would own the collection holding every other user's key
ciphertext plus the list of whose keys exist -- much closer to the users/sessions
case than to a telemetry UUID. Consequence worth knowing: an install with an
account named "keys" will not boot until it is renamed.
decrypt_file(path, key) becomes decrypt_ssh_key(user, name, fernet_key). NAPALM
and Paramiko both want a filesystem path, so the plaintext still has to touch
disk, but it is now staged with tempfile.mkstemp() **without dir=** -- the system
temp directory rather than the mounted volume, the same relocation 1098dba made
for the merge config. Decryption happens before mkstemp, so a wrong Fernet key
leaves no file behind. The 0600 mode, the caller's finally-block cleanup and the
paramiko-connect-failure cleanup are all unchanged.
data/tmp is no longer written to, but it is still created and wiped at startup on
purpose: on an install upgrading from a version that crashed mid-operation, that
wipe is what removes a stale *plaintext* private key.
migrate_legacy_key_files() adopts pre-2.5.0 files at startup, positioned after
migrate_sqlite_users() because it needs the migrated account list. $setOnInsert,
so a re-run cannot replace a key the user has re-uploaded with one whose Fernet
key they no longer have. The file is renamed to .key.migrated, never deleted --
that ciphertext is the user's only copy. It is deliberately absent from
_LEGACY_USER_FILE_SUFFIXES so last commit's sweep cannot take the rollback copy,
and a test pins that.
Backups: per the decision to let keys ride along, mongo_dump() sweeps the keys
collection into the zip. The on-disk exclusion is widened to
(".key", ".key.migrated") -- endswith(".key") does not match the retained name,
and the live ciphertext now arrives via the dump anyway, so the file would be a
redundant second copy of the same secret.
Verified with a real ed25519 key end to end. Round trip against a live MongoDB:
ciphertext stored as Binary with no plaintext substring, decrypt returns
byte-identical output, the staged file is 0600 and outside data/, paramiko loads
it with the original fingerprint, and a wrong Fernet key raises InvalidToken with
nothing staged. Container upgrade against a volume holding a legacy
id_ed25519.key: adopted with a byte-identical sha256, file renamed, the user's
saved Fernet key still decrypts it from inside the container, list_user_keys shows
it for its owner and nothing for anyone else, restart is a no-op, and a full
backup contains keys.bson and no .key* file.
Not verified: an actual push to a VyOS device, which is the only thing that would
prove NAPALM accepts the relocated key_file. Paramiko loading the same staged
file is the closest available evidence.
Mutation-checked: staging back inside data/ fails the outside-the-volume test,
$set instead of $setOnInsert fails the re-upload test, and creating the temp file
before decrypting fails the nothing-staged-on-bad-key test.
docs/ssh-credential-handling.md is the main one, since it is the document an auditor reads: - §2's code map gains ssh_key_store and loses the stale data_file_functions line numbers for the key paths. - §3's sequence diagram writes the ciphertext to MongoDB rather than to disk, and the auditor facts now cover the collection and document shape, the adoption of pre-2.5.0 files with the rename that is never a delete, and the reserved-name situation including why "keys" aborts startup where "instance" does not. - §3 also states plainly that key material is in full backups as of 2.5.0 -- a deliberate reversal of the previous ".key excluded" posture, defensible only because the Fernet key is never stored server-side. The retained .key.migrated file stays excluded as a redundant second copy. - The temp-key lifecycle section is rewritten for decrypt_ssh_key(): the lookup is by (user, name) so one user cannot reach another's key, the "password" field carries the Fernet key on this path rather than a login password, decryption happens before the file is created so a wrong key stages nothing, and mkstemp is called without dir= so the plaintext lands in the system temp directory instead of the mounted volume. - Threat #7, the secret-residence table and the logging notes follow suit, and the data/tmp entry now explains that the wipe is retained specifically to clear a stale plaintext key left by a pre-2.5.0 crash. docs/data-architecture.md: §1's 2.5.0+ store table and diagram list the keys collection and describe the filesystem as outputs only; §4's downgrade procedure gains the key-file rename a rollback needs, with the consequence that skipping it forces every user to re-upload and take a new Fernet key; §6 says the volume now holds no durable state and no secrets, and records the backup exclusion; §7 notes keys.bson now rides along in every dump and why that is acceptable. CLAUDE.md gains ssh_key_store in the module table and the keys collection in Data Storage. The compose security comments and the chart README now name the SSH keys alongside the credential store, and distinguish the two: the bcrypt hashes are a real offline target, the key ciphertext is not directly usable.
Selecting an SSH key for View Diffs or Run Operational Command failed with
"No stored SSH key named 'ibehr.key' for user 'ibehr'", leaving key-based
authentication completely unusable.
SSH keys moved into MongoDB in 2.5.0 and are addressed by their bare name --
ssh_key_store._document_id() builds _id as "<user>/<name>", and
migrate_legacy_key_files() strips the ".key" suffix when adopting a file. The
push form kept appending ".key" to the value, which was correct before 2.5.0
because the name was used to build the path data/<user>/<name>.key, but now
produces a lookup for "<user>/<name>.key" that can never match. Confirmed
against a real store: get_ciphertext("test22", "ibehr") returns 632 bytes,
get_ciphertext("test22", "ibehr.key") returns None.
This broke both reported actions because copyAuthCredentials() copies the
selected radio's value verbatim into the operational-command form.
The route also strips a trailing ".key" defensively, so a browser still holding
a cached copy of the old form resolves rather than erroring. removesuffix rather
than replace, so a key legitimately named "my.keyring" is not mangled -- the
previous session-caching line used replace(".key", "") and had that flaw.
Adds the coverage whose absence let this ship: nothing asserted the value the
form actually submits. The rendering test fails against the old template.
The admin page told operators "SSH keys are excluded from backups for security". That was true of the on-disk .key files, which the zip walk still skips, but it stopped being true when 2.5.0 moved the ciphertext into MongoDB: mongo_dump() sweeps list_collection_names(), so keys.bson is in every archive and every S3 upload. Verified on this install -- the newest dump directory contains keys.bson. Replaced with what actually happens: the archive includes the MongoDB dump and the encrypted keys, and is safe to hold only because the Fernet passphrase is never stored server-side. An operator deciding where to put a backup needs that distinction, so the claim is asserted in a test rather than left to drift again. Brings docs/data-architecture.md up to date with everything since it was last touched: - The version note said two stores moved in 2.5.0; SSH keys were the third. - Section 8 documented two startup migrations and showed a two-line __main__ block. There are now four steps, and the ordering constraints are real in both directions -- migrate_sqlite_users() must precede anything driven by list_usernames(), and sweep_legacy_user_files() must precede mongo_converter() because the converter is what creates the .old files it deletes. Both are now stated, along with why none of it can move into initialize_data_dir(). - Added 8.2 for migrate_legacy_key_files(), which was undocumented, including the $setOnInsert reasoning (a re-run with $set would overwrite a re-uploaded key with ciphertext whose passphrase the user no longer has) and the consequence that an account missing from the users collection keeps its keys unadopted. Renumbered the JSON converter to 8.3 to keep execution order, and updated the two cross-references. - Section 6 claimed data/tmp/ "stages decrypted SSH keys per-operation". Nothing has written there since 2.5.0; keys stage in the system temp dir, as the same section says three bullets later. - Section 7 omitted *.key.migrated from the exclusion list, and a spliced-in sentence had left "That is intentional" attached to the wrong clause. Also records that each zip re-archives every historical dump, so archives grow superlinearly, not just in count. - Noted that only the accounts collection is renameable, and why the escape hatch exists at all. Corrected 13 stale file:line references, all drift from earlier commits moving code within data_file_functions.py. Audited mechanically by resolving every function-anchored reference against the current source rather than by eye.
The System Information card on the admin settings page carried user_count and config_count placeholders that no route ever passed, so both always rendered "N/A", and their "if x else" guards would have shown "N/A" for a real zero anyway. Add user_store.count_users() and data_file_functions.gather_instance_stats(), and pass the result to the template as stats. Counts are instance-wide: accounts (with the disabled subset called out), firewall configurations and snapshots across every account. The counts are driven by the account list rather than by listing collections, so the users, sessions, keys and instance collections are never mistaken for user data, and a disabled account's configurations are still counted because accounts are disabled and never deleted. Read-only, so upgrading an existing install only adds three numbers to the page. FW-GUI has no admin role, so any logged-in user sees the totals -- the same page already offers a full MongoDB dump of every account, so this exposes nothing new.
A full backup previously happened only when someone clicked the button on the Admin Settings page. Since 3.0.0 that archive is also the only copy of the users collection (bcrypt hashes) and the keys collection (encrypted SSH keys), so the cost of not having a recent one went up. The schedule runs from a daemon thread started in __main__, but the thread is not what makes it safe: each due run is claimed with a single atomic find_one_and_update, so the dev reloader's two processes and any extra Helm replica cannot all back up. A claim deliberately does not advance next_run -- _release_run() does that after the attempt finishes, on failure too -- so a process killed mid-zip has its window retried once the lease expires instead of silently skipping the week. Catch-up coalesces: an instance down for three weeks backs up once, then returns to cadence. All configuration lives on the backup_schedule document in the existing instance collection, edited on the Admin Settings page. No environment variables: one source of truth needs no precedence rule between a variable and a stored value, and a setting in the database survives a container replacement and takes effect without a restart. The document shares the instance collection with the telemetry id because a collection name is also a username, so a new collection would mean reserving another one. Retention ships with this rather than after it. Nothing in the app had ever deleted a dump or an archive, and every zip re-archives all retained dumps, so size grew quadratically -- one real install reached 944 MB of archives across 119 dump directories. Automating backups without a bound would have added gigabytes a year. Dumps are pruned before the dump runs, which is what actually bounds the new archive; archives are pruned after the zip is in place, so a failed backup does not leave the set one short. Pruning sorts by mtime, not by the timestamp in the name: those names come from naive local time, so at the end of DST a newer directory sorts earlier and pruning by name would delete the newest backup. S3 is never pruned. Offsite copies exist to survive mistakes made on this host, and the key has no instance identifier, so two instances sharing a bucket would delete each other's archives. Operators should use a lifecycle rule. A new schedule is seeded disabled, so an upgrade changes nothing but the Admin page. Because enabling it on an install that has never pruned can delete hundreds of files hours later in a background thread, saving the form reports the exact counts first, and "Backups to keep" of 0 schedules backups with no deletion. perform_full_backup() is extracted as the request-context-free core, since flash() and session are unavailable on a thread; create_backup() stays the wrapper that turns the outcome into flash messages, so the existing route and its tests are unchanged. The zip is now staged in data/tmp and moved with os.replace, so a process killed mid-zip cannot leave a truncated archive that list_full_backups() would report as real. Known gap: /admin_settings is still login-only, so any user can change these settings, and enabling retention lets them delete other users' backup history. That is close to pre-existing -- the same user can already trigger a full backup of everyone's data -- but it wants a real admin role.
This release outgrew a patch number. It moves three stores into MongoDB (user accounts, encrypted SSH keys, the telemetry instance id), stops writing durable state to the filesystem, removes the in-app backup download, and adds a scheduled backup that deletes old archives. Several of those are one-way migrations on first boot, so a major number communicates the upgrade better than 2.5.0 did. Every 2.5.0 reference becomes 3.0.0, including the "pre-2.5.0" ones. That is a correction rather than a rewrite of history: 2.5.0 was never tagged or published (the last release is v2.4.3), so those notes were always describing this unreleased work. Genuinely historical references to 1.4.0 and earlier are left alone. pyproject.toml is the source of truth that scripts/build.sh reads; .version and the fw-gui entry in uv.lock are updated to match so a build does not produce a diff.
CI sets FWGUI_INSTANCE_ID (.github/workflows/ci.yml) to tag any telemetry a run might emit. That variable short-circuits get_or_create_instance_id() before MongoDB is touched, so the test asserting that the telemetry id and the backup schedule coexist in one collection was really asserting the value of the variable -- and failed on the difference. It passed locally only because the variable is unset there. Clear it with monkeypatch, matching the fixture in test_instance_id.py, so the test exercises the stored document it is about. The cached-id reset also moves to monkeypatch.setattr. It used to be a plain assignment after the assertions, which meant a failure skipped the reset and leaked a cached id into every later test in the session.
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
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.
Bumps to a major version because this release moves three stores into MongoDB and
performs one-way migrations on first boot. An install cannot be rolled back by
simply redeploying the previous image; §8 of
docs/data-architecture.mddocumentsthe rollback path that does exist.
What changes
Authentication moves off SQLite. Accounts live in a MongoDB
userscollection, one document per account,
_id= username. A one-shot startupmigration imports
data/database/auth.dband retains it asauth.db.migratedforrollback only. Because a username is also a collection name,
users,sessions,instanceandkeysare rejected as usernames.SSH keys move into MongoDB as Fernet ciphertext in a
keyscollection. TheFernet key is generated at upload, shown once and never stored, so the server
holds a blob it cannot read. Decrypted keys are staged in the system temp
directory, never under
data/.The telemetry instance id moves into MongoDB, adopting the pre-3.0.0
instance.idfile on upgrade and retiring it asinstance.id.migrated.The filesystem holds outputs only — logs, backups and Mongo dumps — plus the
retained pre-3.0.0 artifacts. Generated
.confcommand files are no longerwritten (NAPALM takes the commands as a string), and leftovers are swept at
startup.
POST /downloadis removed. It read any file underdata/for any logged-inuser, including other users' backups.
Automatic weekly backups, off until enabled. A daemon thread polls a schedule
document, but the thread is not what makes it safe: each due run is claimed with a
single atomic
find_one_and_update, so the dev reloader's two processes and anyextra replica cannot all back up. The claim deliberately does not advance
next_run— the release step does, on failure too — so a process killed mid-ziphas its window retried once the lease expires rather than silently skipping the
week. A missed window is caught up once, not once per week missed.
All of its configuration lives on the
backup_scheduledocument and is edited onthe Admin Settings page. There are no environment variables: one source of truth
needs no precedence rule between a variable and a stored value, and a stored
setting survives a container replacement and applies without a restart.
Retention ships with the scheduler rather than after it. Nothing in the app
had ever deleted a dump or an archive, and every zip re-archives all retained
dumps, so size grew quadratically — one real install reached 944 MB of archives
across 119 dump directories. Dumps are pruned before the dump runs, which is
what actually bounds the new archive; archives are pruned after the zip is in
place, so a failed backup does not leave the set one short. Pruning sorts by
mtime, not by the timestamp in the name: those names come from naive local time,
so at the end of DST a newer directory sorts earlier and pruning by name would
delete the newest backup. S3 is never pruned — offsite copies exist to survive
mistakes made on this host, and the key carries no instance identifier, so two
instances sharing a bucket would delete each other's archives.
Upgrade notes
the Admin Settings page.
files. Saving the form therefore reports the exact counts first, and setting
Backups to keep to
0schedules backups with no deletion at all.auth.db.migrated,instance.id.migratedand*.key.migratedare retained onthe volume for rollback and are deliberately excluded from backup archives — the
live copy of each secret already travels in the Mongo dump.
Verification
pytest— 744 passing, including a two-thread contention test for the run claimand one asserting that pruning follows mtime rather than the name.
bandit -c pyproject.toml -r .— no findings. The scheduler was also exercisedend to end against a real MongoDB in a scratch directory: seeding, saving settings
through the form path, surviving a restart, running when due, pruning to the
configured retention, honouring the saved day/hour on the next slot, coalescing a
three-week outage into one run, and doing nothing while disabled.
Known gap
/admin_settingsis still@login_requiredonly, so any logged-in user can changethese settings, and enabling retention lets them delete other users' backup
history. That is close to pre-existing — the same user can already trigger a full
backup containing everyone's data — but it wants a real admin role. Worth a
follow-up issue rather than a blocker here.
Note on commit messages
Twelve commits on this branch still say 2.5.0 in their messages. That was the
branch name while the work was authored; 2.5.0 was never tagged or published, and
every reference in the shipped code and documentation now says 3.0.0. The messages
were left as-is rather than rewritten, since those commits were already pushed.