Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lmdb] Ignore unreachable TSIG keys in getTSIGKeys #15004

Merged
merged 1 commit into from
Jan 13, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions modules/lmdbbackend/lmdbbackend.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2668,8 +2668,34 @@ bool LMDBBackend::getTSIGKeys(std::vector<struct TSIGKey>& keys)
auto txn = d_ttsig->getROTransaction();

keys.clear();
for (auto iter = txn.begin(); iter != txn.end(); ++iter) {
keys.push_back(*iter);
// In a perfect world, we would simply iterate over txn and add every
// item to the returned vector:
// for (auto iter = txn.begin(); iter != txn.end(); ++iter) {
// keys.push_back(*iter);
// }
// But databases converted from older (< 5) schemas _may_ have multiple
// entries for the same TSIG key name and algorithm, something which is not
// allowed in the v5 database schema. These extra entries will not be found
// by get_multi<> during regular operations, and would only appear in the
// results of this method.
// In order to prevent this, we first only gather the list of key names, and
// in a second step, query for them using a similar logic as getTSIGKey().
// Unfortunately, there does not seem to be a way to know if the database had
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while I hope we never have an incident like this again, this makes me wonder if we should have a way to know. Also, we could try to add a cleanup for this to the v6 migration once we get to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. in fact I think we could add a cleanup command to pdnsutil backend-cmd lmdb .. today if we wanted to

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be an interesting task to get your hands even more dirty in LMDB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's something I'll tinker with eventually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this fix I think you can do this today with b2b migrate. Create a new clean lmdb database form a dirty one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

// been created using the v5 schema (not converted), in which case we could
// use the above, simpler logic.
std::unordered_set<DNSName> keynames;
for (const auto& iter : txn) {
keynames.insert(iter.name);
}
for (const auto& iter : keynames) {
LmdbIdVec ids;
txn.get_multi<0>(iter, ids);
for (auto key_id : ids) {
TSIGKey key;
if (txn.get(key_id, key)) {
keys.push_back(key);
}
}
}
return true;
}
Expand Down
Loading