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.

Sign up for GitHub

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

Jump to bottom

Use Vault transit engine for storing Wallet & PaymentAddress sensitive data #2310

Merged
merged 1 commit into from
Aug 9, 2019
Merged

Use Vault transit engine for storing Wallet & PaymentAddress sensitive data #2310

merged 1 commit into from
Aug 9, 2019

Conversation

dnfd
Copy link

@dnfd dnfd commented Aug 7, 2019

No description provided.

ysv
ysv suggested changes Aug 7, 2019
View reviewed changes
Copy link

@ysv ysv left a comment •
edited by dnfd
Loading

Choose a reason for hiding this comment

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

  • I advice to use vault_lazy_decrypt!

  • Could we add some specs

Gemfile Outdated
@@ -55,6 +55,7 @@ gem 'peatio', '~> 0.6.1'
gem 'rack-cors', '~> 1.0.2', require: false
gem 'env-tweaks', '~> 1.0.0'
gem 'vault', '~> 0.12', require: false
gem 'vault-rails', '~> 0.5.0', git: 'http://github.com/dnfd/vault-rails'
Copy link

Choose a reason for hiding this comment

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

Ask owner if they are going to merge it soon
Otherwise we will probably fork it to rubykube

Also could you check pls that patch you applied on top of master doesn't contain vulnerabilities

Copy link
Author

Choose a reason for hiding this comment

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

hashicorp/vault-rails#67 (comment)

app/models/payment_address.rb Outdated Show resolved Hide resolved
app/models/wallet.rb Outdated Show resolved Hide resolved
app/models/wallet.rb Outdated
end

define_method "#{accessor}=".to_sym do |value|
self.settings = self.settings.merge({ accessor.to_s => value })
Copy link

Choose a reason for hiding this comment

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

I think you could use .merge! and avoid hash in params

self.setting.merge!(attribute => value)

Copy link
Author

@dnfd dnfd Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer self.settings=, it is a method call actually, while merge! modifies a hash returned with settings, and it breaks some specs

config/initializers/vault.rb Outdated Show resolved Hide resolved
db/migrate/20190807092706_add_encrypted_secret_to_payment_address.rb Outdated Show resolved Hide resolved
db/migrate/20190807092706_add_encrypted_secret_to_payment_address.rb Outdated
@@ -0,0 +1,12 @@
class AddEncryptedSecretToPaymentAddress < ActiveRecord::Migration[5.2]
def change
Copy link

@ysv ysv Aug 7, 2019 •
edited by dnfd
Loading

Choose a reason for hiding this comment

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

  • Put columns before timestamps

  • What is the string limit for encrypted attribute ? May be default string(255) is too many or we need more for wallet settings which is actually a long string

  • You can't just remove column you need to transfer existing data to vault before

@dnfd dnfd force-pushed the feature/vault-rails branch 5 times, most recently from 78ffc98 to 87e7dd9 Compare August 9, 2019 12:20
ysv
ysv suggested changes Aug 9, 2019
View reviewed changes
app/models/wallet.rb Outdated
end

define_method "#{attribute}=".to_sym do |value|
update(settings: self.settings.merge(attribute.to_s => value))
Copy link

Choose a reason for hiding this comment

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

update will save record in db ?

Copy link
Author

Choose a reason for hiding this comment

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

I added specs 🙂

app/models/wallet.rb Outdated
# We use this attribute values rules for wallet kinds:
# 1** - for deposit wallets.
# 2** - for fee wallets.
# 3** - for withdraw wallets (sorted by security hot < warm < cold).
ENUMERIZED_KINDS = { deposit: 100, fee: 200, hot: 310, warm: 320, cold: 330 }.freeze
enumerize :kind, in: ENUMERIZED_KINDS, scope: true

# temporary solution
Copy link

Choose a reason for hiding this comment

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

Start comment from capital letter and end with dot

@dnfd dnfd changed the title Integrate vault-rails Use Vault transit secret engine to save wallet settings and payment_address secret and details Aug 9, 2019
@dnfd dnfd force-pushed the feature/vault-rails branch from 87e7dd9 to b8127f3 Compare August 9, 2019 12:49
@ysv ysv changed the title Use Vault transit secret engine to save wallet settings and payment_address secret and details Use Vault transit secret engine for storing Wallet & payment_address sensitive data Aug 9, 2019
@ysv ysv changed the title Use Vault transit secret engine for storing Wallet & payment_address sensitive data Use Vault transit engine for storing Wallet & payment_address sensitive data Aug 9, 2019
@ysv ysv changed the title Use Vault transit engine for storing Wallet & payment_address sensitive data Use Vault transit engine for storing Wallet & PaymentAddress sensitive data Aug 9, 2019
matass
matass reviewed Aug 9, 2019
View reviewed changes
config/initializers/vault.rb Outdated

Vault.configure do |config|
config.address = ENV.fetch('VAULT_URL', 'http://127.0.0.1:8200')
config.token = ENV.fetch('VAULT_TOKEN')
config.ssl_verify = false
config.timeout = 60
config.application = 'peatio'
Copy link

Choose a reason for hiding this comment

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

Maybe fetch from ENV?

Copy link

Choose a reason for hiding this comment

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

yes we will do

@dnfd dnfd force-pushed the feature/vault-rails branch 3 times, most recently from 31b5c73 to e303bf3 Compare August 9, 2019 13:25
@ysv ysv added PR: Approved and removed PR: WIP labels Aug 9, 2019
@dnfd
Integrate vault-rails
d5d5a30 
  * Update migration
@dnfd dnfd force-pushed the feature/vault-rails branch from e303bf3 to d5d5a30 Compare August 9, 2019 15:44
@mod mod merged commit d748e57 into master Aug 9, 2019
@mod mod deleted the feature/vault-rails branch August 9, 2019 16:23
@krtschmr
Copy link

krtschmr commented Aug 9, 2019

It seems like Enterprise Platform pricing is required otherwise no failover in case of emergency. What's pricing on that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ysv ysv ysv requested changes

@matass matass matass left review comments

Assignees
No one assigned
Labels
PR: Approved v2.3
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dnfd @krtschmr @matass @ysv @mod