use origin input key by default

[haorenfsa]

this patch is mean to fix issues like #28, #19
and it's now compatite with openssl shell command and php
note it's not related to pull request #35 : it's only about the key, not data.

things might concern :

1. to compatite with former aes.lua version, one has to explicit write what hash method he/she need when using new() function, which is by default _M.hash.md5;

eg:

aes = require "resty.aes"
aes:new("key", "salt", some_cipher, hash.md5, ....)

2. readme might need to update

[haorenfsa]

eh, I'll take a look at test problems later

[agentzh]

@haorenfsa This would break backward compatibility, right? Better avoid breaking it even though the current impl is wrong.

[agentzh]

@chase Will you please review this PR? @haorenfsa found that you mistakenly use the MD5 digest of the user key instead of the user key itself in the resty.aes module.

[haorenfsa]

@agentzh okey, I'll check it later

[haorenfsa]

the CI test seems passed, but still showing pending

[agentzh]

@haorenfsa Yeah, seems like a github or travis bug.

[agentzh]

We should use 4-space indentations consistently. Please fix other similar places, if any.

[agentzh]

We always use 3 blank lines to separate test blocks. Please use the reindex script to auto-format this .t file. Thank you.

[agentzh]

This line is too long, exceeding 80 columns.

[agentzh]

Better add a trailing comma to simplify adding more options in the future.

[agentzh]

Also, the option name"key_without_gen" is confusing. What does it mean?

BTW, shall we add some docs to README for this change or feature?

[haorenfsa]

about "key_without_gen" option
By default, new() function in aes.lua uses EVP_BytesToKey in openssl to generate key and iv from input key together with salt and etc by md5. Use generated key can avoid some attacks.
But sometimes people just want to use simple key without iv and generation instead, for compatibility or other reason.
so how should we name it? use_origin_key?

about docs
Of course we should. It takes me quite a while when I crash into this.

[agentzh]

@haorenfsa use_raw_key might be a better name? Also, maye we do not need the default option at all?

[haorenfsa]

yeah，it's better. And default can be replaced with nil input

[agentzh]

Please avoid this global variable string. We should avoid use of Lua globals on such Lua hot code paths. We should write

local str_rep = string.rep

function blah(...)
    str_rep(...)
end

This way we also save 2 table looks on hot code paths, one is the table lookup of the key "string" in the global environment table, and the other is the lookup of the key "rep" in the "string" table.

[hy05190134]

LGTM, and I apply the patch for my aes.lua

[wenxzhen]

which version will include this change?

[chase]

If I recall correctly, the use of MD5 was for compatibility of some library or language which I cannot place exactly.

Security-wise, what I committed is quite a big mistake, even if it was for the sake of compatibility. If this matches the valid behavior of the most commonly integrated tools/languages, it should be merged.

That said, I do not have time to help maintain the code I contributed or do code reviews, unfortunately.

[haorenfsa]

almost forgot about this patch... I'll commit a update for tutorial in this patch later this week, and is there anything else need to be altered? @agentzh

@wenxzhen my team has been using this patch in production environment for several months, and it is doing well.

[wenxzhen]

Thanks to @shaoyue

…

On Tue, Nov 28, 2017 at 7:47 PM, shaoyue ***@***.***> wrote: almost forgot about this patch... I'll commit a update for tutorial in this patch later this week, and is there anything else need to be altered? @agentzh <https://github.com/agentzh> @wenxzhen <https://github.com/wenxzhen> my team has been using this patch in production environment for several months, and it is doing well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB-dn5LTegy1i5-tjMLk0HPozp8rCw0Aks5s6_LhgaJpZM4O8OP4> .

[haorenfsa]

@agentzh plz review again when you can spare some time? thanks for the attention

[agentzh]

@haorenfsa I'll look into this as soon as I can manage. Sorry for the delay on my side.

[fancy-rabbit]

same problem. I'm trying to decrypt data encrypted by crypto-js(AES-256, ECB) and worked around it with the following code
local cipher = aes:new(key_encryption_key, nil, aes.cipher(256, 'ecb'), {method = function(key)return key end, iv = string.rep(string.char(0), 16)})
it will always call _hash.method (default MD5) on the key, thus a dummy method function may worked well. (although IV is not used in ECB mode but a dummy one should be provided, too)