Correctly respond with plain text BAD_REQUEST on crypto errors (#59)

Motivation:

We need to respond with plain text if we fail to remove encapsulation

Modifications:

Correctly detect if we failed to remove encapsulation and if this is the case don't try to encapsulate before responding with BAD_REQUEST

Result:

Correctly follow the RFC

codec-ohttp/src/main/java/io/netty/incubator/codec/ohttp/OHttpCiphersuite.java

@@ -19,7 +19,6 @@
 import io.netty.incubator.codec.hpke.KDF;
 import io.netty.incubator.codec.hpke.KEM;
 import io.netty.buffer.ByteBuf;
-import io.netty.handler.codec.DecoderException;
 
 import static java.util.Objects.requireNonNull;
 
@@ -75,19 +74,15 @@ static OHttpCiphersuite decode(ByteBuf in) {
         if (in.readableBytes() < ENCODED_LENGTH) {
             return null;
         }
-        try {
-            byte keyId = in.readByte();
-            short kemId = in.readShort();
-            short kdfId = in.readShort();
-            short aeadId = in.readShort();
-            return new OHttpCiphersuite(
-                    keyId,
-                    KEM.forId(kemId),
-                    KDF.forId(kdfId),
-                    AEAD.forId(aeadId));
-        } catch (Exception e) {
-            throw new DecoderException("invalid ciphersuite", e);
-        }
+        byte keyId = in.readByte();
+        short kemId = in.readShort();
+        short kdfId = in.readShort();
+        short aeadId = in.readShort();
+        return new OHttpCiphersuite(
+                keyId,
+                KEM.forId(kemId),
+                KDF.forId(kdfId),
+                AEAD.forId(aeadId));
     }
 
     @Override

codec-ohttp/src/main/java/io/netty/incubator/codec/ohttp/OHttpServerCodec.java

@@ -16,6 +16,7 @@
 package io.netty.incubator.codec.ohttp;
 
 import io.netty.buffer.ByteBufAllocator;
+import io.netty.channel.ChannelPromise;
 import io.netty.handler.codec.MessageToMessageCodec;
 import io.netty.incubator.codec.hpke.CryptoException;
 import io.netty.buffer.ByteBuf;
@@ -179,8 +180,23 @@ public final void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) th
             HttpUtil.setKeepAlive(response, false);
             onResponse(request, response);
 
-            write(ctx, response, ctx.newPromise().addListener(ChannelFutureListener.CLOSE));
-            flush(ctx);
+            ChannelPromise promise = ctx.newPromise().addListener(ChannelFutureListener.CLOSE);
+            // we should send the response without encapsulation if the error is because of
+            // removing encapsulation, otherwise we should encapsulate it.
+            //
+            // https://www.ietf.org/archive/id/draft-ietf-ohai-ohttp-10.html#section-5.2:
+            // Errors detected by the Oblivious Relay Resource and errors detected by the Oblivious Gateway Resource
+            // before removing protection (including being unable to remove encapsulation for any reason) result in the
+            // status code being sent without protection in response to the POST request made to that resource.
+            //
+            if (cause.getCause() instanceof CryptoException) {
+                // Not able to remove protection, sent without protection.
+                ctx.writeAndFlush(response, promise);
+            } else {
+                // Encapsulate and sent with protection.
+                write(ctx, response, promise);
+                flush(ctx);
+            }
         } else {
             ctx.close();
         }
@@ -271,9 +287,14 @@ private void checkPrefixDecoded()throws CryptoException {
         }
 
         @Override
-        public boolean decodePrefix(ByteBufAllocator alloc, ByteBuf in) {
+        public boolean decodePrefix(ByteBufAllocator alloc, ByteBuf in) throws CryptoException {
             final int initialReaderIndex = in.readerIndex();
-            final OHttpCiphersuite ciphersuite = OHttpCiphersuite.decode(in);
+            final OHttpCiphersuite ciphersuite;
+            try {
+                ciphersuite = OHttpCiphersuite.decode(in);
+            } catch (IllegalArgumentException e) {
+                throw new CryptoException(e);
+            }
             if (ciphersuite == null) {
                 return false;
             }

codec-ohttp/src/test/java/io/netty/incubator/codec/ohttp/OHttpServerCodecTest.java

@@ -81,7 +81,7 @@ protected OHttpVersion selectVersion(String contentTypeValue) {
 
         HttpContent lastContent = new DefaultLastHttpContent(Unpooled.buffer().writeZero(8));
         assertFalse(channel.writeInbound(lastContent));
-        assertEquals(0, content.refCnt());
+        assertEquals(0, lastContent.refCnt());
 
         delayingWriteHandler.writeAndFlushNow();
 
@@ -92,6 +92,46 @@ protected OHttpVersion selectVersion(String contentTypeValue) {
         assertFalse(channel.finish());
     }
 
+    @Test
+    public void testCryptoErrorProduceBadRequest() throws Exception {
+        AsymmetricCipherKeyPair kpR = OHttpCryptoTest.createX25519KeyPair(BouncyCastleOHttpCryptoProvider.INSTANCE,
+                "3c168975674b2fa8e465970b79c8dcf09f1c741626480bd4c6162fc5b6a98e1a");
+        byte keyId = 0x66;
+
+        OHttpServerKeys serverKeys = new OHttpServerKeys(
+                OHttpKey.newPrivateKey(
+                        keyId,
+                        KEM.X25519_SHA256,
+                        Arrays.asList(
+                                OHttpKey.newCipher(KDF.HKDF_SHA256, AEAD.AES_GCM128),
+                                OHttpKey.newCipher(KDF.HKDF_SHA256, AEAD.CHACHA20_POLY1305)),
+                        kpR));
+
+        EmbeddedChannel channel = new EmbeddedChannel(
+                new OHttpServerCodec(BouncyCastleOHttpCryptoProvider.INSTANCE, serverKeys) {
+                    @Override
+                    protected OHttpVersion selectVersion(String contentTypeValue) {
+                        return OHttpVersionDraft.INSTANCE;
+                    }
+                });
+
+        assertFalse(channel.writeInbound(new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/test")));
+
+        // There should be no outbound message yet as we did not try to parse the prefix so far.
+        assertNull(channel.readOutbound());
+
+        // Write some invalid prefix so it will fail.
+        HttpContent lastContent = new DefaultLastHttpContent(Unpooled.buffer().writeZero(8));
+        assertFalse(channel.writeInbound(lastContent));
+
+        FullHttpResponse response = channel.readOutbound();
+        assertEquals(HttpResponseStatus.BAD_REQUEST, response.status());
+        assertTrue(response.release());
+
+        assertFalse(channel.finish());
+        assertEquals(0, lastContent.refCnt());
+    }
+
     private static final class DelayingWriteHandler extends ChannelOutboundHandlerAdapter {
         private PendingWriteQueue queue;
         private ChannelHandlerContext ctx;