talawa-api icon indicating copy to clipboard operation
talawa-api copied to clipboard

No encryption of user email addresses in the database

Open palisadoes opened this issue 1 year ago • 31 comments

Describe the bug

  1. User email addresses are not encrypted in the database
  2. This means a bad actor could SPAM Talawa users if they got access to the database

To Reproduce

  1. Dump the database
  2. View unencrypted email data in the User table

Expected behavior

A solution where:

  1. Email addresses are:
    1. encrypted for storage in the database
    2. decrypted for retrieval from the database
  2. The encryption key is not stored in the database
  3. Uses a random salt in addition to the encryption key
  4. The encryption key is configured with setup.ts
    1. If a value is found, then the default for changing the value must be No
    2. If a value is not found, then the encryption key must be randomly generated
  5. Uses a strong encryption algorithm
  6. The User email address data in the sample_data/users.json file must be updated with the encrypted addresses during the data importation process.
  7. The encryption / decryption key(s) are not stored in the:
    1. code base
    2. database

Actual behavior

  1. Email addresses are not encrypted

Screenshots

  • N/A

Additional details

  1. This is the first of many issues to encrypt PII stored in the database
  2. All tests must pass and be valid
  3. No other functionality must be affected

Potential internship candidates

Please read this if you are planning to apply for a Palisadoes Foundation internship

  • https://github.com/PalisadoesFoundation/talawa/issues/359

palisadoes avatar Jan 29 '24 05:01 palisadoes

Respected sir, I can work in this issue, I am comfortable with our graphql schema, after the last two issues got merged. Thank You.

Anubhav-2003 avatar Jan 29 '24 06:01 Anubhav-2003

I would like to work on this issue

AVtheking avatar Jan 29 '24 11:01 AVtheking

Can I work on this?

devsargam avatar Jan 29 '24 12:01 devsargam

@palisadoes Respected sir,

I wanted to clarify one question. Do you want a single encryption key, with a email specific random salt for every user email. Or a user specific unique encryption key, just like the user specific salt for each email. The latter option would involve creating an in-house KMS for handling the keys.

Thank You.

Anubhav-2003 avatar Jan 30 '24 05:01 Anubhav-2003

  1. A single encryption key, encrypting the field using a random salt.
  2. Here is an example that could be used.
    • https://gist.github.com/AndiDittrich/4629e7db04819244e843
  3. Use a long key, the token generation methodology used in the setup.ts file could be used
  4. Research other implementations as possible candidates and use them if better. The PR reviewers will ask about the rationale for your approach.

palisadoes avatar Jan 30 '24 06:01 palisadoes

@palisadoes Ok sir, Thank You.

Anubhav-2003 avatar Jan 30 '24 07:01 Anubhav-2003

@palisadoes Respected sir,

There was a recent revert of a PR in the API that was causing error related to user signup. I had started my feature branch before the revert. So i had to merge the latest upstream to my feature branch. But as a result a lot of files were changed.

One thing I noticed is that for every file changed, eslint throws multiple linting errors that are already present in the code base. At the moment around a hundred linting errors are showing while I try to commit my changes. How can I disable those errors. Otherwise I am unable to commit. Every new line of code I write is passed through linting checks, but the errors shown are for hundreds of lines of code already present.

Thank You.

Anubhav-2003 avatar Jan 31 '24 18:01 Anubhav-2003

Please ask the talawa-api slack channel for assistance.

palisadoes avatar Jan 31 '24 19:01 palisadoes

@palisadoes Respected sir,

The issue is almost done. But I am using an opensource key management service by HashIcorp, for an in-house secret management. As storing the encryption key as plaintext in the .env file is not secure, and industry standard. But this would require all future users of Talawa-api to install 'Vault' from 'HashICorp' , into their local systems and configure it before they can start contributing. Also when pushed to the main repo, the actual cloud instance that runs the API in production must also be updated with the latest software.

Should I proceed with this major addition of software. Or store the key in the .env file only. I feel that if we make the migration, then all current secrets in the .env file could be migrated to the service as well for better security.

Thank You.

Anubhav-2003 avatar Feb 03 '24 15:02 Anubhav-2003

At this time use the .env file for the simplicity of implementation.

palisadoes avatar Feb 03 '24 16:02 palisadoes

@palisadoes ok sir. Thank You.

Anubhav-2003 avatar Feb 03 '24 16:02 Anubhav-2003

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

github-actions[bot] avatar Feb 21 '24 00:02 github-actions[bot]

This issue is active. I have already raised a PR, it is awaited approval. Thank you.

Anubhav-2003 avatar Feb 21 '24 18:02 Anubhav-2003

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

github-actions[bot] avatar Mar 03 '24 00:03 github-actions[bot]

I have already raised PR for this issue, but due to the merge of my recent PR #1896 and a few others there has been drastic changes in the setup. I will be updating the PR as soon as the new implementations are done. Thank You.

Anubhav-2003 avatar Mar 03 '24 06:03 Anubhav-2003

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

github-actions[bot] avatar Mar 15 '24 00:03 github-actions[bot]

@Anubhav-2003 Are you working on this?

jyotiv737 avatar Mar 18 '24 10:03 jyotiv737

@Anubhav-2003 Are you working on this?

Actually, I have already raised a PR for this, the feature is completely implemented, but due to recent pull request merges. Some tests are failing. Thank you.

Anubhav-2003 avatar Mar 18 '24 12:03 Anubhav-2003

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

github-actions[bot] avatar Mar 29 '24 00:03 github-actions[bot]

Update: working on the latest new PR for this issue after the userType-fix branch merge.

Anubhav-2003 avatar Apr 04 '24 16:04 Anubhav-2003

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

github-actions[bot] avatar Apr 23 '24 00:04 github-actions[bot]

Unassigning due to no activity or open PR

Cioppolo14 avatar Apr 23 '24 00:04 Cioppolo14

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

github-actions[bot] avatar May 04 '24 00:05 github-actions[bot]

I would like to work on this issue. I will proceed with the preferred AES-256-GCM Encryption/Decryption algorithm.

Suyash878 avatar Aug 11 '24 11:08 Suyash878

You will need to ensure that all the encryption / decryption will work:

  1. maintaining existing functionality
  2. across the entire test suite
  3. with the setup script where sample data is imported.

palisadoes avatar Aug 11 '24 17:08 palisadoes

I was planning on implementing function for the encryption key variable in the setup.ts. So my question was, Is there a specific preference for where exactly should this function be implemented? as in for instance after the mongodb URL function and or before the recaptcha function.
Or should I just proceed with what I think would be the most suitable place.

Suyash878 avatar Aug 12 '24 17:08 Suyash878

According to this error, we must switch to eslint.config.js from .eslintrc.json, and moreover .eslintignore is no longer supported either. This could be a potential issue in my opinion. If there's something that needs to be done on my end, I would like to know. image

Suyash878 avatar Aug 16 '24 16:08 Suyash878

According to this error, we must switch to eslint.config.js from .eslintrc.json, and moreover .eslintignore is no longer supported either. This could be a potential issue in my opinion. If there's something that needs to be done on my end, I would like to know. image

Please open an issue to fix this for both Admin and the API

palisadoes avatar Aug 21 '24 14:08 palisadoes

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

github-actions[bot] avatar Sep 02 '24 00:09 github-actions[bot]

Suyash are you working on this.

AnshulKahar2729 avatar Sep 02 '24 02:09 AnshulKahar2729