Brotli little proxy support by ozoz03 · Pull Request #75 · verygoodsecurity/LittleProxy

ozoz03 · 2020-05-12T18:36:06Z

Fixes https://app.clubhouse.io/vgs/story/61834/investigate-brotli-little-proxy-support

Uses #68

osklyarenko · 2020-05-12T18:36:08Z

[ch61834]

Automatically linked to https://app.clubhouse.io/vgs/story/61834

ozoz03 · 2020-05-14T18:09:30Z

+        if (initialRequest instanceof ReferenceCounted) {
+            ((ReferenceCounted)initialRequest).release();
+        }
    }


@viacheslav-fomin-main i am not sure we need this
could pls take a look?

@ozoz03 I don't know where did this commit come from, looks like you rebased your PR incorrecty.
this was removed in #48, pls revert this change.

yeah, I think this is not needed

…abled (verygoodsecurity#1)

…rotliDecoder.java for readability

… an embedded channel

…ssed file should be the array size length

…rotli filter

osklyarenko · 2020-05-21T10:39:21Z

+      InternalLoggerFactory.getInstance(BrotliDecoder.class);
+
+  static {
+    BrotliLoader.isBrotliAvailable();


what is this for?

osklyarenko · 2020-05-21T10:40:10Z

+      return null;
+    }
+
+    ByteArrayOutputStream out;


can we put it inside of try / finally?

osklyarenko · 2020-05-21T10:41:57Z

+
+  public static byte[] decompress(byte[] compressedArray) throws IOException {
+    if(compressedArray == null) {
+      return null;


why not return byte[0]?

osklyarenko · 2020-05-21T10:42:08Z

+    try (BrotliInputStream is = new BrotliInputStream(new ByteArrayInputStream(compressedArray))) {
+      out = new ByteArrayOutputStream();
+      if (!decompress(out, is)) {
+        return null;


why not return byte[0]?

osklyarenko · 2020-05-21T10:42:52Z

+        return null;
+      }
+    } catch (IOException e) {
+      log.error("Unhandled exception when decompressing brotli", e);


why is this error? are there log.error in the gzip decoder? try log.warn

also this will duplicate log error as the exception is rehtrown

osklyarenko · 2020-05-21T10:55:06Z

+  private static boolean decompress(OutputStream output, BrotliInputStream brotliInputStream) throws IOException {
+    byte[] decompressBuffer = new byte[BROTLI_MAX_NUMBER_OF_BLOCK_TYPES];
+    // is the stream ready for us to decompress?
+    int bytesRead;
+    try {
+      bytesRead = brotliInputStream.read(decompressBuffer);
+    } catch (IOException e) {
+      // unexpected end of input, not ready to decompress, so just return
+      return false;
+    }
+    // continue reading until we have hit EOF
+    while (bytesRead > -1) { // -1 means EOF
+      output.write(decompressBuffer, 0, bytesRead);
+      Arrays.fill(decompressBuffer, (byte) 0);
+      bytesRead = brotliInputStream.read(decompressBuffer);
+    }
+    return true;
+  }


Suggested change

private static boolean decompress(OutputStream output, BrotliInputStream brotliInputStream) throws IOException {

byte[] decompressBuffer = new byte[BROTLI_MAX_NUMBER_OF_BLOCK_TYPES];

// is the stream ready for us to decompress?

int bytesRead;

try {

bytesRead = brotliInputStream.read(decompressBuffer);

} catch (IOException e) {

// unexpected end of input, not ready to decompress, so just return

return false;

}

// continue reading until we have hit EOF

while (bytesRead > -1) { // -1 means EOF

output.write(decompressBuffer, 0, bytesRead);

Arrays.fill(decompressBuffer, (byte) 0);

bytesRead = brotliInputStream.read(decompressBuffer);

}

return true;

}

private static boolean decompress(OutputStream output, BrotliInputStream brotliInputStream) {

byte[] decompressBuffer = new byte[BROTLI_MAX_NUMBER_OF_BLOCK_TYPES];

// is the stream ready for us to decompress?

int bytesRead;

try {

bytesRead = brotliInputStream.read(decompressBuffer);

// continue reading until we have hit EOF

while (bytesRead > -1) { // -1 means EOF

output.write(decompressBuffer, 0, bytesRead);

Arrays.fill(decompressBuffer, (byte) 0);

bytesRead = brotliInputStream.read(decompressBuffer);

}

return true;

} catch (IOException e) {

log.warn("Could not decompress a brotli stream.", e);

// unexpected end of input, not ready to decompress, so just return

return false;

} catch (RuntimeException e) {

log.warn("Could not decompress a brotli stream.", e);

// unexpected runtime error

return false;

}

}

this impl has an uneven error handling strategy.

changes the impl so
1 - the error handling strategy is consistent
2 - runtime errors coming from the netty byte buf manipulations are accounted for
3 - removed the declared checked IOException from the signature.

the netty byte buf manipulations

https://github.com/nixxcode/jvm-brotli/blob/f4e21fd3e5a321a65e670d28beb3931bb6ffbda6/jvmbrotli/src/main/java/com/nixxcode/jvmbrotli/dec/BrotliInputStream.java#L77-L96

viacheslav-fomin-main · 2020-05-21T13:28:55Z

    protected void aggregateContentForFiltering(ChannelPipeline pipeline,
            int numberOfBytesToBuffer) {
-        pipeline.addLast("inflater", new HttpContentDecompressor());
+        pipeline.addLast("inflater", new BrotliHttpContentDecompressor());


the name was general before and now it is brotli specific, isn't it now handing 2 different compression algorithms?

can it be GeneralHttpContentDecompressor?

osklyarenko · 2020-05-21T14:19:24Z

+  @Override
+  protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {
+    /*
+       use in.alloc().buffer() instead of Unpooled.buffer() as best practice.
+       See: https://github.com/netty/netty/wiki/New-and-noteworthy-in-4.0#pooled-buffers
+    */
+    try (ByteBufOutputStream output = new ByteBufOutputStream(in.alloc().buffer())) {
+      try (BrotliInputStream brotliInputStream = new BrotliInputStream(new ByteBufInputStream(in))) {
+        if (!decompress(output, brotliInputStream)) return;
+      } catch (IOException e) {
+        log.error("Unhandled exception when decompressing brotli", e);
+        throw e;
+      }
+      out.add(output.buffer());
+    }
+  }


Suggested change

@Override

protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {

/*

use in.alloc().buffer() instead of Unpooled.buffer() as best practice.

See: https://github.com/netty/netty/wiki/New-and-noteworthy-in-4.0#pooled-buffers

*/

try (ByteBufOutputStream output = new ByteBufOutputStream(in.alloc().buffer())) {

try (BrotliInputStream brotliInputStream = new BrotliInputStream(new ByteBufInputStream(in))) {

if (!decompress(output, brotliInputStream)) return;

} catch (IOException e) {

log.error("Unhandled exception when decompressing brotli", e);

throw e;

}

out.add(output.buffer());

}

}

@Override

protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {

/*

use in.alloc().buffer() instead of Unpooled.buffer() as best practice.

See: https://github.com/netty/netty/wiki/New-and-noteworthy-in-4.0#pooled-buffers

*/

boolean success = true;

ByteBuf outBuffer = null;

try (ByteBufOutputStream output = new ByteBufOutputStream(in.alloc().buffer())) {

try (ByteBufInputStream bbin = new ByteBufInputStream(in)) {

try (BrotliInputStream brotliInputStream = new BrotliInputStream(bbin)) {

success = decompress(output, brotliInputStream);

if (!success) {

return;

}

} catch (IOException e) {

log.error("Unhandled exception when decompressing brotli", e);

throw e;

}

}

outBuffer = output.buffer();

if (outBuffer.isReadable()) {

out.add(outBuffer);

} else {

success = false;

}

} finally {

if (!success) {

if (outBuffer != null) {

outBuffer.release();

}

}

}

}

this implementation follows practices established by other implementations of the decoder i found in the netty source files

a buffer should be released if the decompression was unsuccessful. otherwise we have a memory leak

reading properties of an output buffer should be checked. if they are unfit, the buffer is released.

osklyarenko · 2020-05-21T14:21:46Z

+       use in.alloc().buffer() instead of Unpooled.buffer() as best practice.
+       See: https://github.com/netty/netty/wiki/New-and-noteworthy-in-4.0#pooled-buffers
+    */
+    try (ByteBufOutputStream output = new ByteBufOutputStream(in.alloc().buffer())) {


usually the buffer allocation is done via ctx.

osklyarenko · 2020-05-21T14:25:46Z

+  public BrotliDecoder() {
+  }
+
+  public static byte[] decompress(byte[] compressedArray) throws IOException {


who is the consumer of this public method? so far i see only tests. if so, pls remove it.

osklyarenko · 2020-05-21T14:56:09Z

+  protected void encode(ChannelHandlerContext ctx, ByteBuf uncompressed, ByteBuf out) throws Exception {
+    ByteBufOutputStream dst = new ByteBufOutputStream(uncompressed.alloc().buffer());
+    Encoder.Parameters params = new Encoder.Parameters().setQuality(this.compressionQuality);
+    try {
+      BrotliOutputStream brotliOutputStream = new BrotliOutputStream(dst, params);
+      try {
+        brotliOutputStream.write(ByteBufUtil.getBytes(uncompressed));
+      } finally {
+        brotliOutputStream.flush();
+        brotliOutputStream.close();
+        out.writeBytes(dst.buffer());
+      }
+    } catch (IOException e) {
+      log.error("Unhandled exception when compressing brotli", e);
+      throw e;
+    }
+
+  }


Suggested change

protected void encode(ChannelHandlerContext ctx, ByteBuf uncompressed, ByteBuf out) throws Exception {

ByteBufOutputStream dst = new ByteBufOutputStream(uncompressed.alloc().buffer());

Encoder.Parameters params = new Encoder.Parameters().setQuality(this.compressionQuality);

try {

BrotliOutputStream brotliOutputStream = new BrotliOutputStream(dst, params);

try {

brotliOutputStream.write(ByteBufUtil.getBytes(uncompressed));

} finally {

brotliOutputStream.flush();

brotliOutputStream.close();

out.writeBytes(dst.buffer());

}

} catch (IOException e) {

log.error("Unhandled exception when compressing brotli", e);

throw e;

}

}

protected void encode(ChannelHandlerContext ctx, ByteBuf uncompressed, ByteBuf out) throws Exception {

if (!uncompressed.isReadable() || uncompressed.readableBytes() == 0) {

return;

}

ByteBufOutputStream dst = new ByteBufOutputStream(uncompressed.alloc().buffer());

Encoder.Parameters params = new Encoder.Parameters().setQuality(this.compressionQuality);

try {

BrotliOutputStream brotliOutputStream = new BrotliOutputStream(dst, params);

try {

brotliOutputStream.write(ByteBufUtil.getBytes(uncompressed, uncompressed.readerIndex(), uncompressed.readableBytes(), false));

} finally {

brotliOutputStream.flush();

brotliOutputStream.close();

out.writeBytes(dst.buffer());

}

} catch (IOException e) {

log.error("Unhandled exception when compressing brotli", e);

throw e;

}

}

nestorsokil · 2020-05-26T13:29:26Z

+    ByteBuf outBuffer = null;
+    try (ByteBufOutputStream output = new ByteBufOutputStream(in.alloc().buffer())) {
+      try (ByteBufInputStream bbin = new ByteBufInputStream(in)) {
+        try (BrotliInputStream brotliInputStream = new BrotliInputStream(bbin)) {


You can have multiple statements in a single try()

nestorsokil · 2020-05-26T13:34:13Z

+  public static boolean decompress(OutputStream output, BrotliInputStream brotliInputStream) {
+    byte[] decompressBuffer = new byte[BROTLI_MAX_NUMBER_OF_BLOCK_TYPES];
+    // is the stream ready for us to decompress?
+    int bytesRead;


it seems it can be moved inside try

nestorsokil · 2020-05-26T13:35:38Z

+      }
+    } finally {
+      if (!success) {
+        if (outBuffer != null) {


can be collapsed with &&

nestorsokil · 2020-05-26T13:43:26Z

+
+    String contentEncoding = response.headers().get(HttpHeaderNames.CONTENT_ENCODING);
+    if (contentEncoding != null) {
+      // Content-Encoding was set, either as something specific or as the IDENTITY encoding


should this be 'was not set'?

it means that it was already somehow encoded

nestorsokil · 2020-05-26T13:46:48Z

+  private static final int DEFAULT_COMPRESSION_QUALITY = 4;
+
+  /*
+   If the Brotli encoding is being used to compress streams in real-time,


comment seems to be identical to the above, can we describe this one properly?

ozoz03 requested review from mjallday, osklyarenko and viacheslav-fomin-main as code owners May 12, 2020 18:36

ozoz03 changed the title ~~Investigate brotli little proxy support~~ [WIP] Investigate brotli little proxy support May 12, 2020

ozoz03 force-pushed the ozasymenko/ch61834/investigate-brotli-little-proxy-support branch from a4f92a1 to d2cc6cf Compare May 13, 2020 12:24

ozoz03 commented May 14, 2020

View reviewed changes

mjallday and others added 23 commits May 15, 2020 09:42

Fixing memory leak on initial request when HttpObjectAggregator is en…

425b1d0

…abled (verygoodsecurity#1)

Brotli Decoder Codec for Netty is passing

8cd9f33

Rebae

f20e206

Attribute link to the AbstractCompressionTest. Clarify bytesRead in B…

8acbaac

…rotliDecoder.java for readability

Add Brotli Encoding tests

6661676

Updated attribution for the abstract encoder test from Netty

f14790d

Allow usage of compression and decompression without needing to be in…

f400dc1

… an embedded channel

When decoding a message less than the byte array length, the decompre…

af2b0e9

…ssed file should be the array size length

Add a BrotliEncoderDecoder test to start the integration testing

ac23de3

Start a local Jetty webserver to do integration tests for Brotli

29c0d7d

So close

01c7ddc

Integration tests are fully working, ready to start integrating the B…

702b2c2

…rotli filter

Refactor tests and add helper init method

7e7d2d3

more test fixes for the encoder / decoder

8e9220f

Added brotli handlers

0bd0530

Added handlers to pipeline

07cc7a0

refactored decompression

de1505e

removed odd test

52a4435

docker brotli

01ddede

docker brotli 2

7d72e64

docker brotli 2 fix

0d99cad

docker test

3dc0f60

docker brotli 1 again

754327a

osklyarenko reviewed May 21, 2020

View reviewed changes

viacheslav-fomin-main reviewed May 21, 2020

View reviewed changes

osklyarenko reviewed May 21, 2020

View reviewed changes

ozoz03 added 3 commits May 21, 2020 19:47

removed ReferenceCounted release()

ae7ecdb

code review changes

ef2f4ac

brotli support check

6669654

ozoz03 requested review from k-sever, osklyarenko and viacheslav-fomin-main May 22, 2020 08:52

ozoz03 added 3 commits May 25, 2020 12:05

decoder refactored

52c8971

refactored encoder

7d702f9

reverted AbstractProxyTest

90b34ed

nestorsokil reviewed May 26, 2020

View reviewed changes

ozoz03 added 2 commits May 26, 2020 17:39

reverted ProxyToServerConnection changes

9083776

a minor comment fix

5753ab6

ozoz03 requested a review from nestorsokil May 26, 2020 15:55

ozoz03 added 4 commits May 26, 2020 19:53

reverted ProxyToServerConnection changes 2

0077c3d

reverted ProxyToServerConnection changes 3

f25b1b0

reverted ProxyToServerConnection changes 4

6cd2555

reverted ProxyToServerConnection changes 5

e73c4df

k-sever approved these changes May 26, 2020

View reviewed changes

k-sever merged commit cc2c5e3 into verygoodsecurity:vgs-edition May 26, 2020

Conversation

ozoz03 commented May 12, 2020

Uh oh!

osklyarenko commented May 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osklyarenko May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nestorsokil May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

osklyarenko May 21, 2020 •

edited

Loading

nestorsokil May 26, 2020 •

edited

Loading