Skip to content
Snippets Groups Projects

Draft: resolver,api: tweak logic for AD bit and set AA if all RRsets have rank KR_RANK_AUTH.

Open Draft: resolver,api: tweak logic for AD bit and set AA if all RRsets have rank KR_RANK_AUTH.
menakite/knot-resolver:tweak-ad-set-aa into master
3 unresolved threads
Open menakite requested to merge menakite/knot-resolver:tweak-ad-set-aa into master
3 unresolved threads

AD: my understanding is that RFC 4035, section 3.2.3, mandates non-empty Answer or Authority section to set the AD bit in responses.

AA: by using logic similar to that for the AD bit, AA bit in responses is set in resolver's answer_finalize if all RRsets are non-secure (so no AD bit) and have rank KR_RANK_AUTH.

KR_RANK_AUTH is safe enough because in current code it is never set alone.

If a RRset has only KR_RANK_AUTH, it indicates that an authoritative response is wanted.

Can add and use another flag if desired, but I'd prefer not to.

Merge request reports

Members who can merge are allowed to add commits.
Approval is optional
Merge blocked: 3 checks failed
Merge request must not be draft.
Unresolved discussions must be resolved.
Merge request must be rebased, because a fast-forward merge is not possible.

Merge details

  • The source branch is 394 commits behind the target branch.
  • 8 commits and 1 merge commit will be added to master.
  • Source branch will be deleted.

  • Closes issues #913 and #914

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • Vladimír Čunát
      Vladimír Čunát @vcunat ·
      Owner

      I completely fail to see why the _AUTH rank should imply AA=1 flag, nor the algorithm proposed.

      AA=1 means that the server sending the answer is an authoritative server for the first record in ANSWER section.

      Knot Resolver can do this for data generated locally (e.g. the local-data: part of config). For others it doesn't make sense to me.

    • menakite
      menakite @menakite ·
      Author Contributor

      Thanks for your review. You made me think and I've reworked almost everything.

      Algorithm: it was a deliberate choice for it to be extremely defensive and check everything everywhere. But you are right, it was exaggerated.

      Flag: I choose _AUTH because I noticed it is always paired with another flag, so felt like setting only _AUTH, with no other flag, would be safe. In my tests I didn't get AA for anything that wasn't local data, but I may have missed some case. Btw, I have added a different flag.

      I'd appreciate if you could re-review the fixup commits.

    • Please register or sign in to reply
  • Vladimír Čunát
    Vladimír Čunát @vcunat started a thread on an outdated change in commit 3c4f5efd
lib/resolve.c
375 375 VERBOSE_MSG(last, "insecure because of opt-out\n");
376 376 secure = false; /* the last answer is insecure due to opt-out */
377 377 }
378 if (!request->answ_selected.len && !request->auth_selected.len) {
379 /* RFC 4035, section 3.2.3 mandates non-empty Answer or Authority. */
380 secure = false;
381 }
  • menakite added 40 commits

    added 40 commits

    • 0f062dbc...ae7b3e6c - 33 commits from branch knot:master
    • 4372e7be - resolver: don't set AD if both Answer and Authority are empty.
    • fabfdc03 - resolver,api: set AA bit if all RRsets have (only) rank KR_RANK_AUTH.
    • 14c1e33a - resolver: update documentation for flag KR_RANK_AUTH.
    • afaf6776 - fixup! resolver,api: set AA bit if all RRsets have (only) rank KR_RANK_AUTH.
    • 30fa447d - fixup! resolver,api: set AA bit if all RRsets have (only) rank KR_RANK_AUTH.
    • d98e1414 - fixup! resolver,api: set AA bit if all RRsets have (only) rank KR_RANK_AUTH.
    • 3b812768 - fixup! resolver: update documentation for flag KR_RANK_AUTH.

    Compare with previous version

    Toggle commit list
  • menakite marked this merge request as draft from menakite/knot-resolver@afaf67768a32e98df2b975e617970f2637aae12b

    marked this merge request as draft from menakite/knot-resolver@afaf67768a32e98df2b975e617970f2637aae12b

  • menakite
    menakite @menakite started a thread on commit 30fa447d
    • lib/resolve.c
    65 65
    66 66 bool kr_rank_test(uint8_t rank, uint8_t kr_flag)
    67 67 {
    68 68 if (kr_fails_assert(kr_rank_check(rank) && kr_rank_check(kr_flag)))
    • Comment on lines +50 to 68
      menakite
      menakite @menakite ·
      Author Contributor

      This is just to avoid the assert in line 68.

      Edited by menakite
    • Vladimír Čunát
      Vladimír Čunát @vcunat ·
      Owner

      The thing is a bitmap. So 33 already meant _SECURE + _OMIT.

    • Vladimír Čunát
      Vladimír Čunát @vcunat ·
      Owner

      There's a comment that we can't add more bits than we have now

      /* @note Rank must not exceed 6 bits */

      I think this comment doesn't hold anymore, but it's not easy to be sure, as rank is handled on many places.

      Edited by Vladimír Čunát
    • Vladimír Čunát
      Vladimír Čunát @vcunat ·
      Owner

      Still, the combination of _SECURE + _OMIT even does make sense to me for this use case. It basically implies that we did no validation but consider the RR secure, i.e. that exactly makes sense for locally generated data.

      So, we could keep 33, but some rank-testing code would need tweaking.

    • Please register or sign in to reply
  • menakite added 1 commit

    added 1 commit

    • 7444519d - fixup! resolver,api: set AA bit if all RRsets have (only) rank KR_RANK_AUTH.

    Compare with previous version

    • Please register or sign in to reply