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 TLSA and RRSIG support #71

Merged
merged 4 commits into from
Sep 12, 2020
Merged

Add TLSA and RRSIG support #71

merged 4 commits into from
Sep 12, 2020

Conversation

DutchDestroyer
Copy link
Contributor

When I started workig on this issue, I wasn't aware of the other PR that was already created regarding support for TLSA.
I've also added RRSIG support though, so the data that can be displayed on a TLSA request is the same as when done with dig.

I tried to adopt the code styll as much as possible but also added some new methods to make the code around the RRSIG better readable. Also added two unit tests, one for the RRSIG and one for the TLSA GetRecord methods

Please let me know what you think of this.

Best,

Mark

@MichaCo
Copy link
Owner

MichaCo commented May 13, 2020

Sorry for the delay, but I only have very limited time right now to work on this...

First of all, thanks for the contribution! This is much appreciated.

I currently have to fix some minor things and release 1.3.2.
After that, I'll review this PR and we can work on getting it in.

@DutchDestroyer
Copy link
Contributor Author

Hi,
Do you already have an idea when you will be able to review this MR?

@MichaCo MichaCo merged commit 124a546 into MichaCo:dev Sep 12, 2020
@MichaCo
Copy link
Owner

MichaCo commented Sep 12, 2020

Hey,
I just reviewed all 3 PRs adding TLSA records.
I decided to merge yours and then do more adjustments as a lot of details are not what I want in the library.
Some property names and conversions to strings for example.
But don't worry, I'll just make the changes after merging this and let you know so you can see what I meant ;)

One thing for example is your date format extension. You are trying to format a Unix timestamp.
There are already build in methods in DateTimeOffset to get an instance from Unix seconds.
Your format also doesn't format month and days properly if the value has only one digit.

Anyways, thanks for the contribution!

@MichaCo
Copy link
Owner

MichaCo commented Sep 12, 2020

See #86 for my adjustments

rj2Skipper pushed a commit to rj2Skipper/DnsClient.NET that referenced this pull request Sep 15, 2020
* 'dev' of https://github.com/MichaCo/DnsClient.NET:
  More DNSSEC resource records (MichaCo#87)
  Some changes and cleanup to the TLSA and RRSig records (MichaCo#86)
  Add TLSA and RRSIG support (MichaCo#71)

# Conflicts:
#	src/DnsClient/DnsRecordFactory.cs
#	src/DnsClient/QueryType.cs
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.

3 participants