Separate tests for Signature and Authorization headers

[liamdennehy]

At present all sign and verify commands seem to operate only on the Authorization header. In practise this header is usually only used for authentication, while Signature implies content protection as spelled out in v11. Indeed a resource may require an Authorization: Bearer to be preserved while the request is signed with Signature (APIs exist today with this requirement), or use Authorization: Signature to authorise a request containing content protected by a Signature from another party.

This suite should include:

• Separate sign modes for each header
• Separate verify modes for each header (including not failing on non-signature Authorization header being present)
• Verification that a sign action does not affect Authorization header, and vice versa.

[aljones15]

I think I can handle this by adding a new option to the binary maybe

  --header (Signature|Authorization)

Our existing project should be able to handle it.
As for verifying that the sign action does affect the Authorization header and vice versa not sure on that one. That might be a better assertion to canonicalize as we could pass in a Signature and Authorization header with conflicting values and see what happens.

[liamdennehy]

we could pass in a Signature and Authorization header

I noticed some "sign" tests pass in an incomplete header, expecting the implementation to parse and correct it. I don't see the utility here - in general a library will simply receive a command to sign/authorise and make the entire header. What real-world scenario are we reflecting here where a signer would start with an incomplete header?

Surely we should just pass in the instruction "sign this message in this header with this keyid..." rather than expecting them to first parse a bad header - something the library is designed to reject anyway? Otherwise there's a lot of work an implementer needs to do to parse the input just to pass the test...

As for not impacting the opposite header, it doesn't even need to be valid under this spec, just that 1) it exists and has a value, and 2) the implementation spits it out exactly as was provided...

[aljones15]

Could you link me to one of the HTTP messages with an incomplete header? I will definitely correct any HTTP Messages with invalid headers.

[liamdennehy]

As of 947cdc6, for this invocation:

generator.sh sign --private-key <file> --headers date --algorithm hs2019 --key-type rsa

The generator receives the following on stdin:

GET /basic/request HTTP/1.1
Connection: keep-alive
User-Agent: Mozilla/5.0 (Macintosh)
authorization: Signature keyId="foo",algorithm="hmac",signature="test"
Date: Tue, 13 Aug 2019 14:34:53 GMT

Since the keyId is not provided on the command line, I presume I'm supposed to deduce it from the supplied (invalid) Authorization header, then combine it with the command-line parameters and replace the existing header?

[aljones15]

This is invalid because authorization is lower case (which I am going to correct) or another reason?

p.s. commit for capitalizing Authorization is here: 616dd63

Was anything else invalid because obviously the binary I'm using is parsing the message just fine.

[liamdennehy]

This is invalid because authorization is lower case... ?

According to HTTP spec, headers are not case-sensitive. If anything varying the case in tests add robustness - a lowercase authorization or signature header should still validate.

This is invalid because ... another reason?

In terms of the test case, yes.

1. The invocation does not include the keyId, so I don't have enough info to generate a valid signature header from the command-line parameters alone. Is this test expecting a fail because keyId is not provided, or extract the right value it from the supplied header?
2. If the latter, no real-world scenario would start with what looks like a signature header to get parameters. I would generally invoke with "sign this message", not "read this partially/badly signed message and adjust".

Essentially: Why are you feeding authorization: Signature into the generator in sign mode in the first place?

[aljones15]

@liamdennehy

1. keyId is essentially not used in the tests. A path is passed in to the public key for sign and the private key for verify. Hence dereferncing from keyId is not really necessary. I kept the keyId there because it's required and would often result in an error. I have now added to it to the sign tests (the verify tests already had it).

2. I tried removing Authorization from the headers as it appears it is causing confusion, but this caused some of the verify tests to fail. Perhaps this is bad test design as the sign httpMessages should probably be separated from the verify ones.

[liamdennehy]

Ah, I see the confusion.

keyId is mandatory, but the test invocation doesn't supply it so I have no value to provide. So, I assumed it needed to be the value in the provided header.

Dereferencing isn't possible most times, as the value may have no relation to the key value. A missing keyId in a provided header MUST produce an error, and a verifier needs it, so there's no clarity on how to pass.

[aljones15]

thanks so much for this issue I have removed the Authorization header entirely from the sign tests, added keyId to the parameters passed to sign, and also refactored to cut down on the number of httpMessages used in the tests. Let me know if I can close this unless you feel we still need an Authorization/ Signature toggle. oh wait I'm sorry we have not gotten to the verify tests yet so let me go through them. Sorry man a little under the weather today.

[liamdennehy]

Definitely still need the toggle.

There are production APIs today that need a correct Signature header alongside an Authorization header of an entirely different type. Implementations need to generate only the type requested and preserve the existing one, so we need to test for that.

[aljones15]

what would make more sense for the name of the option:

--construct Signature
--header Signature
--mode Authorization

I prefer construct.

[liamdennehy]

Tried seeing if the spec has clarity, but it actually treats the two modes as different worlds with hardly any language in common. "mode" is already taken to indicate canon/sign/verify (at least I've proposed it in (#14) so that would only be confusing, and "header" is too similar to "headers".

I think "construct" is reasonable. Let me know when it's implemented and I'll see if I can write up the instructions in the README if you'd like.

[aljones15]

@msporny has suggested

--add <addHeader>