Skip to content
Snippets Groups Projects

DNSSEC - NSEC/NSEC3 chain fix

Merged Ghost User requested to merge chain-fix into master

NSEC/NSEC3 chain fix. Takes 9 seconds for testing zone (a big one), while master takes 4 minutes to fix it. 'Nuff said.

Merge request reports

Merged by avatar (Apr 18, 2025 9:30am UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 855 knot_node_t *parent = knot_zone_contents_get_node(contents, cutted);
    856 assert(parent || node == contents->apex);
    857 bool parent_empty = parent && parent->rrset_count == 0;
    858 while (parent_empty) {
    859 knot_node_t *empty_node = NULL;
    860 int ret = knot_zone_tree_remove(contents->nodes, parent->owner,
    861 &empty_node);
    862 if (ret != KNOT_EOK) {
    863 return ret;
    864 }
    865 assert(empty_node && empty_node->rrset_count == 0);
    866 cutted = knot_wire_next_label(cutted, NULL);
    867 parent = knot_zone_contents_get_node(contents, cutted);
    868 knot_node_free(&empty_node);
    869 parent_empty = parent && parent->rrset_count == 0;
    870 }
    • Author Contributor

      This whole block of code is wrong. Nodes are marked empty in xfrin_mark_empty(), called for every node in the tree, traversed from last to first node (at least that's what should be done, maybe the traversing function zone_contents_tree_apply_inorder_reverse() does not work as expected), and their parents' child counts are adjusted so that when the parent is processed, it is already marked as having no children, if all were marked empty before. Thus all relevant empty non-terminals are marked as empty as well and should be deleted later in the xfrin_remove_empty_nodes() function. If you delete the node here, there may be an invalid read when that function tries to remove it.

    • Author Contributor

      xfrin_remove_empty_nodes() does not work, most probably due to wrong trie iteration: Here's the order in which the function gets the nodes, after the node a.b.c.d.e.f.test03.co.uk. is removed:

      co.uk. -> noop
      f.test03.co.uk. -> noop
      d.e.f.test03.co.uk. -> children count is not 0, noop
      c.d.e.f.test03.co.uk. -> children count is not 0, noop
      a.b.c.d.e.f.test03.co.uk. -> rr_count == 0 and children count == 0, mark empty, adjust b.c.d ...
      e.f.test03.co.uk. -> noop
      test03.co.uk. -> noop
      b.c.d.e.f.test03.co.uk. -> mark as empty, because of a.b.c. ...

      Not quite what we wanted, because it only deletes a.b.c.d.e.f.g.test03.co.uk. and b.c.d.... We can either create a new walk for trie @mvavrusa ?, or use the modified code and remove the xfrin_remove_empty_nodes() function @lslovak ? I don't see any issues with marking empty nodes during DDNS and deleting their empty parents if needed, but I'm probably missing something.

    • Author Contributor

      FYI: fixed by creating new function, replacement for xfrin_mark_empty() that is not dependent on any specific tree walk.

    • :white_check_mark: xfrin_mark_empty() makes sense

  • Author Contributor

    src/libknot/dnssec/zone-nsec.c not reviewed.

    Postponing this to 1.4.x, there is a lot of new code and a lot of changes, which may potentially lead to a lot of problems. This needs two independent reviews and a lot of testing.

  • mentioned in issue #119 (closed)

  • Ghost User mentioned in merge request !129 (merged)

    mentioned in merge request !129 (merged)

  • Ghost User mentioned in merge request !131 (closed)

    mentioned in merge request !131 (closed)

  • 216 216 knot_update_serial_t soa_up,
    217 217 uint32_t *used_lifetime,
    218 218 uint32_t *used_refresh,
    219 uint32_t new_serial)
    219 uint32_t new_serial,
    220 hattrie_t **sorted_changes)
  • 1177 1270 /*!
    1178 1271 * \brief Sign changeset created by DDNS or zone-diff.
    1179 1272 */
    1180 int knot_zone_sign_changeset(const knot_zone_contents_t *zone,
    1273 int knot_zone_sign_changeset(const knot_zone_t *zone,
    1181 1274 const knot_changeset_t *in_ch,
    1182 1275 knot_changeset_t *out_ch,
    1276 hattrie_t **sorted_changes,
  • 1188 1282 }
    1189 1283
    1190 1284 // Create args for wrapper function - hattrie for duplicate sigs
    1191 changeset_signing_data_t args = { .zone = zone, .zone_keys = zone_keys,
    1285 changeset_signing_data_t args = { .zone = zone->contents,
    1286 .zone_keys = zone_keys,
    1192 1287 .policy = policy,
    1193 1288 .changeset = out_ch,
    1194 .signed_table = hattrie_create()};
    1289 .signed_tree = hattrie_create()};
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading