Fix other methods in Zonemaster::LDNS::RR::NSEC3 and NSEC3PARAM#178
Merged
marc-vanderwal merged 3 commits intoDec 4, 2023
Merged
Conversation
Contributor
Author
|
For the time being, I’m marking this PR as a draft because I’d like #177 merged first. |
matsduf
reviewed
Nov 20, 2023
Some pieces of data in NSEC3 resource records are encoded using the base32-hex encoding as specified in RFC 4648 § 7. Yet the unit tests covering NSEC3 resource record objects do not compare the binary representations of these pieces of data using the base32-hex encoding, relying instead on base64. This impairs the legibility of those unit tests. This commit adds the MIME::Base32 library as a test-time dependency. It isn’t needed at compile-time or run-time.
The next_owner() method in Zonemaster::LDNS::RR::NSEC3 did not work as documented. Its return value was not the value of the next hashed owner name field, but the same value with the length byte prepended. This choice does not make the function as useful as one may hope. This commit ensures that the next_owner() method returns the next hashed owner name field, and only that. The unit test for this function compares base64-encoded strings. This hampers legibility because conversions between base32 and base64 are not trivial and only a very astute reader would notice that the next_owner() method had this kind of catch. As a bonus, add a useful tip in the method’s documentation.
The salt() method in Zonemaster::LDNS::RR::NSEC3PARAM did not work as documented. Like two of those accessor methods in NSEC3 I’ve fixed previously, its return value is a byte string that starts with an extraneous length byte, which isn’t necessary. The unit test for this function compares base64-encoded strings. This hampers legibility because conversions between hexadecimal strings and base64 strings are not trivial and only a very astute reader would notice that the salt() method also had a similar problem. Also improve the documentation and the unit test coverage for that method.
f37a23e to
7dfef87
Compare
marc-vanderwal
added a commit
to marc-vanderwal/zonemaster
that referenced
this pull request
Nov 22, 2023
Pull request zonemaster/zonemaster-ldns#178 adds a test-time dependency on MIME::Base32. This commit documents the change. This package should be installed manually in build environments. The module does not seem to be packaged in Red Hat-based Linux distributions. On those OSes, the MIME::Base32 has to be installed through CPAN.
Contributor
|
Conclusion from meeting today that installation instructions are to be updated to include the new dependency. |
Contributor
Author
Yes, I opened a PR to that effect: zonemaster/zonemaster#1223 |
tgreenx
approved these changes
Nov 23, 2023
matsduf
approved these changes
Dec 4, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
I discovered problems in other accessor methods in Zonemaster::LDNS::RR::NSEC3 and NSEC3PARAM.
The methods in question returned data in the wrong format: instead of the expected binary string, they returned the string prepended with a length byte, which is not very useful and can be misleading. This behavior is not described in the methods’ documentation either.
Context
Preparation of PR #177.
Changes
.tfiles;Note: although this PR is to be considered a breaking change, none of the methods were used by Zonemaster::Engine when this PR was originally created (with the
developbranch checked out).How to test this PR
Unit tests should pass.