From bbaf176049cd6b3fd47256a34926c39ccaf4fec4 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 18 Apr 2023 18:36:02 +0530 Subject: [PATCH 1/7] ssvm: fix post request header case mismatch Fixes #7442 Signed-off-by: Abhishek Kumar --- .../resource/HttpUploadServerHandler.java | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java index 8e3e4908dcea..a71271b5a426 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java @@ -27,7 +27,6 @@ import java.util.Map; import java.util.Map.Entry; -import io.netty.handler.codec.DecoderException; import org.apache.cloudstack.storage.template.UploadEntity; import org.apache.cloudstack.utils.imagestore.ImageStoreUtil; import org.apache.commons.lang3.StringUtils; @@ -41,6 +40,7 @@ import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.SimpleChannelInboundHandler; +import io.netty.handler.codec.DecoderException; import io.netty.handler.codec.http.DefaultFullHttpResponse; import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.HttpContent; @@ -130,22 +130,16 @@ public void channelRead0(ChannelHandlerContext ctx, HttpObject msg) throws Excep long contentLength = 0; for (Entry entry : request.headers()) { - switch (entry.getKey()) { - case HEADER_SIGNATURE: - signature = entry.getValue(); - break; - case HEADER_METADATA: - metadata = entry.getValue(); - break; - case HEADER_EXPIRES: - expires = entry.getValue(); - break; - case HEADER_HOST: - hostname = entry.getValue(); - break; - case HttpHeaders.Names.CONTENT_LENGTH: - contentLength = Long.parseLong(entry.getValue()); - break; + if (HEADER_SIGNATURE.equalsIgnoreCase(entry.getKey())) { + signature = entry.getValue(); + } else if (HEADER_METADATA.equalsIgnoreCase(entry.getKey())) { + metadata = entry.getValue(); + } else if (HEADER_EXPIRES.equalsIgnoreCase(entry.getKey())) { + expires = entry.getValue(); + } else if (HEADER_HOST.equalsIgnoreCase(entry.getKey())) { + hostname = entry.getValue(); + } else if (HttpHeaders.Names.CONTENT_LENGTH.equalsIgnoreCase(entry.getKey())) { + contentLength = Long.parseLong(entry.getValue()); } } logger.info("HEADER: signature=" + signature); From d98b408d67eb60d76da93d9d0241b92408438c72 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 20 Apr 2023 18:15:53 +0530 Subject: [PATCH 2/7] refactor, add test Signed-off-by: Abhishek Kumar --- .../resource/HttpUploadServerHandler.java | 79 +++++++++++-------- .../resource/HttpUploadServerHandlerTest.java | 74 +++++++++++++++++ 2 files changed, 118 insertions(+), 35 deletions(-) create mode 100644 services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java index a71271b5a426..959d91ee7515 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java @@ -18,11 +18,11 @@ import static io.netty.buffer.Unpooled.copiedBuffer; import static io.netty.handler.codec.http.HttpHeaders.Names.CONNECTION; -import static io.netty.handler.codec.http.HttpHeaders.Names.CONTENT_LENGTH; import static io.netty.handler.codec.http.HttpHeaders.Names.CONTENT_TYPE; import java.io.IOException; import java.net.URI; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -63,7 +63,7 @@ import io.netty.util.CharsetUtil; public class HttpUploadServerHandler extends SimpleChannelInboundHandler { - private static final Logger logger = Logger.getLogger(HttpUploadServerHandler.class.getName()); + private static final Logger logger = Logger.getLogger(HttpUploadServerHandler.class); private static final HttpDataFactory factory = new DefaultHttpDataFactory(true); @@ -79,16 +79,46 @@ public class HttpUploadServerHandler extends SimpleChannelInboundHandler getUploadRequestUsefulHeaders(HttpHeaders headers) { + Map headerMap = new HashMap<>(); + for (Entry entry : headers) { + UploadHeader headerType = UploadHeader.fromName(entry.getKey()); + if (headerType != null) { + headerMap.put(headerType, entry.getValue()); + } + } + for (UploadHeader type : UploadHeader.values()) { + logger.info(String.format("HEADER: %s=%s", type, headerMap.get(type))); + } + return headerMap; + } + public HttpUploadServerHandler(NfsSecondaryStorageResource storageResource) { this.storageResource = storageResource; } @@ -123,30 +153,8 @@ public void channelRead0(ChannelHandlerContext ctx, HttpObject msg) throws Excep URI uri = new URI(request.getUri()); - String signature = null; - String expires = null; - String metadata = null; - String hostname = null; - long contentLength = 0; - - for (Entry entry : request.headers()) { - if (HEADER_SIGNATURE.equalsIgnoreCase(entry.getKey())) { - signature = entry.getValue(); - } else if (HEADER_METADATA.equalsIgnoreCase(entry.getKey())) { - metadata = entry.getValue(); - } else if (HEADER_EXPIRES.equalsIgnoreCase(entry.getKey())) { - expires = entry.getValue(); - } else if (HEADER_HOST.equalsIgnoreCase(entry.getKey())) { - hostname = entry.getValue(); - } else if (HttpHeaders.Names.CONTENT_LENGTH.equalsIgnoreCase(entry.getKey())) { - contentLength = Long.parseLong(entry.getValue()); - } - } - logger.info("HEADER: signature=" + signature); - logger.info("HEADER: metadata=" + metadata); - logger.info("HEADER: expires=" + expires); - logger.info("HEADER: hostname=" + hostname); - logger.info("HEADER: content-length=" + contentLength); + Map headers = getUploadRequestUsefulHeaders(request.headers()); + long contentLength = headers.get(UploadHeader.CONTENT_LENGTH) != null ? Long.parseLong(headers.get(UploadHeader.CONTENT_LENGTH)) : 0; QueryStringDecoder decoderQuery = new QueryStringDecoder(uri); Map> uriAttributes = decoderQuery.parameters(); uuid = uriAttributes.get("uuid").get(0); @@ -154,9 +162,10 @@ public void channelRead0(ChannelHandlerContext ctx, HttpObject msg) throws Excep UploadEntity uploadEntity = null; try { // Validate the request here - storageResource.validatePostUploadRequest(signature, metadata, expires, hostname, contentLength, uuid); + storageResource.validatePostUploadRequest(headers.get(UploadHeader.SIGNATURE), headers.get(UploadHeader.METADATA), + headers.get(UploadHeader.EXPIRES), headers.get(UploadHeader.HOST), contentLength, uuid); //create an upload entity. This will fail if entity already exists. - uploadEntity = storageResource.createUploadEntity(uuid, metadata, contentLength); + uploadEntity = storageResource.createUploadEntity(uuid, headers.get(UploadHeader.METADATA), contentLength); } catch (InvalidParameterValueException ex) { logger.error("post request validation failed", ex); responseContent.append(ex.getMessage()); @@ -274,7 +283,7 @@ private void writeResponse(Channel channel, HttpResponseStatus statusCode) { response.headers().set(CONTENT_TYPE, "text/plain; charset=UTF-8"); if (!close) { // There's no need to add 'content-length' header if this is the last response. - response.headers().set(CONTENT_LENGTH, buf.readableBytes()); + response.headers().set(HttpHeaders.Names.CONTENT_LENGTH, buf.readableBytes()); } // Write the response. ChannelFuture future = channel.writeAndFlush(response); diff --git a/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java new file mode 100644 index 000000000000..71c77e2e09a1 --- /dev/null +++ b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java @@ -0,0 +1,74 @@ +// 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.cloudstack.storage.resource; + +import java.util.Map; + +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import io.netty.handler.codec.http.DefaultHttpHeaders; +import io.netty.handler.codec.http.HttpHeaders; + +@RunWith(MockitoJUnitRunner.class) +public class HttpUploadServerHandlerTest { + + @Spy + @InjectMocks + HttpUploadServerHandler httpUploadServerHandler = new HttpUploadServerHandler(Mockito.mock(NfsSecondaryStorageResource.class)); + + private void runGetUploadRequestUsefulHeadersTestForHost(String hostHeaderKey, String hostHeaderValue) { + HttpHeaders request = new DefaultHttpHeaders(); + request.add(hostHeaderKey, hostHeaderValue); + Map headers = httpUploadServerHandler.getUploadRequestUsefulHeaders(request); + Assert.assertEquals(hostHeaderValue, headers.get(HttpUploadServerHandler.UploadHeader.HOST)); + } + + @Test + public void testGetUploadRequestUsefulHeadersHeaderKeyDifferentCase() { + String host = "SomeHost"; + runGetUploadRequestUsefulHeadersTestForHost(HttpUploadServerHandler.UploadHeader.HOST.getName(), host); + runGetUploadRequestUsefulHeadersTestForHost(HttpUploadServerHandler.UploadHeader.HOST.getName().toUpperCase(), host); + runGetUploadRequestUsefulHeadersTestForHost("X-Forwarded-Host", host); + } + + @Test + public void testGetUploadRequestUsefulHeadersAllKeys() { + HttpHeaders request = new DefaultHttpHeaders(); + String sign = "Sign"; + String metadata = "met"; + String expires = "ex"; + String host = "SomeHost"; + long contentLength = 100L; + request.add("Signature", sign); + request.add("metadata", metadata); + request.add("X-EXPIRES", expires); + request.add("X-Forwarded-Host", host); + request.add(HttpHeaders.Names.CONTENT_LENGTH, String.valueOf(contentLength)); + Map headers = httpUploadServerHandler.getUploadRequestUsefulHeaders(request); + Assert.assertEquals(sign, headers.get(HttpUploadServerHandler.UploadHeader.SIGNATURE)); + Assert.assertEquals(metadata, headers.get(HttpUploadServerHandler.UploadHeader.METADATA)); + Assert.assertEquals(expires, headers.get(HttpUploadServerHandler.UploadHeader.EXPIRES)); + Assert.assertEquals(host, headers.get(HttpUploadServerHandler.UploadHeader.HOST)); + Assert.assertEquals(String.valueOf(contentLength), headers.get(HttpUploadServerHandler.UploadHeader.CONTENT_LENGTH)); + } +} \ No newline at end of file From b5cbaa81ac59247f18e866f03a523744e675bdb3 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 20 Apr 2023 18:17:14 +0530 Subject: [PATCH 3/7] not needed Signed-off-by: Abhishek Kumar --- .../cloudstack/storage/resource/HttpUploadServerHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java index 959d91ee7515..7189b64824aa 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java @@ -63,7 +63,7 @@ import io.netty.util.CharsetUtil; public class HttpUploadServerHandler extends SimpleChannelInboundHandler { - private static final Logger logger = Logger.getLogger(HttpUploadServerHandler.class); + private static final Logger logger = Logger.getLogger(HttpUploadServerHandler.class.getName()); private static final HttpDataFactory factory = new DefaultHttpDataFactory(true); From b4889825a0ee5247ef1cca083c0b641f3b30c93e Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 20 Apr 2023 18:55:15 +0530 Subject: [PATCH 4/7] Update services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com> --- .../cloudstack/storage/resource/HttpUploadServerHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java index 7189b64824aa..fcbad928ded4 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java @@ -80,7 +80,7 @@ public class HttpUploadServerHandler extends SimpleChannelInboundHandler Date: Thu, 20 Apr 2023 18:55:27 +0530 Subject: [PATCH 5/7] Update services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com> --- .../cloudstack/storage/resource/HttpUploadServerHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java index fcbad928ded4..aec1560f4113 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandler.java @@ -81,7 +81,7 @@ public class HttpUploadServerHandler extends SimpleChannelInboundHandler Date: Thu, 20 Apr 2023 18:56:57 +0530 Subject: [PATCH 6/7] fix Signed-off-by: Abhishek Kumar --- .../storage/resource/HttpUploadServerHandlerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java index 71c77e2e09a1..6658a27e6b5b 100644 --- a/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java +++ b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java @@ -59,8 +59,8 @@ public void testGetUploadRequestUsefulHeadersAllKeys() { String expires = "ex"; String host = "SomeHost"; long contentLength = 100L; - request.add("Signature", sign); - request.add("metadata", metadata); + request.add("x-Signature", sign); + request.add("X-metadata", metadata); request.add("X-EXPIRES", expires); request.add("X-Forwarded-Host", host); request.add(HttpHeaders.Names.CONTENT_LENGTH, String.valueOf(contentLength)); From 83d1f190dc25ee7f49d8fd6e030591fe8b131d9c Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 20 Apr 2023 18:57:44 +0530 Subject: [PATCH 7/7] new line Signed-off-by: Abhishek Kumar --- .../storage/resource/HttpUploadServerHandlerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java index 6658a27e6b5b..4f1ae39842e4 100644 --- a/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java +++ b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/resource/HttpUploadServerHandlerTest.java @@ -71,4 +71,4 @@ public void testGetUploadRequestUsefulHeadersAllKeys() { Assert.assertEquals(host, headers.get(HttpUploadServerHandler.UploadHeader.HOST)); Assert.assertEquals(String.valueOf(contentLength), headers.get(HttpUploadServerHandler.UploadHeader.CONTENT_LENGTH)); } -} \ No newline at end of file +}