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

Buffer bump #1446

Merged
merged 2 commits into from Apr 27, 2023
Merged

Buffer bump #1446

merged 2 commits into from Apr 27, 2023

Conversation

systemcrash
Copy link
Contributor

Also version bump

See individual commit messages

@tmthrgd
Copy link
Collaborator

tmthrgd commented Mar 29, 2023

Do people really care about GPG/PGP these days?

version.go Outdated Show resolved Hide resolved
@miekg
Copy link
Owner

miekg commented Mar 29, 2023

To add to @tmthrgd reply: this is with older RSA keys or something? Messsages this big will also lead to fragmentation problems on the wire

@systemcrash
Copy link
Contributor Author

Do people really care about GPG/PGP these days?

case in point.

To add to @tmthrgd reply: this is with older RSA keys or something? Messsages this big will also lead to fragmentation problems on the wire

messages this small evidently lead to problems in this lib before it even gets on the wire.

your point is moot: DOT, DOH, TCP based protos resolve this. These problems are discussed in the RFC.

@miekg
Copy link
Owner

miekg commented Mar 29, 2023 via email

@systemcrash
Copy link
Contributor Author

just upping the value also isn't a real solution. Add a space (as bind also randomly does) should (i think) also work, as then you get multiple tokens. all the newer based protos are mostly for the last mile, so of very much limited value

It's low hanging fruit - the RFC states just a single line of base64 without elaborating further.

@systemcrash
Copy link
Contributor Author

Add a space (as bind also randomly does) should (i think) also work, as then you get multiple tokens.

How does adding a space into the source text help the buffer of fixed size, which holds the text?

Your test cases seem to be designed to trigger the error through sheer length, whether split by spaces or not.

@systemcrash
Copy link
Contributor Author

systemcrash commented Mar 29, 2023

OK - how about this? Just reduce the overall buffer size to 512 bytes, and make it something we can grow by 512 bytes every time we run out. It also fixes my problem, and obsoletes length test cases (or at least reverses the fail logic).

@systemcrash systemcrash requested review from miekg and removed request for tmthrgd March 29, 2023 17:05
make max token size buffers str and com something we can grow.

Why? Because.

Reasons: When experimenting with OPENPGPKEY records, which themselves
are basically a key, if my zone file already has an RSA 4096 public key
record, this lib goes boom:

dns: bad OPENPGPKEY PublicKey: "token length insufficient for parsing" at line: .....

The key is good.

Testing against bare ed25519 keys whose base64 length is ~320 characters
and there are no problems

Testing against a bare RSA4096 key whose base64 length is ~3100 characters
and: problems.

Bare is the key word here, since for DNS, one ideally publishes as bare
a key as possible, minus signatures, minus photos, minus extra
metadata beyond the essential that will push a record length up.

A typical public key with two RSA 4K subkeys and several signatures may
rise to > ~21000 bytes.
Reverse TLSA test record fail logic

TestNewRRCommentLengthCrasherString: ...
@miekg
Copy link
Owner

miekg commented Apr 27, 2023

OK - how about this? Just reduce the overall buffer size to 512 bytes,

if that's easy to do, then yes

@systemcrash
Copy link
Contributor Author

systemcrash commented Apr 27, 2023 via email

@miekg miekg merged commit f07f1e6 into miekg:master Apr 27, 2023
4 checks passed
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.

None yet

3 participants