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

Add drill trace option in dnssec boefje #2979

Merged
merged 4 commits into from
May 27, 2024
Merged

Conversation

dekkers
Copy link
Contributor

@dekkers dekkers commented May 22, 2024

Changes

This fixes the linked issue that findings were shown while the DNSSEC check was valid. The trace option is added to the drill command so drill will trace from the root servers down instead of relying on a recursive DNS server. The added value of using this option is that it outputs the keys it find and those will be included in the raw file.

When comparing the results of the old code with the new code I also saw that old code incorrectly reported a self signed signature (which can be tested with dnssec-failed.org) as no DNSSEC instead of invalid DNSSEC

Issue link

Closes #2566

QA

I used the following hostnames to test the changes myself:

  • No DNSSEC: google.com
  • Invalid DNSSEC: ok.bogussig.ok.bad-dnssec.wb.sidnlabs.nl, signotincepted.bad-dnssec.wb.sidnlabs.nl, ok.ok.sigexpired.bad-dnssec.wb.sidnlabs.nl, unknownalgorithm.ok.ok.bad-dnssec.wb.sidnlabs.nl, dnssec-failed.org
  • Valid DNSSEC: ok.ok.ok.bad-dnssec.wb.sidnlabs.nl

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@dekkers dekkers requested a review from a team as a code owner May 22, 2024 21:25
"Unknown cryptographic algorithm",
"DNSSEC signature has expired",
]
for result_line in reversed(result.splitlines()):
Copy link
Contributor

Choose a reason for hiding this comment

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

Document why we reverse the lines before iterating. I caused me the question the if tree below as I did not see how it would handle the Key lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment that explains it.

Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 I appreciate the clear explanation, info for QA, new tests and the extra bug being fixed.

@stephanie0x00
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.
  • I checked the logs for errors and/or warnings and made issues where necessary

What works:

Looks good! The domain from the ticket also works as expected. Have verified cases for: no dnssec, invalid dnssec and valid dnssec. All cases seem to work as expected.

What doesn't work:

n/a

Bug or feature?:

n/a

@underdarknl underdarknl merged commit 807839a into main May 27, 2024
9 checks passed
@underdarknl underdarknl deleted the use-trace-in-dnssec-boefje branch May 27, 2024 11:31
jpbruinsslot added a commit that referenced this pull request Jun 11, 2024
* main: (78 commits)
  Translations update from Hosted Weblate (#3048)
  Translations update from Hosted Weblate (#3018)
  Fix empty consumes of boefjes will trigger tasks in scheduler (#3017)
  Fixes text in secondary menu on scan profile detail page (#3035)
  chore: Resolves css-issues found by sonarcloud (#3034)
  Add raw AuthToken SQL migration (#3009)
  Translations update from Hosted Weblate (#3012)
  Rewrite xtdb-cli.py with "click" (#2957)
  Phase out the Repository model from the KATalogus (#2984)
  Fix merge conflicts in weblate (#3007)
  Translations update from Hosted Weblate (#2996)
  Reports: Fix select all OOIs (#2909)
  Adding IPv6 support to documentation for Docker setups (#2813)
  Translations update from Hosted Weblate (#2930)
  User documentation for reports (#2898)
  Fix task api status code response for malformed id in the scheduler (#2953)
  Add drill trace option in dnssec boefje (#2979)
  Updated packages (#2972)
  Update granian and remove workaround for fixed bug (#2980)
  Fix typing in boefjes/normalizers (#2933)
  ...
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.

DNSSEC boefje shows finding while the DNSSEC chain is valid
4 participants