Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use origin input key by default #46

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 48 additions & 23 deletions lib/resty/aes.lua
Original file line number Diff line number Diff line change
Expand Up @@ -125,50 +125,75 @@ cipher = function (size, _cipher)
end
_M.cipher = cipher

function _M.new(self, key, salt, _cipher, _hash, hash_rounds)

local options = {
default = 0x01,
key_without_gen = 0x02
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

-- may add other things like padding mode; or use bit 'or/and' operation for combined option
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too long, exceeding 80 columns.

}
_M.options = options

function _M.new(self, key, salt, _cipher, _hash, hash_rounds, option)
local encrypt_ctx = ffi_new(ctx_ptr_type)
local decrypt_ctx = ffi_new(ctx_ptr_type)
local _cipher = _cipher or cipher()
local _hash = _hash or hash.md5
local hash_rounds = hash_rounds or 1
local option = option or options.default
local _cipherLength = _cipher.size/8
local gen_key = ffi_new("unsigned char[?]",_cipherLength)
local gen_iv = ffi_new("unsigned char[?]",_cipherLength)

if type(_hash) == "table" then
if not _hash.iv or #_hash.iv ~= 16 then
return nil, "bad iv"
end
if option == options.default then
if type(_hash) == "table" then
if not _hash.iv or #_hash.iv ~= 16 then
return nil, "bad iv"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

if _hash.method then
local tmp_key = _hash.method(key)

if _hash.method then
local tmp_key = _hash.method(key)
if #tmp_key ~= _cipherLength then
return nil, "bad key length"
end

if #tmp_key ~= _cipherLength then
ffi_copy(gen_key, tmp_key, _cipherLength)

elseif #key ~= _cipherLength then
return nil, "bad key length"
end

ffi_copy(gen_key, tmp_key, _cipherLength)
else
ffi_copy(gen_key, key, _cipherLength)
end

elseif #key ~= _cipherLength then
return nil, "bad key length"
ffi_copy(gen_iv, _hash.iv, 16)

else
ffi_copy(gen_key, key, _cipherLength)
end
if salt and #salt ~= 8 then
return nil, "salt must be 8 characters or nil"
end

ffi_copy(gen_iv, _hash.iv, 16)
if C.EVP_BytesToKey(_cipher.method, _hash, salt, key, #key,
hash_rounds, gen_key, gen_iv)
~= _cipherLength
then
return nil
end
end

else
if salt and #salt ~= 8 then
return nil, "salt must be 8 characters or nil"
elseif option == options.key_without_gen then
-- use origin key
if not key then
return nil, "key is nil!"
end

if C.EVP_BytesToKey(_cipher.method, _hash, salt, key, #key,
hash_rounds, gen_key, gen_iv)
~= _cipherLength
then
return nil
if #key > _cipherLength then
return nil, "key too long"
end

-- note it's key padding not data padding, and it use zero padding only;
key = key..string.rep("\0", _cipherLength - #key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ffi_copy(gen_key, key, _cipherLength)
end

C.EVP_CIPHER_CTX_init(encrypt_ctx)
Expand Down
21 changes: 21 additions & 0 deletions t/aes.t
Original file line number Diff line number Diff line change
Expand Up @@ -293,3 +293,24 @@ failed to new: bad iv
--- no_error_log
[error]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

=== TEST 11: AES key_without_gen hello
--- http_config eval: $::HttpConfig
--- config
location /t {
content_by_lua '
local aes = require "resty.aes"
local str = require "resty.string"
local aes_default = aes:new("secret", nil, nil, nil, nil, aes.options.key_without_gen)
local encrypted = aes_default:encrypt("hello")
ngx.say("AES-128 CBC MD5: ", str.to_hex(encrypted))
local decrypted = aes_default:decrypt(encrypted)
ngx.say(decrypted == "hello")
';
}
--- request
GET /t
--- response_body
AES-128 CBC MD5: c290e8b6de4ea5773414e019fe7f17a3
true
--- no_error_log
[error]