davsclaus commented on code in PR #27125:
URL: https://github.com/apache/camel/pull/27125#discussion_r4146726797
##########
components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/HMACAccumulator.java:
##########
@@ -133,13 +133,10 @@ static class CircularBuffer {
public void write(byte[] data, int pos, int len) {
if (available >= len) {
Review Comment:
The wrap-around logic below looks correct to me (`first = min(len, length -
write)`, then the remainder from `pos + first` to index 0), and it also fixes
the old copy from offset 0 instead of `pos`. Non-blocking and already the case
before this PR: when `available < len`, the write is silently dropped. The
callers cannot reach that today, because `decryptUpdate` gets at most
`bufferSize` bytes and the buffer is `bufferSize + maclength`. Still, throwing
an `IllegalStateException` in an `else` branch would fail loudly rather than
drop data silently if that invariant ever changes.
##########
components/camel-crypto/src/test/java/org/apache/camel/converter/crypto/CryptoDataFormatLargePayloadTest.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.converter.crypto;
+
+import javax.crypto.KeyGenerator;
+
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+
+/**
+ * The HMAC (appended by default) is split off in a circular buffer on
unmarshal. The buffer must wrap around for any
+ * message larger than the buffer size (4096 bytes by default).
+ */
+public class CryptoDataFormatLargePayloadTest extends CamelTestSupport {
+
+ @Test
+ void testRoundTripSmallerThanBuffer() throws Exception {
+ doRoundTrip(100);
+ }
+
+ @Test
+ void testRoundTripBufferSize() throws Exception {
+ doRoundTrip(4096);
+ }
+
+ @Test
+ void testRoundTripLargerThanBuffer() throws Exception {
+ doRoundTrip(5000);
+ }
+
+ @Test
+ void testRoundTripLarge() throws Exception {
+ doRoundTrip(100_000);
+ }
+
+ private void doRoundTrip(int size) throws Exception {
Review Comment:
Optional: a negative test next to the round trips would be useful here,
because the MAC check over a wrapped buffer could never be reached before this
fix. For example, marshal a 5000-byte payload, flip one byte of the ciphertext
(and separately drop the last cipher block), then assert that `unmarshal`
throws `IllegalStateException` with `HMACAccumulator.AUTHENTICATION_FAILED`. I
checked this locally with a scratch test and it holds, so this is only
regression protection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
