Skip to content
This repository has been archived by the owner on Jun 30, 2021. It is now read-only.

Switch to SHA2 for secret key hashing #303

Merged
merged 2 commits into from
Jul 5, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion apps/admin_api/lib/admin_api/invites/inviter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ defmodule AdminAPI.Inviter do
{:ok, user} =
User.insert(%{
email: email,
password: Crypto.generate_key(32)
password: Crypto.generate_base64_key(32)
})

user
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ defmodule AdminAPI.V1.AdminAPIAuthPlugTest do

describe "AdminAPIAuthPlug.call/2 with provider auth" do
test "authenticates if both access key and secret key are correct" do
conn = test_with("OMGProvider", @access_key, @secret_key)
conn = test_with("OMGProvider", @access_key, Base.url_encode64(@secret_key))

refute conn.halted
assert conn.assigns.authenticated == true
Expand Down
3 changes: 2 additions & 1 deletion apps/admin_api/test/support/conn_case.ex
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,11 @@ defmodule AdminAPI.ConnCase do
def provider_request(path, data \\ %{}, opts \\ [])
when is_binary(path) and byte_size(path) > 0 do
{status, _opts} = Keyword.pop(opts, :status, :ok)
secret_key = Base.url_encode64(@secret_key)

build_conn()
|> put_req_header("accept", @header_accept)
|> put_auth_header("OMGProvider", @access_key, @secret_key)
|> put_auth_header("OMGProvider", @access_key, secret_key)
|> post(@base_dir <> path, data)
|> json_response(status)
end
Expand Down
2 changes: 1 addition & 1 deletion apps/ewallet_db/lib/ewallet_db/api_key.ex
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ defmodule EWalletDB.APIKey do
attrs =
attrs
|> Map.put_new_lazy(:account_uuid, fn -> get_master_account_uuid() end)
|> Map.put_new_lazy(:key, fn -> Crypto.generate_key(@key_bytes) end)
|> Map.put_new_lazy(:key, fn -> Crypto.generate_base64_key(@key_bytes) end)

%APIKey{}
|> changeset(attrs)
Expand Down
2 changes: 1 addition & 1 deletion apps/ewallet_db/lib/ewallet_db/auth_token.ex
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ defmodule EWalletDB.AuthToken do
owner_app: Atom.to_string(owner_app),
user_uuid: user.uuid,
account_uuid: if(account, do: account.uuid, else: nil),
token: Crypto.generate_key(@key_length)
token: Crypto.generate_base64_key(@key_length)
}

insert(attrs)
Expand Down
2 changes: 1 addition & 1 deletion apps/ewallet_db/lib/ewallet_db/forget_password_request.ex
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ defmodule EWalletDB.ForgetPasswordRequest do
Generates a forget password request for the given user.
"""
def generate(user) do
token = Crypto.generate_key(@token_length)
token = Crypto.generate_base64_key(@token_length)
{:ok, _} = insert(%{token: token, user_uuid: user.uuid})
ForgetPasswordRequest.get(user, token)
end
Expand Down
31 changes: 28 additions & 3 deletions apps/ewallet_db/lib/ewallet_db/helpers/crypto.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,38 @@ defmodule EWalletDB.Helpers.Crypto do
"""
alias Plug.Crypto

@spec generate_key(non_neg_integer()) :: binary()
def generate_key(key_bytes) when is_integer(key_bytes) do
@spec generate_base64_key(non_neg_integer()) :: binary()
def generate_base64_key(key_bytes) when is_integer(key_bytes) do
key_bytes
|> :crypto.strong_rand_bytes()
|> generate_key()
|> Base.url_encode64(padding: false)
end

@spec generate_key(non_neg_integer()) :: binary()
def generate_key(key_bytes) when is_integer(key_bytes) do
:crypto.strong_rand_bytes(key_bytes)
end

@spec hash_secret(String.t()) :: String.t()
def hash_secret(secret) do
:crypto.hash(:sha384, secret)
|> Base.encode16(padding: false)
|> String.downcase()
end

@spec verify_secret(String.t(), String.t()) :: boolean
def verify_secret(secret, hash) do
case Base.url_decode64(secret) do
{:ok, decoded} ->
decoded
|> hash_secret()
|> secure_compare(hash)

:error ->
false
end
end

@spec hash_password(nil) :: nil
def hash_password(nil), do: nil

Expand Down
2 changes: 1 addition & 1 deletion apps/ewallet_db/lib/ewallet_db/invite.ex
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ defmodule EWalletDB.Invite do
"""
def generate(user, opts \\ [preload: []]) do
# Insert a new invite
{:ok, invite} = insert(%{token: Crypto.generate_key(@token_length)})
{:ok, invite} = insert(%{token: Crypto.generate_base64_key(@token_length)})

# Assign the invite to the user
changeset = change(user, invite_uuid: invite.uuid)
Expand Down
10 changes: 6 additions & 4 deletions apps/ewallet_db/lib/ewallet_db/key.ex
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ defmodule EWalletDB.Key do
@primary_key {:uuid, UUID, autogenerate: true}
# String length = ceil(key_bytes / 3 * 4)
@key_bytes 32
@secret_bytes 128

schema "key" do
external_id(prefix: "key_")
Expand All @@ -37,7 +38,8 @@ defmodule EWalletDB.Key do
|> cast(attrs, [:access_key, :secret_key, :account_uuid])
|> validate_required([:access_key, :secret_key, :account_uuid])
|> unique_constraint(:access_key, name: :key_access_key_index)
|> put_change(:secret_key_hash, Crypto.hash_password(attrs[:secret_key]))
|> put_change(:secret_key_hash, Crypto.hash_secret(attrs[:secret_key]))
|> put_change(:secret_key, Base.url_encode64(attrs[:secret_key], padding: false))
|> assoc_constraint(:account)
end

Expand Down Expand Up @@ -83,8 +85,8 @@ defmodule EWalletDB.Key do
attrs =
attrs
|> Map.put_new_lazy(:account_uuid, fn -> get_master_account_uuid() end)
|> Map.put_new_lazy(:access_key, fn -> Crypto.generate_key(@key_bytes) end)
|> Map.put_new_lazy(:secret_key, fn -> Crypto.generate_key(@key_bytes) end)
|> Map.put_new_lazy(:access_key, fn -> Crypto.generate_base64_key(@key_bytes) end)
|> Map.put_new_lazy(:secret_key, fn -> Crypto.generate_key(@secret_bytes) end)

%Key{}
|> changeset(attrs)
Expand Down Expand Up @@ -122,7 +124,7 @@ defmodule EWalletDB.Key do
end

def authenticate(%{secret_key_hash: secret_key_hash} = key, secret) do
case Crypto.verify_password(secret, secret_key_hash) do
case Crypto.verify_secret(secret, secret_key_hash) do
true -> {:ok, key}
_ -> false
end
Expand Down
6 changes: 3 additions & 3 deletions apps/ewallet_db/test/ewallet_db/helpers/crypto_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ defmodule EWalletDB.Helpers.CryptoTest do
use ExUnit.Case
alias EWalletDB.Helpers.Crypto

describe "generate_key/1" do
describe "generate_base64_key/1" do
test "returns a key with the specified length" do
key = Crypto.generate_key(32)
key = Crypto.generate_base64_key(32)
# ceil(32 * 4 / 3)
assert String.length(key) == 43

# Test with another length to make sure it's not hardcoded.
key = Crypto.generate_key(64)
key = Crypto.generate_base64_key(64)
# ceil(64 * 4 / 3)
assert String.length(key) == 86
end
Expand Down
19 changes: 12 additions & 7 deletions apps/ewallet_db/test/ewallet_db/key_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,19 @@ defmodule EWalletDB.KeyTest do
test_insert_generate_external_id(Key, :id, "key_")
test_insert_generate_timestamps(Key)
test_insert_generate_length(Key, :access_key, 43)
test_insert_generate_length(Key, :secret_key, 43)
test_insert_generate_length(Key, :secret_key, 171)
test_insert_prevent_duplicate(Key, :access_key)

test "hashes secret_key with bcrypt before saving" do
test "hashes secret_key with sha384 and hex digest it before saving" do
{res, key} = Key.insert(params_for(:key, %{secret_key: "my_secret"}))

hashed =
:crypto.hash(:sha384, "my_secret")
|> Base.encode16(padding: false)
|> String.downcase()

assert res == :ok
assert "$2b$" <> _ = key.secret_key_hash
assert hashed == key.secret_key_hash
refute key.secret_key == key.secret_key_hash
end

Expand All @@ -97,7 +102,7 @@ defmodule EWalletDB.KeyTest do
})
|> Key.insert()

{res, key} = Key.authenticate("access123", "secret321")
{res, key} = Key.authenticate("access123", Base.url_encode64("secret321"))
assert res == :ok
assert %Key{} = key
assert key.account.uuid == account.uuid
Expand All @@ -108,9 +113,9 @@ defmodule EWalletDB.KeyTest do
|> params_for(%{access_key: "access123", secret_key: "secret321"})
|> Key.insert()

assert Key.authenticate("access123", "unmatched") == false
assert Key.authenticate("unmatched", "secret321") == false
assert Key.authenticate("unmatched", "unmatched") == false
assert Key.authenticate("access123", Base.url_encode64("unmatched")) == false
assert Key.authenticate("unmatched", Base.url_encode64("secret321")) == false
assert Key.authenticate("unmatched", Base.url_encode64("unmatched")) == false
end

test "returns nil if access_key and/or secret_key is nil" do
Expand Down
6 changes: 3 additions & 3 deletions apps/ewallet_db/test/support/factory.ex
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ defmodule EWalletDB.Factory do

def invite_factory do
%Invite{
token: Crypto.generate_key(32)
token: Crypto.generate_base64_key(32)
}
end

Expand Down Expand Up @@ -164,8 +164,8 @@ defmodule EWalletDB.Factory do

%Key{
access_key: access_key,
secret_key: secret_key,
secret_key_hash: Crypto.hash_password(secret_key),
secret_key: Base.url_encode64(secret_key),
secret_key_hash: Crypto.hash_secret(secret_key),
account: insert(:account),
deleted_at: nil
}
Expand Down