fix(hmac) handle invalid Base64-encoded signatures #2283
Conversation
|
see also #2189 |
|
Yep, thanks @Tieske. This PR would obviate that one. |
p0pr0ck5
force-pushed
the
fix/hmac-digest-equals
branch
2 times, most recently
from
3327946 to
fd1627b
Compare
| @@ -108,6 +108,21 @@ describe("Plugin: hmac-auth (access)", function() | |||
| assert.equal(SIGNATURE_NOT_VALID, body.message) | |||
| end) | |||
|
|
|||
| it("show not be authorized when the HMAC signature is properly base64 encoded", function() | |||
There was a problem hiding this comment.
"is not properly base64 encoded" ?
p0pr0ck5
force-pushed
the
fix/hmac-digest-equals
branch
from
fd1627b to
5e278e4
Compare
| local body = assert.res_status(403, res) | ||
| body = cjson.decode(body) | ||
| assert.equal(SIGNATURE_NOT_VALID, body.message) | ||
| end) |
There was a problem hiding this comment.
As @Tieske pointed out in the other PR, this test does indeed pass without the fix. Hmmm 🤔
There was a problem hiding this comment.
Incorrectly written test. The value to be tested is the signature portion of the (proxy) authorization header, not the whole authorization header.
And also remember this new test would still not fail with the existing codebase, because digest_2 is never examined.
There was a problem hiding this comment.
noting here for posterity because github doesnt track patchset versions in PR with amended commits:
@thibaultcha's comment refers to the face that previously the authorization header was not a properly encoded base64 value. the purpose of the test is to determine the behavior of the module when the signature portion of the header is not properly encoded, not that the whole header value is an invalid encoding.
There was a problem hiding this comment.
And also remember this new test would still not fail with the existing codebase, because digest_2 is never examined.
Yes, noticed that as well. Quite the surprise.
There was a problem hiding this comment.
it would have still failed if digests were different. Just the initial length check was wrong.
p0pr0ck5
force-pushed
the
fix/hmac-digest-equals
branch
from
5e278e4 to
1f6b7e4
Compare
| it("should not be authorized when the HMAC signature is not properly base64 encoded", function() | ||
| local date = os.date("!%a, %d %b %Y %H:%M:%S GMT") | ||
| local hmacAuth = [["hmac username="bob",algorithm="hmac-sha1",]] | ||
| ..[[headers="date",signature="not really a base64 encoded value!!!"]] |
There was a problem hiding this comment.
Looks much better, I just ran a few debug statements and realized what the test was doing 😅
p0pr0ck5
force-pushed
the
fix/hmac-digest-equals
branch
from
1f6b7e4 to
148ed47
Compare
|
shouldn't this be against |
|
@Tieske why? This doesn't feel like a super critical hotfix. |
p0pr0ck5
force-pushed
the
fix/hmac-digest-equals
branch
from
148ed47 to
c70d6a8
Compare
We don't need to try to compare against an empty string. Additionally, remove the needless length comparison. This was bugged to begin with, but the length comparison is needless, and doing so removes the constant time factor from this function.
p0pr0ck5
force-pushed
the
fix/hmac-digest-equals
branch
from
c70d6a8 to
19e2e79
Compare
|
re-targeted and rebased to merge into master |
We don't need to try to compare against an empty string.
Additionally, remove the needless length comparison. This was bugged
to begin with, but the length comparison is needless, and doing so
removes the constant time factor from this function.