Skip to content
This repository has been archived by the owner on Dec 20, 2018. It is now read-only.

GDPR Support #1341

Closed
3 tasks done
blowdart opened this issue Aug 2, 2017 · 29 comments
Closed
3 tasks done

GDPR Support #1341

blowdart opened this issue Aug 2, 2017 · 29 comments
Assignees
Milestone

Comments

@blowdart
Copy link
Member

blowdart commented Aug 2, 2017

  • Extension points to enable encryption of user data
  • Extension point to enable downloading of user data
  • Extension point to enable deletion of user data
@blowdart blowdart added this to the 2.1.0 milestone Aug 2, 2017
@HaoK
Copy link
Member

HaoK commented Aug 3, 2017

Should tackle along with #836

@HaoK HaoK self-assigned this Aug 3, 2017
@HaoK
Copy link
Member

HaoK commented Aug 3, 2017

@blowdart how do we want this encryption to work? I.e. is this something that's done at the store level, so the public APIs don't really change, we'd just add a new service or two with some new options and our default EF implementation would use some kind of new IIdentityDataProtectionService that would basically a string Protect/Unprotect that the store would use automatically on most of the user fields resulting in encrypted strings in the db: i.e. PasswordHash/Email/tokens/login providers/keys/claims

Or would we want this at the manager layer, where the TUser poco is encrypted, and you have to call manager APIs to read things off the user. This would make it hard to use the entity by itself which might be a fatal drawback... i.e. user.Email would be gibberish

@blowdart
Copy link
Member Author

blowdart commented Aug 3, 2017

Yea, I think we have a protect/unprotect (not encrypt/decrypt, that's a loaded term). It needs to be on a per object basis, and the stores call protect before writing, and unprotect after reading, then it's in plain text in memory (the regulations are aimed at protection at rest). We'd also need the protect/unprotect for individual items we lookup/search on.

I don't think we can limit it to just our base class, folks add properties that may need protection. Also it needs to be usable outside of the stores, for secondary objects (image IP address logging for example).

To be flexible and cope with future requirements I think we need to do it per object, taking a purpose or other indicator so people can adjust what's used based on the information (e.g. credit cards would need more protection than, say, a username)

@HaoK
Copy link
Member

HaoK commented Aug 3, 2017

So you are thinking more like IIdentityProtector<TEntity> with Protect()/Unprotect() and it manipulates the POCO?

@HaoK
Copy link
Member

HaoK commented Aug 3, 2017

This would be something the managers would do not the stores, we want the stores to be dumb simple and just responsible for serialization of the data.

@blowdart
Copy link
Member Author

blowdart commented Aug 3, 2017

Or a copy of the Poco, so we don't mutate the clear text one. managers doing it are fine.

@HaoK
Copy link
Member

HaoK commented Aug 3, 2017

Not sure we can copy without doing bad things to EF... @divega @ajcvickers thoughts?

@blowdart
Copy link
Member Author

blowdart commented Aug 3, 2017

Clone doesn't work? Bah. Well then we'd unprotect on the way back out. Which is expensive :(

@HaoK
Copy link
Member

HaoK commented Aug 3, 2017

That sounds pretty terrible...Let me noodle around a bit

@blowdart
Copy link
Member Author

blowdart commented Aug 3, 2017

Thing is, if we change it, well, if the user tries to use the instance after saving, it's going to be full of unintelligible crap.

@HaoK
Copy link
Member

HaoK commented Aug 3, 2017

Yup, not really sure how we can make this work, I guess clone before save is cleanest there

@divega
Copy link

divega commented Aug 3, 2017

We should chat about this in depth. At this point I cannot think of a way this could be implemented without adding a new layer of indirection that would impact the whole Identity stack, unless protection can happen "transparently" in layers below Identity.

E.g. one of the stretch goals for EF Core in 2.1 is to enable setting up "conversions" as part of the mapping, which need to be applied when property values roundtrip between objects and the database. These "conversions" would be a good place to do protection. EF Core happens to already have a natural place to configure this at a property level which is controller by the user: OnModelCreating().

On the other hand if we did it this way it would become a feature of the EF Core provider for Identity, and I am not sure that would satisfy the requirements.

@HaoK
Copy link
Member

HaoK commented Oct 30, 2017

Mocked up a minimal new Privacy tab in the manage controller, that has a link to download personal data, and a button to delete the user.

The download just calls an action that returns a placeholder string ("This is the user data for user.UserName"). It would be up to the app to plug real user serialization code.

The delete button just calls userManager.Delete(user) which is sufficient for the default template. They would obviously have to customize this as well to handle any thing they added...

We can review this at triage this week to discuss how it looks / if we need to add anything

@blowdart @ajcvickers

@HaoK
Copy link
Member

HaoK commented Jan 17, 2018

Download/deletion added via: f1ed482

@blowdart blowdart modified the milestones: 2.1.0, 2.1.0-preview2 Jan 18, 2018
@itzAnish
Copy link

itzAnish commented Jan 23, 2018

Hello,
Is it possible to store the email column in AspNetUsers table to be encrypted using Application level i.e System.Security.Cryptography or Database level i.e. Always Excrypted (SQL Server 2016) ?

We are using ASP.Net Identity with our e-commerce application and need to follow GDPR for customer data.

Thanks.

@StingyJack
Copy link

Please do not lock everyone into using EF.

@HaoK
Copy link
Member

HaoK commented Mar 8, 2018

Encryption extensbility added via #1562

@HaoK HaoK closed this as completed Mar 8, 2018
@HaoK
Copy link
Member

HaoK commented Mar 22, 2018

Currently the GDPR work only enabled protections for the user table by default, we need to add additional logic to also protect the contents of the user token table... and should also review potentially any other tables to protect

@Eilon
Copy link
Member

Eilon commented Mar 28, 2018

Any work here for preview2?

@HaoK
Copy link
Member

HaoK commented Mar 28, 2018

So @blowdart @ajcvickers the question is basically should we be protecting things like the user tokens table automatically when ProtectData is on as well. My guess is yes since things like your recovery codes and access tokens and what not are all potentially stored here... in the clear...

And if so, is this something we need to address for preview2, or is this ok to fix in rc1? I think either way this needs to be fixed before shipping the feature...

@ajcvickers
Copy link
Contributor

I defer to @blowdart

@HaoK
Copy link
Member

HaoK commented Mar 28, 2018

I guess the reality is this might not be a simple fix if we want to do this, so assuming this is something we want to do, it likely should be done in rc1 as I doubt this is something we want to ram in tomorrow :)

@blowdart
Copy link
Member Author

Rc1

@HaoK
Copy link
Member

HaoK commented Mar 28, 2018

So the full list of tables we should consider if we need to protect additional data:

Roles (role id, role name)
UserRoles (userid, role id)
UserClaims (userid, claim name, claim type)
RoleClaims (role id, claim name, claim type)
UserTokens (userid, token name, token value)
UserLogin (userid, providerName, providerKey)

@blowdart any of these stand out that should be protected, maybe user login since it has their provider key which with the provider would uniquely identify a user. Do we need to protect their claims?

@blowdart
Copy link
Member Author

Adding the attribute does nothing unless something is wired up right? Then being over cautious won't be a bad thing.

@HaoK
Copy link
Member

HaoK commented Mar 28, 2018

Ignoring the attribute for now, the top level switch is identityOptions.ProtectUserData = true, do we want any columns in these additional tables to be protected

@HaoK
Copy link
Member

HaoK commented Apr 9, 2018

We will protect:

  • all UserTokens
  • the providerKey for UserLogins

And include these in the default DownloadPersonalData page

@HaoK
Copy link
Member

HaoK commented Apr 12, 2018

Done via #1745 we can't actually protect the provider keys for logins at this time, I'll file a new issue to revisit that in 2.2

@HaoK HaoK closed this as completed Apr 12, 2018
@HaoK HaoK added 3 - Done and removed 2 - Working labels Apr 12, 2018
@HaoK
Copy link
Member

HaoK commented Apr 12, 2018

https://github.com/aspnet/Identity/issues/1753 for tracking protecting the provider keys

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants