Don't load files in tarball exceeding size_limit_bytes
#6516
Conversation
|
Running OPA with the following configuration: services:
test:
url: https://dl.styra.com
bundles:
test:
resource: load/bundle-opa-200.tar.gz
size_limit_bytes: 1024Fails in both cases, as the file of ~200MB is way bigger than the configured limit. The difference is that now the failure preceeds reading the file in the first place. Before fix: After fix: |
| @@ -206,6 +214,9 @@ func (d *dirLoader) NextFile() (*Descriptor, error) { | |||
| if d.filter != nil && d.filter(filepath.ToSlash(path), info, getdepth(path, false)) { | |||
| return nil | |||
| } | |||
| if d.maxSizeLimitBytes > 0 && info.Size() > d.maxSizeLimitBytes { | |||
| return fmt.Errorf("file %s size %d exceeds limit of %d", path, info.Size(), d.maxSizeLimitBytes) | |||
| } | |||
There was a problem hiding this comment.
Nit: It would be great if we could apply a common format for the error message for this and the tar one.
There was a problem hiding this comment.
There was a problem hiding this comment.
I had to revert this as files like data.json and .signatures would not pass through that path.. at least not in our tests.
| @@ -343,6 +362,11 @@ func (t *tarballLoader) NextFile() (*Descriptor, error) { | |||
| } | |||
| } | |||
|
|
|||
| if t.maxSizeLimitBytes > 0 && header.Size > t.maxSizeLimitBytes { | |||
There was a problem hiding this comment.
The description for the header.Size is Logical file size in bytes and Header.Size determines how many bytes can be read for the next file. It seems this would reflect the actual file size, correct?
| if bb, ok := f.reader.(*bytes.Buffer); ok { | ||
| _ = f.Close() // always close, even on error | ||
|
|
||
| if int64(bb.Len()) >= sizeLimitBytes { | ||
| return *bb, fmt.Errorf("bundle file '%v' size (%d bytes) exceeded max size (%v bytes)", | ||
| strings.TrimPrefix(f.Path(), "/"), bb.Len(), sizeLimitBytes-1) | ||
| } | ||
|
|
||
| return *bb, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Not clear to me why we need this check here. By the time this method is called we've already called NextFile which has the size check. So if we have an offending file we would have already failed. We can keep the changes limited to the loaders looks like.
There was a problem hiding this comment.
OK, removing the size check from here.
anderseknert
force-pushed
the
loader-size-limit
branch
2 times, most recently
from
f8aa25d
to
baaadd1
Compare
There was a problem hiding this comment.
@anderseknert the changes lgtm. Not sure if we need to update the readFile method and change only the loaders. Please let me know if I missed something. Thanks.
|
@ashutosh-narkar sorry, I had apparently forgot to submit my comments 😅 Removing the check from there broke checking other types of files than .rego. |
Previously we'd check the size limit *after* the file was read, which mostly defeats the point of the limit. Now we check the size from the header in the tar archive and exit early if it exceeds the configured limit. In order to do this, I had to extend the `DirectoryLoader` interface with a method to set the max size. While I added implementations for the other (than tarball) loader types, the limit is not currently set anywhere for those. Perhaps we'll want to do that at some later point but it feels like this is mainly relevant when files are loaded via remote bundles. Fixes open-policy-agent#6514 Signed-off-by: Anders Eknert <anders@styra.com>
ashutosh-narkar
force-pushed
the
loader-size-limit
branch
from
baaadd1
to
d7dd0eb
Compare
Previously we'd check the size limit after the file was read, which mostly defeats the point of the limit. Now we check the size from the header in the tar archive and exit early if it exceeds the configured limit.
In order to do this, I had to extend the
DirectoryLoaderinterface with a method to set the max size. While I added implementations for the other (than tarball) loader types, the limit is not currently set anywhere for those. Perhaps we'll want to do that at some later point but it feels like this is mainly relevant when files are loaded via remote bundles.Fixes #6514