Skip to content

Allow asn_lookup --update-database to create a missing database file - #2724

Open
cristianchiriac wants to merge 1 commit into
certtools:developfrom
cristianchiriac:fix/asn-lookup-missing-database-file
Open

cristianchiriac wants to merge 1 commit into
certtools:developfrom
cristianchiriac:fix/asn-lookup-missing-database-file

Conversation

@cristianchiriac

Copy link
Copy Markdown

Fixes #2689.

Bug

ASNLookupExpertBot.update_database() rejects the database file whenever it doesn't already exist:

for database_path in set(bots.values()):
    if not Path(database_path).is_file():
        raise ValueError('Database file does not exist or is not a file.')
    elif not os.access(database_path, os.W_OK):
        raise ValueError('Database file is not writeable.')

But the actual write further down (pyasn.mrtx.dump_prefixes_to_file) creates the file fresh, and the parent directory is already created if needed (database_dir.mkdir(parents=True, exist_ok=True), right before the write). So this check blocks the documented first-time setup: as reported, the file has to be pre-created with "whatever content" before --update-database will even attempt to run.

Sibling expert bots with the same "download and cache a database" pattern (domain_suffix, domain_valid, maxmind_geoip, recordedfuture_iprisk, tor_nodes) don't have this pre-existence check at all — asn_lookup is the outlier.

Fix

Only reject a path that already exists and isn't a plain file, or an existing file that isn't writeable. A missing file is left alone — it's created by the update itself.

Test plan

  • Added TestASNLookupUpdateDatabase with three cases: a missing database file is now allowed through (reaches the mocked network step instead of raising), an existing unwritable file is still rejected, and an existing directory at the database path is still rejected.
  • Verified the core existence/writability check logic in isolation. I don't have a Linux/WSL environment set up here to run the full suite (intelmq.lib.utils imports grp and fcntl unconditionally, so the package doesn't import at all on native Windows) — CI should confirm the added tests pass.

Fixes certtools#2689. update_database() rejected the database file whenever it
didn't already exist ('Database file does not exist or is not a
file.'), even though the actual write further down
(pyasn.mrtx.dump_prefixes_to_file) creates the file fresh and already
creates the parent directory if needed. This blocked the documented
first-time setup: the file has to be pre-created with 'whatever
content' before the update would even attempt to run.

Sibling expert bots with the same 'download and cache a database'
pattern (domain_suffix, domain_valid, maxmind_geoip,
recordedfuture_iprisk, tor_nodes) don't have this pre-existence check
at all.

Only reject an existing path that isn't a plain file, or an existing
file that isn't writeable; a missing file is left to be created by the
update itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

asn expert database update fails if file does not already exist

1 participant