mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
126 stars 41 forks source link

Find a simpler replacement for our current encrypted api-key fields #1767

Open EnTeQuAk opened 7 years ago

EnTeQuAk commented 7 years ago

Porting https://github.com/EnTeQuAk/m2secret (originally forked from Andy and originally written back in 2009) to https://github.com/pyca/cryptography/ kinda revealed that our currently used system for api-keys is a little bit over-engineered for it's use-case and also revealed how old and not up-to-date cryptographicaly m2secret is (sha1+hmac, no constant time compares, it's generally a bit more complex at some places than it should be). All that is fixable but that takes a lot of work and needs careful reviews and I honestly don't like us to maintain cryptographic code if we don't have to.

As far as I understood we are encrypting the API keys so that there's another layer of security in case a security-hole like a SQL injection happens and the attacker gets all or some of our users api-keys which wouldn't be immediately be useful to him and we have another layer to invalid them all by changing our server-side secret key.

Potential proposals:

Whatever we use will need to have better or similar security features (easy to expire / block keys and accounts, safe against sql injection and other attacks, etc) and at best should be good maintained and a generally accepted solution to avoid brewing our own stuff when there's no need for it.

cc kumar303 (initial api-key implementation) and jvehent (security) to the discussion and for potential input

As far as I can see it this isn't really a ticking timebomb since the algorithms we use aren't up-to-date but are not yet known as insecure. Also, we need a system that takes old api-tokens into account and allows silent upgrades to a potential new system.

┆Issue is synchronized with this Jira Task

jvehent commented 7 years ago

Before picking an algorithm, we need to answer the question "Does AMO ever want to show users their current API tokens, or simply allow them to change them?".

Sites differ in this approach. Twitter will show API tokens, AWS will not. Obviously, the latter is more secure because a DB leak or fraudulent access to an account wouldn't leak tokens, but it's also less usable.

I have a strong preference for storing tokens in an irreversible manner, and only allowing users to change them. If we can implement that, I'd recommend to use bcrypt with a random salt for each key. Don't use the single secret key of AMO across all hashes, it's not resistant to rainbow tables attacks.

kumar303 commented 7 years ago

I'm glad we're finally moving this off of M2Crypto! Here are some responses just for historical context:

Porting m2secret ... to pyca/cryptography kinda revealed that our currently used system for api-keys is a little bit over-engineered for it's use-case

The use case of the secret API key is this: a developer uses it to generate a one-time token (a JWT) to upload their add-on package to Mozilla. Mozilla then signs it for installation in Firefox. If this secret key were ever leaked out, an attacker could masquerade as another developer and inject malware into Firefox.

For example, if a popular extension like uBlock Origin were self-hosted and an attacker stole this developer's secret key, they could very quickly propagate an add-on containing malware to all uBlock Origin users.

This was the original decision for why we wanted to encrypt the secret key in the database. We were worried that a SQL injection or some other database-only exploit might leak it.

Does AMO ever want to show users their current API tokens, or simply allow them to change them?

Either way would work. Not showing tokens might be inconvenient for developers but that's not a deal breaker. They can just regenerate the token if they forget what it is.

I have a strong preference for storing tokens in an irreversible manner, and only allowing users to change them.

The reason we store the secret key is because we are currently using JWTs in a symmetric way. The developer signs a JWT on every API request with the secret key and the server uses the same secret key to verify the signature. This architecture could be changed if needed but we do have a lot of tools currently invested in the JWT approach.

But, yeah, it would be helpful to store keys in an irreversible manner because of the threat I mentioned above.

EnTeQuAk commented 7 years ago

Related to https://github.com/mozilla/addons/issues/4567