Keychain: account manager for external systems

[hparfr]

See readme for more info

[oca-clabot]

Hey @hparfr, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

• Raph (no github login found)

Appreciation of efforts,
OCA CLAbot

[lasley]

This is very dangerous IMO & I am explicitly 👎

This is enforcing bad practice and the entire keychain can be easily compromised in the UI with code similar to the following:

model = env['keychain.account']

log(
    str(env['keychain.account'].search([]).read()),
)

Given the current security, it wouldn't even require an admin to run this exploit.

There is also no way to expire the existing key in the event of compromise, which is a 100% necessity with anything encryption.

I'm sorry that I am not providing anything helpful to this PR, but this is because we have been toiling over this for a while & have not been able to come up with a production viable solution at this point.

I would not trust my data with either this module or the PoCs we have already built for this purpose, thus the 👎 . Security is no joke, and a simple disclaimer is not enough IMO.

[hparfr]

Again, the aim of this module is to store credentials like smtp server, carriers, market places, magento, ftp servers and so on and don't leak them with db dumps or search_read.

It's credentials you need in odoo and it's credentials which are currently stored in plain text by the modules and are easily recoverable with your snippet.

With keychain module, at runtime you need to do 2 actions to decrypt a password :

• get the record from the ORM (which is filtered with odoo's ACL)
• get the key from the config manager (which is only accessible by python)

From a security point of view : the current state with odoo out of the box is very terrible. With this keychain module, it's certainly not perfect but a little more secure.

This is enforcing bad practice

Which one ?

and the entire keychain can be easily compromised in the UI with code similar to the following:

No with keychain supported modules you will get :

• if you are not in authorized group : access_error
• else : an encrypted password

With other modules, you will get :

• if you are not in authorized group : access_error
• else : a clear text password

There is also no way to expire the existing key in the event of compromise, which is a 100% necessity with anything encryption.

In the event of a compromise you have to change all your passwords.
If you want key expiration or x509 certificates your PR is welcome :) I hope the current design of this module allows this kind of enhancement.

but this is because we have been toiling over this for a while & have not been able to come up with a production viable solution at this point.

It seams your requirements (storing credit card or personal / medical data) are very different.
You need conform to norms (like PCI-DSS) and your responsibility may be engaged if you leak this kind of data : this is not the aim of this module.

[hparfr]

Responding to comments on #622 (comment)

This module will be available on the App Store. I can guarantee that it will be installed on production servers by administrators that did not read the disclaimer in the ReadMe.

This module is unusable / useless if no modules consumes his services. I hope, the developer who add this module as a dependecy of his own module will read the readme. If he don't (which is plausible) we can't do anything about it.

I do also like the idea of a separate service. I created a Golang app recently to do some key management & other useful things, and have been thinking of integrating it into Clouder

It's a good idea to externalize the storage of sensitive data, but it may be overkill to store smtp credentials or delivery carrier accounts.

[rvalyi]

@hparfr @lasley I won't comment about the module because I'm not a security expert. That being said, usually a good way to inject secrets in an app is via environment variables.

These can be managed with tools such https://www.vaultproject.io if something more high level than editing these secrets over ssh is required.

[lasley]

@rvalyi - Ohhhhh I like that. I'm going to see about making a connector for VaultProject after I play around with it a bit more.

@hparfr - I agree on the need, but this is adding a tad bit of security and also the possibility of mass data corruption if the key needs to be revoked. The tradeoff is more dangerous than it is worth IMO.

Obviously the community can override my feelings, but they are firm on this. My project was doing nearly the exact same things you are here & I abandoned it because it is flawed from the ground up.

I recommend reading into OWASP guidelines on these subjects. They explain it all much better then me, and you will notice that nearly nothing of the guidelines is covered.

Again, I agree it isn't covered in Odoo either, but you are specifically advertising this module as a keychain. That name implicitly states you are managing the keys in a safe fashion.

• https://www.owasp.org/index.php/Key_Management_Cheat_Sheet
• https://www.owasp.org/index.php/Cryptographic_Storage_Cheat_Sheet
• https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet

[hparfr]

Thanks @lasley for having taking time to expose your arguments.

[lasley]

@hparfr - I honestly feel real bad about blocking this & am trying to come up with a viable solution for the key management + a proposal to make this more secure. I'll update you.

[bealdav]

Hi @lasley,

Ok there are more secure ways with external and complex system, but our all customers can't pay to have an erp (aka all in one features system) and 36 connectors with external system to maintain over the time.
It depends of criticity of these keys

@hparfr proposal use a secure way, I think

Fernet is built on top of a number of standard cryptographic primitives. Specifically it uses:

   AES in CBC mode with a 128-bit key for encryption; using PKCS7 padding.
   HMAC using SHA256 for authentication.
   Initialization vectors are generated using os.urandom().

It ensures all password are not clear text.

It's a v9 module. If in v10 or v11 you provide a better way to do it, then we'll migrate to the new solution, but please don't break continuous improvement of the community, it's a step in the right direction

https://en.wikipedia.org/wiki/Continual_improvement_process

👍

[lasley]

@bealdav - This isn't simply about security. This is also about potential data destruction with no avenue of resolve. Encryption isn't just flipping a few bits, then flipping them back with the appropriate key when needed - you also need to build a way for re-encryption of data against new keys & the revocation of old. This is standard in any application that encrypts anything, and any product without it is dangerous. Continuous improvement does not help the user of the application in the middle of a breach scenario that is faced with the decision of having to destroy all their encrypted data in order to revoke a key. Revocation is not a feature of an encryption system, it is a requirement.

[bealdav]

@lasley If security was the main point, you'd haven't choose odoo at all, don't you think ?
Here it's better secure than odoo standard practice. Nowdays any module store its password with no rule, some time in clear text.
Rome was not built in one day and here it's v9, an old version, now.
Improvement will comes after in v10
You can propose a feature to provide revocation, no problem.
@hparfr could you consider to add revocation in roadmap ?

@lasley Thanks for your contribution on security subjects. It's a real important for us to have expert like you on this matter, just let community go step by step to real good practice as you suggest.

[legalsylvain]

Hi,
I'm not expert in the security domain, but I find the design of this module very good. To steal uncrypted credentials you have to have access to the database AND the config openerp file. So I see it as something secured.
Maybe there are other possibilities, but for the time being, this is a good solution, and better than nothing, specially because there are actually some OCA / not OCA modules with clear credentials (not secure), or others with clear token (not very secure) so 👍 for the concept.

@bealdav said :

It's a v9 module. If in v10 or v11 you provide a better way to do it, then we'll migrate to the new solution, but please don't break continuous improvement of the community, it's a step in the right direction

👍 on that point, too.

@OCA/board , @StefanRijnhart, @Others, could you give your PoV ?

kind regards.

[bealdav]

ping @guewen @yvaucher

[mourad-ehm]

👍 because this module make one step in the right direction. It brings a solution to managing credentials for external applications which not exist now in odoo.

[hparfr]

Thanks @elicoidal for your precise review.

[hparfr]

Requested changes taken in account.
This PR is now ready to be merged.

Thanks everyone for your comments and reviews.

[hparfr]

What may I do for this PR to be accepted ?

• fixed requested changes
• tests ok

[bealdav]

ping @elicoidal @lasley

[lasley]

Sorry but I cannot sign off with this potential for data loss. A PSC will need to dismiss my review for merge. There's definitely enough thumbs though and it sounds like @elicoidal was onboard with the risk, so you should be good here soon.

[elicoidal]

Can you check travis?
Can you add in the readme a disclaimer/warning with a link to this discussion?

[hparfr]

Travis is failing on other modules when database_cleanup is included
https://travis-ci.org/OCA/server-tools/builds/199916286

[lasley]

@hparfr - The build was fixed in #719, so you should be able to clear the issue here with a rebase on the current 9 🚀