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

Allow use of fs.FS for $INCLUDE and wrap errors #1526

Merged
merged 2 commits into from
Jan 15, 2024
Merged

Allow use of fs.FS for $INCLUDE and wrap errors #1526

merged 2 commits into from
Jan 15, 2024

Conversation

dpifke
Copy link
Contributor

@dpifke dpifke commented Jan 9, 2024

This adds ZoneParser.SetIncludeAllowedFS, to specify an fs.FS when enabling support for $INCLUDE, for reading included files from somewhere other than the local filesystem.

I've also modified ParseError to support wrapping another error, such as errors encountered while opening the $INCLUDE target. This allows for much more robust handling, using errors.Is() instead of testing for particular strings (which may not be identical between fs.FS implementations).

ParseError was being constructed in a lot of places using positional instead of named members. Updating ParseError initialization after the new member field was added makes this change seem a lot larger than it actually is.

The changes here should be completely backwards compatible. The ParseError change should be invisible to anyone not trying to unwrap it, and ZoneParser will continue to use os.Open if the existing SetIncludeAllowed method is called instead of the new SetIncludeAllowedFS method.

@dpifke
Allow use of fs.FS for $INCLUDE and wrap errors
3a4f2f2 
This adds ZoneParser.SetIncludeAllowedFS, to specify an fs.FS when
enabling support for $INCLUDE, for reading included files from
somewhere other than the local filesystem.

I've also modified ParseError to support wrapping another error, such
as errors encountered while opening the $INCLUDE target.  This allows
for much more robust handling, using errors.Is() instead of testing
for particular strings (which may not be identical between fs.FS
implementations).

ParseError was being constructed in a lot of places using positional
instead of named members.  Updating ParseError initialization after
the new member field was added makes this change seem a lot larger
than it actually is.

The changes here should be completely backwards compatible.  The
ParseError change should be invisible to anyone not trying to unwrap
it, and ZoneParser will continue to use os.Open if the existing
SetIncludeAllowed method is called instead of the new
SetIncludeAllowedFS method.
@dpifke
Copy link
Contributor Author

dpifke commented Jan 9, 2024

(Deleted previous comment where I completely got wrong when fs.FS was introduced. Sorry for the noise.)

fs.FS was introduced in Go 1.16: https://go.dev/doc/go1.16#fs

Error wrapping was introduced in Go 1.13: https://go.dev/doc/go1.13#error_wrapping

@miekg
Copy link
Owner

miekg commented Jan 9, 2024

brilliant idea!

Code looks good, bit messy indeed due to the error changes. Some comments incomding

miekg
miekg reviewed Jan 9, 2024
View reviewed changes
scan.go Outdated
@@ -220,6 +228,12 @@ func (zp *ZoneParser) SetIncludeAllowed(v bool) {
zp.includeAllowed = v
}

// SetIncludeAllowedFS controls whether $INCLUDE directives are allowed, and
// specifies an [fs.FS] from which to open and read files.
func (zp *ZoneParser) SetIncludeAllowedFS(v bool, fsys fs.FS) {
Copy link
Owner

Choose a reason for hiding this comment

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

On the fence if we need a whole new method for this. Should we give SetIncludeAllowed an optional fs.FS parameter instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new method because I didn't want to break the method signature for existing library users.

We could change SetIncludeAllowed(v bool) to SetIncludeAllowed(v bool, fs ...fs.FS), which should be backwards compatible, but would need to decide how to handle multiple fs.FS being provided. (Try each one in sequence until $INCLUDE is found?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shipping separate methods as a non-breaking change, and then consolidating them if/when you release a v2 is an option.

Copy link
Owner

Choose a reason for hiding this comment

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

ack. Think my main objection is the overlap in functionality. Maybe a better idea is to have a seperate method to set the underlying FS? SetIncludeFS that just sets the FS

@dpifke
Don't duplicate SetIncludeAllowed; clarify edge cases
d59cf64 
Rather than duplicate functionality between SetIncludeAllowed and
SetIncludeAllowedFS, have a method SetIncludeFS, which only sets the
fs.FS.

I've improved the documentation to point out some considerations for
users hoping to use fs.FS as a security boundary.

Per the fs.ValidPath documentation, fs.FS implementations must use
path (not filepath) semantics, with slash as a separator (even on
Windows).  Some, like os.DirFS, also require all paths to be relative.
I've clarified this in the documentation, made the includePath
manipulation more robust to edge cases, and added some additional
tests for relative and absolute paths.
@miekg miekg merged commit 50fbccd into miekg:master Jan 15, 2024
4 checks passed
jefferai pushed a commit to jefferai/dns that referenced this pull request Aug 7, 2024
@johanbrandhorst @noxer
@jimlambrt @miekg @wintbiit @dpifke @dependabot @gibson042 @montag451 @cesarkuroiwa @dcarrillo @janik-cloudflare @eest @bemasc @steffsas @Infinoid
Merge upstream changes (#3)
d7de0c1 
* Swap closing order in `inAxfr` and `inIxfr` (miekg#1511)

* Fix closing order

* Comment to make clear that the close order is deliberate

---------

Co-authored-by: Tim Scheuermann <tscheuermann@anexia-it.com>

* feat: add support for ReuseAddr (miekg#1510)

* feat: add support for ReuseAddr

* Update listen_reuseport.go

* Update listen_reuseport.go

* fixup! feat: add support for ReuseAddr

---------

Co-authored-by: Miek Gieben <miek@miek.nl>

* Release 1.1.57

* Try explaining duplicate RCODEs

Add extra link to the docs for the duplicate Rcode entries

See miekg#1523

Signed-off-by: Miek Gieben <miek@miek.nl>

* docs: added ninedos to readme (miekg#1522)

* Allow use of fs.FS for $INCLUDE and wrap errors (miekg#1526)

* Allow use of fs.FS for $INCLUDE and wrap errors

This adds ZoneParser.SetIncludeAllowedFS, to specify an fs.FS when
enabling support for $INCLUDE, for reading included files from
somewhere other than the local filesystem.

I've also modified ParseError to support wrapping another error, such
as errors encountered while opening the $INCLUDE target.  This allows
for much more robust handling, using errors.Is() instead of testing
for particular strings (which may not be identical between fs.FS
implementations).

ParseError was being constructed in a lot of places using positional
instead of named members.  Updating ParseError initialization after
the new member field was added makes this change seem a lot larger
than it actually is.

The changes here should be completely backwards compatible.  The
ParseError change should be invisible to anyone not trying to unwrap
it, and ZoneParser will continue to use os.Open if the existing
SetIncludeAllowed method is called instead of the new
SetIncludeAllowedFS method.

* Don't duplicate SetIncludeAllowed; clarify edge cases

Rather than duplicate functionality between SetIncludeAllowed and
SetIncludeAllowedFS, have a method SetIncludeFS, which only sets the
fs.FS.

I've improved the documentation to point out some considerations for
users hoping to use fs.FS as a security boundary.

Per the fs.ValidPath documentation, fs.FS implementations must use
path (not filepath) semantics, with slash as a separator (even on
Windows).  Some, like os.DirFS, also require all paths to be relative.
I've clarified this in the documentation, made the includePath
manipulation more robust to edge cases, and added some additional
tests for relative and absolute paths.

* Bump golang.org/x/net from 0.17.0 to 0.19.0 (miekg#1520)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.17.0 to 0.19.0.
- [Commits](golang/net@v0.17.0...v0.19.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump golang.org/x/sys from 0.13.0 to 0.15.0 (miekg#1518)

Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.13.0 to 0.15.0.
- [Commits](golang/sys@v0.13.0...v0.15.0)

---
updated-dependencies:
- dependency-name: golang.org/x/sys
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add NXT record (miekg#1516)

This add the NXT record (2535) to implement all records from the RFC.

Also does a s/RFC RFC/RFC/ as I happen to bumb into that will editing
the comments.

Signed-off-by: Miek Gieben <miek@miek.nl>

* Add ISDN record (miekg#1515)

We had the type code, this add the rest. Other RRs from 1183 are also
fully impl. don't know why this one wasn't.

Signed-off-by: Miek Gieben <miek@miek.nl>

* Bump golang.org/x/tools from 0.13.0 to 0.17.0 (miekg#1529)

Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.13.0 to 0.17.0.
- [Release notes](https://github.com/golang/tools/releases)
- [Commits](golang/tools@v0.13.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Release 1.1.58

* Improve NewRR documentation (miekg#1531)

In particular, document the default origin.

* Add incus to the list of users (miekg#1535)

* Add option to do a zone transfer via TLS (miekg#1533)

* New func InTLS

Perform zone transfer via TLS

* Test xfr via TLS

* New field TLS, used to transfer via TLS

---------

Co-authored-by: Cesar Kuroiwa <cesar@registro.br>

* IsDomainName: check for escape as last character (miekg#1532)

Keep track if the escape, if still true when returning isDomainName
should return false.

TODO:
- Should still be done in packDomainName as well.
- And that should be tested
- Some tests now fail

There are multiple other places that supposedly also check for this, but
they are not called in the parsing.

Fixes: miekg#1528

Signed-off-by: Miek Gieben <miek@miek.nl>

* Bump golang.org/x/sys from 0.16.0 to 0.17.0 (miekg#1541)

Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.16.0 to 0.17.0.
- [Commits](golang/sys@v0.16.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/sys
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump golang.org/x/net from 0.20.0 to 0.21.0 (miekg#1542)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.20.0 to 0.21.0.
- [Commits](golang/net@v0.20.0...v0.21.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump golang.org/x/tools from 0.17.0 to 0.19.0 (miekg#1551)

Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.17.0 to 0.19.0.
- [Release notes](https://github.com/golang/tools/releases)
- [Commits](golang/tools@v0.17.0...v0.19.0)

---
updated-dependencies:
- dependency-name: golang.org/x/tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: fix some comments (miekg#1547)

Signed-off-by: xiaoxiangxianzi <zhaoyizheng@outlook.com>

* Add ifconfig.es to the list of users (miekg#1554)

* Fix counting of escape sequences when splitting TXT strings (miekg#1540)

`endingToTxtSlice`, used by TXT, SPF and a few other types, parses a
string such as `"hello world"` from an RR's content in a zone file.
These strings are limited to 255 characters, and `endingToTxtSlice`
automatically splits them if they're longer than that. However, it
didn't count the length correctly: escape sequences such as `\\` or
`\123` were counted as multiple characters (2 and 4 respectively in
these examples), but they should only count as one character because
they represent a single byte in wire format (which is where this 255
character limit comes from). This commit fixes that.

* Fix possible out-of-bounds read in endingToTxtSlice (miekg#1557)

* Update escapedStringOffset to improve readability

This function was, admittedly, a little difficult to follow. This new
version is slightly more verbose, but, in my opinion, easier to
understand.

* Fix possible out-of-bounds read in endingToTxtSlice caused by escapedStringOffset

If the input had a trailing backslash (normally the start of an escape
sequence) with nothing following it, `escapedStringOffset` would return
the length of the input, plus one (!), as the result index, causing an
out-of-bounds read and panic in `endingToTxtSlice`.

Consistent with, e.g., commit 2230854,
I've decided to make this an error since it definitely indicates that
the string isn't valid.

Credit to OSS-Fuzz -- thank you!

* Bump golang.org/x/sys from 0.18.0 to 0.20.0 (miekg#1571)

Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.18.0 to 0.20.0.
- [Commits](golang/sys@v0.18.0...v0.20.0)

---
updated-dependencies:
- dependency-name: golang.org/x/sys
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump golang.org/x/net from 0.22.0 to 0.25.0 (miekg#1569)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.22.0 to 0.25.0.
- [Commits](golang/net@v0.22.0...v0.25.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump golang.org/x/tools from 0.19.0 to 0.22.0 (miekg#1574)

Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.19.0 to 0.22.0.
- [Release notes](https://github.com/golang/tools/releases)
- [Commits](golang/tools@v0.19.0...v0.22.0)

---
updated-dependencies:
- dependency-name: golang.org/x/tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* (*Transfer) Out: Increment WaitGroup in example (miekg#1572)

* Add a hook to catch invalid messages (miekg#1568)

* Add a hook to catch invalid messages

Currently there are hooks for reading messages off the wire (DecorateReader),
checking if they comply with policy (MsgAcceptFunc), and generating responses
(Handler).  However, there is no hook that notifies the server when a message is
dropped or rejected due to a syntax error.  That makes it hard to monitor these
packets without repeating the parsing process.

This PR adds a hook for notifications about invalid packets.

* s/InvalidMsg/MsgInvalid/g

* These two too

Signed-off-by: Miek Gieben <miek@miek.nl>

* Add RFC 9540 oblivious services via service binding records (miekg#1567)

* update list of RFCs

Signed-off-by: Miek Gieben <miek@miek.nl>

* add rfc3596 to the list (miekg#1577)

---------

Signed-off-by: Miek Gieben <miek@miek.nl>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: xiaoxiangxianzi <zhaoyizheng@outlook.com>
Co-authored-by: Tim Scheuermann <noxer@users.noreply.github.com>
Co-authored-by: Tim Scheuermann <tscheuermann@anexia-it.com>
Co-authored-by: Jim <jlambert@hashicorp.com>
Co-authored-by: Miek Gieben <miek@miek.nl>
Co-authored-by: WintBit <wintbit@gmail.com>
Co-authored-by: Dave Pifke <dave@pifke.org>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: montag451 <montag451@laposte.net>
Co-authored-by: Cesar Kuroiwa <abc@kuroiwa.com.br>
Co-authored-by: Cesar Kuroiwa <cesar@registro.br>
Co-authored-by: xiaoxiangxianzi <164908047+xiaoxiangxianzi@users.noreply.github.com>
Co-authored-by: dcarrillo <daniel.carrillo@gmail.com>
Co-authored-by: Janik Rabe <janik@cloudflare.com>
Co-authored-by: Patrik Lundin <patrik@sigterm.se>
Co-authored-by: Benjamin M. Schwartz <ben@bemasc.net>
Co-authored-by: Steffen Sassalla <32709406+steffsas@users.noreply.github.com>
Co-authored-by: Infinoid <mark-github@glines.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miekg miekg miekg left review comments

@tmthrgd tmthrgd Awaiting requested review from tmthrgd tmthrgd is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dpifke @miekg