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.

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

Store hash of email addresses rather than the plain text? #622

Closed
huumn opened this issue Nov 9, 2023 · 10 comments · Fixed by #1111
Closed

Store hash of email addresses rather than the plain text? #622

huumn opened this issue Nov 9, 2023 · 10 comments · Fixed by #1111
Assignees
Labels
backburner postpone consideration of ticket difficulty:medium-hard

Comments

@huumn
Copy link
Member

huumn commented Nov 9, 2023

This is something that I've been background thinking about for awhile.

For our purposes, we don't need to store email addresses that aren't subscribed to the newsletter.

I've been wondering if it might make sense to hash them with some salt

@huumn huumn changed the title Store hash of email addresses rather the plain text? Store hash of email addresses rather than the plain text? Nov 9, 2023
@huumn huumn added backburner postpone consideration of ticket difficulty:medium-hard labels Feb 5, 2024
@SatsAllDay
Copy link
Contributor

Does the backburner label mean a FOSS contributor such as myself shouldn't pick this one up at this time? Or is that just for employees to look elsewhere for work?

This one reminds me of the API Keys changes where we ended up hashing API Keys in the DB. It piques my interest.

@huumn
Copy link
Member Author

huumn commented Apr 20, 2024

Yep, I'm just trying to indicate it isn't a priority. I gave this one backburner because there are consequences to this that I haven't thought about carefully.

A. The primary way we get new newsletter subscribers is when they sign up with email.
B. If we ever want to send notification digests we'll need to store the email in plaintext.

So UX of A & B degrade. I can't tell if it's worth it. Open to thoughts.

@SatsAllDay
Copy link
Contributor

I need to familiarize myself with the email sign up flow, but here are some of my initial thoughts.

A. The primary way we get new newsletter subscribers is when they sign up with email.

It would probably be a good idea to only subscribe the new stacker to the newsletter if they opt-in, meaning not automatically by creating an account with email as the primary auth method. So by default, storing the hashed email here would be ok. As soon as a user opts in to receive the newsletter, we could prompt them for the email address they want to receive it at, do some kind of verification to ensure they own the email address, and then store that address in plaintext as a newsletter subscriber email once verified.

This could still be part of the sign-up flow, but maybe have it be an extra check box so the users have explicit control? It may already be this way, I need to test it out...

B. If we ever want to send notification digests we'll need to store the email in plaintext.

I agree, we would need emails in plaintext to support this. I would probably propose a similar process to step 1 above, where by default this is not enabled, and you must opt-in to receive these emails, and be informed that by opting in, we are going to store your email address in plaintext, as a requirement of delivery/sending.

In general though, we do not need emails in plaintext just for auth, as you originally noted. I guess it depends on how interested you are in driving adoption of the newsletter emails, and if you think people won't manually opt-in to it after the fact, instead of automatically subscribing them to it?

In my opinion, given how prevalent privacy is on SN, and given that we offer auth methods like LNAuth, we should take a privacy-first stance on this and only store emails in plaintext if the user specifically provides it for the purpose of receiving emails, like newsletters and notification digests, and not just for the purpose of authentication.

</end-spiel>

@huumn
Copy link
Member Author

huumn commented Apr 23, 2024

I've been thinking about it and I agree.

We'll need to figure out how to handle the newsletter and how to otherwise promote it in the ui though.

@SatsAllDay
Copy link
Contributor

I'll think it over and try to work up a proposal here for discussion.

@SatsAllDay
Copy link
Contributor

I am working up a PR to see the effect of these changes and help make some decisions. A couple of notes I've made thus far:

  1. Salting the email before hashing doesn't really work because the user would have to therefore provide the salt in order for us to do a lookup, which won't work. So I think in this case, we're limited to hashing the email address as-is before saving in the DB. Similar to what we do for API Keys.
  2. No longer storing the email address also has the impact of not being able to show the user the email address that is currently linked to their account in the settings page, should they want to unlink the email address. Are you ok with an experience like the following, where users can unlink the email address, but they don't know which email address it is?
image

@huumn
Copy link
Member Author

huumn commented Apr 26, 2024

  1. I was thinking we would provide the salt, that way you'd really need both the db and the salt to confirm a known email address belonged to a nym on sn.
  2. It'll probably be a new source of support tickets but we already have this problem with lnauth.

@SatsAllDay SatsAllDay self-assigned this Apr 26, 2024
@SatsAllDay
Copy link
Contributor

  1. I was thinking we would provide the salt, that way you'd really need both the db and the salt to confirm a known email address belonged to a nym on sn.

Can you elaborate a little on what you mean by "we would provide the salt"? Do you mean, we give it to users when they create an account via email on SN? Or that we as SN would provide the salt when doing the lookup in the DB? If the latter, this is what I started with, but quickly realized that when a user attempts to login with just an email, it's impossible to know which salt to salt it with, unless you intend to use a shared salt for all emails? I was operating under the assumption that each email would have its own salt, but maybe that's where the confusion came from. If it's a shared salt, then yes, it could be a env var secret or something that way DB access doesn't give you everything. That should work.

  1. It'll probably be a new source of support tickets but we already have this problem with lnauth.

As long as you're ok with this implication :) I hadn't considered it previously until I found it locally so wanted to surface it.

@huumn
Copy link
Member Author

huumn commented Apr 26, 2024

If it's a shared salt, then yes, it could be a env var secret or something that way DB access doesn't give you everything. That should work.

Yes that one! :)

As long as you're ok with this implication

We'll see lol

@ekzyis
Copy link
Member

ekzyis commented Apr 28, 2024

  1. It'll probably be a new source of support tickets but we already have this problem with lnauth.

As long as you're ok with this implication

We'll see lol

Since we're moving into the direction of encrypting credentials for attached wallets, E2EE for #56, #519 and other stuff, we will probably provide users with some kind of passphrase anyway. I think we could use the same passphrase for users to backup their accounts in case they lose access1. If they can't login, they could select "lost access?" or similar below "Been here before?" or "New to town?" which redirects them to a page to enter their passphrase to regain access.

This should solve any support request if the information is prominent enough that you will lose access if you lose this passphrase, similar to losing seed words in bitcoin.

Footnotes

  1. This would mean it's a single point of failure if an attacker gets plaintext access, but it doesn't mean we're using the same encryption keys. We can derive different encryption keys for different methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backburner postpone consideration of ticket difficulty:medium-hard
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants