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

Update DNS report #2413

Merged
merged 9 commits into from
Feb 7, 2024
Merged

Update DNS report #2413

merged 9 commits into from
Feb 7, 2024

Conversation

madelondohmen
Copy link
Contributor

Changes

This PR updates the DNS report in the Generate Report flow to match the new design.

The new design can be seen here:
https://www.figma.com/file/QRL8085Z827bNdO0bIDdFL/KAT-%7C-Design-%7C-For-review?type=design&node-id=2305-3142&mode=design

Changes:

  • The table has been changed ("Record - Name - TLL - Data" instead of "Record - Value - Found By")
  • "IP address lookup" chapter has been removed (and is now shown as an "A" record in the table)
  • Disclaimer added (because not all the DNSRecords are parsed in OpenKAT)

Extra info

  1. The "Security measures" chapter hasn't been deleted as in the design. This chapter should stay and was forgotten about in the new design.

  2. For now, only the DNS report has been modified for the Generate Report flow. The Figma design shows the Aggregate Report flow, which doesn't exist yet (should be build firts).

Issue link

Closes #2406

Demo

afbeelding


Code Checklist

  • All the commits in this PR are properly PGP-signed and verified.
  • This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
  • I have written unit tests for the changes or fixes I made.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

  • I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
  • I have made corresponding changes to the documentation, if necessary.
  • I have included comments in the code to elaborate on what is not self-evident from the code itself, including references to issues and discussions online, or implicit behavior of an interface.

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.

@madelondohmen madelondohmen requested a review from a team as a code owner February 1, 2024 08:12
@madelondohmen madelondohmen self-assigned this Feb 1, 2024
ammar92
ammar92 previously approved these changes Feb 2, 2024
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. Just a side note: I thought we generally capitalize H1, H2, and H3 headings, but I'm not sure.

@underdarknl
Copy link
Contributor

I dont see a CAA record in that table, But I do see that it somehow has that security measure, this seems illogical?
Mispo.es does not actually have a CAA record. So maybe the check logic there is broken?

I'd also like to see the various DNS related findings listed. Eg, no ipv6 on the nameservers, or not enough nameservers.

@stephanie0x00
Copy link
Contributor

stephanie0x00 commented Feb 2, 2024

After reviewing the report it would be nice to change the following things.

  • Sorting of the records: the A records is generally shown before the AAAA record. This could maybe be fixed with this PR.

  • A text should be added below the header "Records found", a text suggestion is: "The table below gives an overview of the DNS records that were found for the abovementioned DNSzone." The 'note' on missing/implemented records can be after that.

  • The other reports should also include similar texts above the tables. I'll create a separate ticket for this which will include all the texts for each report. See ticket Human readable primary key in report sections #2421

  • For readability of the report we should change the title of the headers in the report from "DNS Report for Hostname|internet|mispo.es" to just the domain "mispo.es". I've discussed this with @noamblitz and this requires a new ticket, as this also needs to be fixed in other reports. With this change we can then also make a small alteration in the texts above the table to also include the domain to "The table below gives an overview of the DNS records that were found for the DNSzone ." See ticket Human readable primary key in report sections #2421

  • The introduction text should be updated as it doesn't match the contents of the report. I've discussed this with @noamblitz and this should become a universal introduction as this introduction is identical to all reports. See ticket Update the introduction text of the reports #2422

@stephanie0x00
Copy link
Contributor

I dont see a CAA record in that table, But I do see that it somehow has that security measure, this seems illogical? Mispo.es does not actually have a CAA record. So maybe the check logic there is broken?

I'd also like to see the various DNS related findings listed. Eg, no ipv6 on the nameservers, or not enough nameservers.

CAA are actually shown, but mispo.es doesn't have them. In the QA I did see it for one of the domains I tested.

@stephanie0x00
Copy link
Contributor

Small additions are fixed. Ready for merge.

ammar92
ammar92 previously approved these changes Feb 6, 2024
@madelondohmen
Copy link
Contributor Author

I dont see a CAA record in that table, But I do see that it somehow has that security measure, this seems illogical? Mispo.es does not actually have a CAA record. So maybe the check logic there is broken?

I'd also like to see the various DNS related findings listed. Eg, no ipv6 on the nameservers, or not enough nameservers.

Restarting the bits solved the missing CAA Record. It was a problem on my end.

@underdarknl underdarknl merged commit 267a8fd into main Feb 7, 2024
12 checks passed
@underdarknl underdarknl deleted the feature/add-dns-generate-report branch February 7, 2024 09:21
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.

Generate Report - Update DNS report
4 participants