#98 - end to end testing of AWS Signature 4 based on AWS test suite #120

devsprint · 2016-12-16T12:28:26Z

…t validation.

…quest validation.

lightbend-cla-validator · 2016-12-16T12:28:26Z

Hi @devsprint,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

ktoso · 2016-12-16T12:48:56Z

Refs #98
Thanks, not sure if I'll be able to review today but hope someone might be able to :)

devsprint · 2016-12-16T15:04:44Z

CLA signed

… Scala test suite refactoring, in prgress.

…f the code.

patriknw

LGTM

patriknw · 2016-12-21T13:49:18Z

@agolubev is this something you would like to review?

agolubev · 2016-12-21T14:21:56Z

@patriknw thanks for the notification. I'll review this today

agolubev

PR has a lot of header manipulation and seems like 3 or 4 akka-http problems were identified. Would be good to have them as tickets with ticket number in code at least

agolubev · 2016-12-23T21:21:28Z

s3/src/main/scala/akka/stream/alpakka/s3/auth/CanonicalRequest.scala

-  private def uriEncode(str: String) = URLEncoder.encode(str, "utf-8")
+  private def encodeQueryParams(name: String, value: String): String =
+    if (name.contains(' ')) {
+      s"${uriEncode(name.takeWhile(_ != ' ')).replace("%7E", "~")}="


should this be changed to just string append?

agolubev · 2016-12-23T21:22:09Z

s3/src/main/scala/akka/stream/alpakka/s3/auth/CanonicalRequest.scala

+
+  private def uriEncode(str: String) = if (isAlreadyURLEncoded(str)) str else URLEncoder.encode(str, "utf-8")
+
+  // TODO: this could be removed when the Uri class in akka-http will accept utf-8


I was not able to find appropriate ticket in akka-http. Should one be created?

akka/akka-http#86 - this is a similar issue, but for Cyrillic URIs

could you add ticket number to comment into code?

Sure. it will be in next push

agolubev · 2016-12-23T21:31:56Z

s3/src/test/scala/akka/stream/alpakka/s3/auth/SignerSpec4.scala

+    whenReady(stsResult) { builtSts =>
+      withClue(s"According to the expected string to sign stored in ${sts._1}") {
+        if (sts._1 == "post-x-www-form-urlencoded.sts") {
+          // TODO: skip this test because we can't avoid adding charset into http request as the test suite require.


do we need to consider this as http ticket?

akka/akka-http#689

agolubev · 2016-12-23T21:31:56Z

s3/src/test/scala/akka/stream/alpakka/s3/auth/SignerSpec4.scala

+    probes.get.foreach {
+      case (fileName, expectedResult) =>
+        testCanonicalRequest(fileName, expectedResult)
+    }


All 4 test cases are different only with testMethod and expected result - can this be a single function with results collection and test function as arguments?

agolubev · 2016-12-23T21:38:44Z

s3/src/test/scala/akka/stream/alpakka/s3/auth/SignerSpec4.scala

+   * @return the Uri
+   */
+  private def encodeIfRequired(rawUri: String): Uri = {
+    val pattern = """^([!#$&-;=?-\[\]_a-z~]|%[0-9a-fA-F]{2})+$$""".r


need some explanation here, maybe comments

agolubev · 2016-12-23T21:38:47Z

s3/src/test/scala/akka/stream/alpakka/s3/auth/SignerSpec4.scala

+   */
+  private def encodeIfRequired(rawUri: String): Uri = {
+    val pattern = """^([!#$&-;=?-\[\]_a-z~]|%[0-9a-fA-F]{2})+$$""".r
+    // TODO: this look like a bug in Uri parser in akka-http ("//" is generating empty path)


Another akka-http ticket?

akka/akka-http#690

agolubev · 2016-12-23T21:42:18Z

s3/src/test/scala/akka/stream/alpakka/s3/auth/SignerSpec4.scala

+      if (current.contains(':')) {
+        current :: acc
+      } else {
+        // TODO: trim and ',' has been added to pass the test, but it looks like a bug in HttpParser code from akka-http.


same as above

agolubev · 2016-12-23T21:49:24Z

In general - looks good

…lated issues.

devsprint · 2016-12-27T11:57:18Z

@agolubev please take a look to last PR.

agolubev

Looks good. Thanks!

jrudolph · 2016-12-29T13:59:48Z

s3/src/main/scala/akka/stream/alpakka/s3/auth/CanonicalRequest.scala

  }

+  private def fixContentTypeHeaderParameter(headers: String): String =
+    headers.replace("charset=UTF-8", "charset=utf8")


This will break signing, as you sign a different request than what akka-http will send.

jrudolph · 2016-12-29T14:08:52Z

I think this approach cannot work because not all of the test cases (the .req files) can be represented verbatim by the akka-http model. In the end, the point of the akka-http model is to remove many of the redundant representations by a single typed model which makes it harder to get many of the representation details wrong.

If you want to use the test cases, you need to check in the first step what the actually requests are that akka-http will send out and check these against the original test case. This will probably need to use HttpRequestRenderer or actual end-to-end testing by running the request through the complete Http.clientLayer to figure out what the actual request is. In many cases, it will not be the same as what the test cases need. The problem is then, that you cannot use the provided test data because you cannot make akka-http to send them unchanged, so you need to sign something else.

Worse, the changes to the signer might actually break signing because the canonical representation doesn't seem to be the same as what will be sent through akka-http, so that AWS will have to reject requests which were manipulated by the signer.

jrudolph · 2016-12-29T14:10:03Z

s3/src/main/scala/akka/stream/alpakka/s3/auth/CanonicalRequest.scala

+      .replace(":", "%3A")
+      .replace("%2F", "/")
+      .replace("%7E", "~")
+      .replace("+", "%20")

  def canonicalHeaderString(headers: Seq[HttpHeader]): String = {
    val grouped = headers.groupBy(_.lowercaseName())


This seems to be missing the value of req.entity.contentType which is encoded specially in the akka-http model.

devsprint · 2016-12-29T14:38:03Z

@jrudolph hmm, I understand your reasons and thanks for clarifications. Some parts of the test data from Amazon will not work with current constrains, but this is a small part of it, the ones that are having a body, where most of them are not having any. For this I will work out how to adapt it such that we can use it with smaller changes (.req, .creq.. changes). There are 2 test data sets that are failing (POST-x-www-form-urlencoded and POST-x-www-form-urlencoded-parameters). Are you ok with this approach ?

patriknw · 2017-01-13T09:06:17Z

Not sure how important the comment is, do you know @ktoso ?
Can we merge and do a follow up?

devsprint · 2017-01-13T09:07:44Z

@patriknw I will update the pull request based on @jrudolph comments but I will be only able to do it over the week-end.

patriknw · 2017-01-13T09:09:00Z

Ok, thanks for the update, then I'll leave this open

ktoso · 2017-01-13T09:15:13Z

s3/src/test/scala/akka/stream/alpakka/s3/auth/SignerSpec4.scala

+/**
+ * Load the probes files from resource folder.
+ */
+object Probes {


Probes is a bit of a weird name, in Akka probes are TestProbes hm.

ktoso · 2017-01-13T09:18:45Z

Agree with @jrudolph's both contents. We must use the actual APIs to verify the test actually is testing what will be put onto the wire. Now it's a weird mix of hand constructing things which may be not the way we'd render these - please make the .req files into actual Akka HTTP model types directly. Where it's not possible we need to fix something or provide workarounds.

patriknw · 2017-02-05T21:14:40Z

@devsprint any news here?

jrudolph · 2017-03-28T14:22:31Z

Sorry for keeping this so long.

I think the bottom line here is:

it is impossible to implement the AWS test suite including the hashes given by the test suite because some of the test cases cannot be rendered by akka-http (that's by design, semantically equivalent headers can be rendered)
it is likely that this PR will break signing because the signer is changed in a way to make the tests pass (but the tests do not assert what akka-http renders)

Here's a sketch how to implement signing tests:

provide or generate a set of interesting test requests as akka-http HttpRequest (internal HttpRequestParser infrastructure could be used to parse the AWS test files, signatures of the test files cannot be used)
run each of the requests through akka-http's internal HttpRequestRenderer or use Http.clientLayer() flow (connect all ports of the BidiFlow to probes and then push a request and read the network side, to get access to the actual wire rendering of the request
use any third party library to sign this request data (which will involve parsing the request with whatever the third-party tool provides)
check that the alpakka AWS signer generates the same signature

Not sure if it makes sense to keep this PR open as the real solution might not have much in common with what we have here.

WDYT?

devsprint · 2017-03-28T14:25:47Z

@jrudolph I have tried to find a way to still use the test cases from Amazon but as you also obeserved is it imposible to use them. I think that the right option is to close this PR.

jrudolph · 2017-03-28T15:50:39Z

Ok. Let's close it for now. I'll add a comment in the ticket to see the comments here before tackling it again. Anyway, thanks for your work, @devsprint!

akka#98 WIP - implemented the validation for first step. Canonical Re…

161c96a

…quest validation.

devsprint added 4 commits December 18, 2016 06:54

akka#98 - WIP - completed end to end test suite, refactored the code.…

e9c9bb3

… Scala test suite refactoring, in prgress.

akka#98 -WIP - added more comments, removed unused code.

b917b21

akka#98 - completed end to end testing and improved the readability o…

e014d7d

…f the code.

corrected typo

7ce0424

devsprint changed the title ~~#98 WIP - implemented the validation for first step. Canonical Reques…~~ #98 - end to end testing of AWS Signature 4 based on AWS test suite Dec 21, 2016

patriknw approved these changes Dec 21, 2016

View reviewed changes

agolubev reviewed Dec 23, 2016

View reviewed changes

devsprint mentioned this pull request Dec 26, 2016

Allow HttpRequest entity content-type without parameters. akka/akka-http#689

Open

refactored test code as suggested and add referrences to akka-http re…

d212183

…lated issues.

agolubev approved these changes Dec 28, 2016

View reviewed changes

jrudolph reviewed Dec 29, 2016

View reviewed changes

ktoso reviewed Jan 13, 2017

View reviewed changes

brianmowen mentioned this pull request Feb 9, 2017

Add AWS Lex Support #178

Closed

patriknw assigned jrudolph Mar 20, 2017

jrudolph closed this Mar 28, 2017

jrudolph mentioned this pull request Mar 28, 2017

Implement Amazon's Request-signing Test Suite #98

Open


		private def uriEncode(str: String) = if (isAlreadyURLEncoded(str)) str else URLEncoder.encode(str, "utf-8")

		// TODO: this could be removed when the Uri class in akka-http will accept utf-8

#98 - end to end testing of AWS Signature 4 based on AWS test suite #120

#98 - end to end testing of AWS Signature 4 based on AWS test suite #120

Conversation

devsprint commented Dec 16, 2016

lightbend-cla-validator commented Dec 16, 2016

ktoso commented Dec 16, 2016

devsprint commented Dec 16, 2016

patriknw left a comment

Choose a reason for hiding this comment

patriknw commented Dec 21, 2016

agolubev commented Dec 21, 2016

agolubev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsprint Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agolubev commented Dec 23, 2016

devsprint commented Dec 27, 2016

agolubev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrudolph commented Dec 29, 2016

Choose a reason for hiding this comment

devsprint commented Dec 29, 2016 • edited Loading

patriknw commented Jan 13, 2017

devsprint commented Jan 13, 2017

patriknw commented Jan 13, 2017

Choose a reason for hiding this comment

ktoso commented Jan 13, 2017

patriknw commented Feb 5, 2017

jrudolph commented Mar 28, 2017

devsprint commented Mar 28, 2017

jrudolph commented Mar 28, 2017

devsprint Dec 26, 2016 •

edited

Loading

devsprint commented Dec 29, 2016 •

edited

Loading