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.

Sign up for GitHub

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

Jump to bottom

Move RR content validation away from nslord #385

Merged
merged 5 commits into from
Aug 27, 2020
Merged

Move RR content validation away from nslord #385

merged 5 commits into from
Aug 27, 2020

Conversation

nils-wisiol
Copy link
Contributor

No description provided.

@nils-wisiol nils-wisiol force-pushed the 20200514_validation branch from d8a2419 to 60fe992 Compare May 15, 2020 15:05
nils-wisiol
nils-wisiol commented May 15, 2020
View reviewed changes
Copy link
Contributor Author

@nils-wisiol nils-wisiol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This gets a couple more tests, but is ready for generally ready for review :)

@nils-wisiol nils-wisiol force-pushed the 20200514_validation branch 2 times, most recently from 7768554 to b7cbdac Compare May 15, 2020 17:47
@nils-wisiol
Copy link
Contributor Author

In relation to #334, this imposes an additional 255 char limit "per field" which we may not want

@nils-wisiol
Copy link
Contributor Author

In relation to #334, this imposes an additional 255 char limit "per field" which we may not want

Actually, this limit is per "token" and not per record, and was apparently introduced by BIND. It seems to be widely accepted although I could not find any standard on it.

With the current status of this PR, we would start to enforce this widely accepted non-standard limitation, because it is enforced by dnspython. We could make an exception, but this may interfere with future development that we have planned.

I separated the tests for the "255 limit" per token and "500 limit" per record, and adapted the error message for the former appropriately.

@andrewtj
Copy link

I can clarify the size limits. TXT records are composed of character-strings. A character-string is up to 255 arbitrary bytes, preceded by their length in a single byte.

In ASCII presentation format a character-string can be longer than 255 bytes as non printable ASCII bytes are presented in the \DDD format.

So for example a single zero byte character-string would be "\000" in ASCII presentation format and on the wire would be the two byte sequence 0x01 for the length and 0x00 for the content.

There is no size limit on the TXT record itself other than imposed by RDLENGTH (16-bits) and the restraints of being in a message (header, RR owner meta, etc).

@andrewtj
Copy link

I should also mention that PowerDNS doesn't parse TXT records in an entirely standard way. It is briefly mentioned here but I can't find a precise reference to exactly what it does. Possibly its behaviour has lead to some of the confusion here.

@peterthomassen
Copy link
Member

I can clarify the size limits. TXT records are composed of character-strings. A character-string is up to 255 arbitrary bytes, preceded by their length in a single byte.
...
There is no size limit on the TXT record itself other than imposed by RDLENGTH (16-bits) and the restraints of being in a message (header, RR owner meta, etc).

Ah, I also remember that pdns permits TXT records to be longer than 255 bytes "on the admin side" (database, API etc.), and handles the 255-byte chunking as an aspect of client-side serialization.

I think that's a pretty reasonable decision, but it causes the above problem if we introduce a validation layer that works a) on the data that we want to send to the pdns API, but b) is based on "client-side style" serialization/deserialization (using dnspython).

@Habbie, did I summarize this correctly?

peterthomassen
peterthomassen requested changes May 17, 2020
View reviewed changes
Copy link
Member

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

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

Great work! I have some minor suggestions regarding code smoothness here and there, but otherwise really great.

There's of course the outstanding issue with the 255 byte limit (it's actually a mismatch between what our API considers a record, and what dnspython considers a record or token), and the inclusion of SMIMEA.

@nils-wisiol
Copy link
Contributor Author

resolved a couple of annotations, added a couple of tests. Of particular interest are the TXT canonicalization tests: we newly accept now foo bar, which means "foo" "bar". The semantics of anything in quotation marks didn't change.

The increase of the max token length is still missing.

@nils-wisiol nils-wisiol force-pushed the 20200514_validation branch from 6857403 to f5e813d Compare May 27, 2020 08:02
@nils-wisiol nils-wisiol marked this pull request as ready for review May 27, 2020 08:02
@nils-wisiol
Copy link
Contributor Author

I (re)introduced non-standard TXT/SPF records with token length up to 256**8-1. The validation is now still based upon dnspython, but involves quite some copy-paste code. Not sure how we want to proceed with this, some options:

  • only offer standard records, at the cost of the user's convenience
  • don't care about the copy-paste code, as it is likely to last (we wouldn't expect changes to the TXT record format)
  • a smarter solution (you indicated in code annotations that you may have an idea, @peterthomassen ?)

Instead of a database migration, I opted to canonicalize record content every time we talk to pdns. We can apply a migration anytime (not saying it shouldn't be now) and then remove this piece of code again.

Some tests for more specific record types are still missing.

@nils-wisiol
Copy link
Contributor Author

Anyways, this extends the TXT record interface to also accept not-quoted record contents. For interpretation details, see the tests. (It follows RFC 1035 with the exception of longer tokens.)

@nils-wisiol nils-wisiol force-pushed the 20200514_validation branch from f5e813d to b1e98fa Compare May 27, 2020 09:07
@nils-wisiol
Copy link
Contributor Author

This needs some more e2e testing to make sure pdns accepts our canonicalized input.

@nils-wisiol nils-wisiol force-pushed the 20200514_validation branch 3 times, most recently from 2b40c35 to 4f9ec40 Compare June 1, 2020 20:46
@nils-wisiol
Copy link
Contributor Author

I cleaned up the history. What's still missing:

  • add a database migration that canonicalizes all database record contents
  • determine what's our canonical TXT and SPF presentation format: chunked or not
    • if not chunked, determine what we send to pdns (chunked or not)
  • unresolved conversations from above

@nils-wisiol nils-wisiol force-pushed the 20200514_validation branch 6 times, most recently from ade3217 to 8a13302 Compare August 26, 2020 13:31
peterthomassen
peterthomassen approved these changes Aug 26, 2020
View reviewed changes
Copy link
Member

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

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

Great stuff! Lgtm, very minor comments

@nils-wisiol nils-wisiol force-pushed the 20200514_validation branch 3 times, most recently from cb06b03 to 9d3f501 Compare August 27, 2020 08:24
@nils-wisiol
Copy link
Contributor Author

ready for second round, everything after the force push is new and addressing review comments

@nils-wisiol
feat(www): remove obsolete autogernated certs upon next startup
7757b89
@nils-wisiol
fix(api): overhaul of exception handling
3d30ba2 
This fixes some faulty behavior, such as
- returning 503 when actually 500 is appropriate and
- counting db-unrelated errors towards the db outage metric.
@nils-wisiol
Copy link
Contributor Author

ready for next round of review

peterthomassen
peterthomassen requested changes Aug 27, 2020
View reviewed changes
Copy link
Member

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

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

two small changes

docs/dns/rrsets.rst Outdated
@@ -93,8 +93,8 @@ Field details:
Records must be given in presentation format (a.k.a. "BIND" or zone file
format). Record values that are not given in canonical format, such as
``127.000.0.1`` will be converted by the API into canonical format, e.g.
Copy link
Member

Choose a reason for hiding this comment

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

adjust (leading zeros disallowed)

@nils-wisiol nils-wisiol merged commit 8fdb998 into master Aug 27, 2020
@nils-wisiol nils-wisiol deleted the 20200514_validation branch August 27, 2020 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterthomassen peterthomassen peterthomassen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
002: Record Content Validation
Development

Successfully merging this pull request may close these issues.
4 participants
@nils-wisiol @andrewtj @peterthomassen @NewRedsquare