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

Support for storing app keys in settings #1399

Closed
cdvv7788 opened this issue Jun 7, 2016 · 23 comments
Closed

Support for storing app keys in settings #1399

cdvv7788 opened this issue Jun 7, 2016 · 23 comments

Comments

@cdvv7788
Copy link
Contributor

cdvv7788 commented Jun 7, 2016

I have been using django-allauth for a while now and every time i create a new instance i wonder why the data required for a social connection is hold into a model instead of the settings. This makes somewhat difficult having several instances and complicates the automated deployment of new instances.
I know that some additional data is stored and related to the socialaccount object, but that is not something that cannot be done with settings.
Is there any specific reason that i am missing for this design decision?
PD. not complaining, just curious.

@Akay7
Copy link
Contributor

Akay7 commented Jun 8, 2016

I'm not recommend to you have any tokens and secret codes in repository. Same recommended in many books Two scoops of Django one of them.
If you have trouble after deploy your app(with clear database), then you can write script which will automatically put tokens into your database.

@cdvv7788
Copy link
Contributor Author

cdvv7788 commented Jun 8, 2016

Yeah, i know that having tokens in settings is not a good practice, but you could load them from environment variables to avoid saving them in your repository. Loading the data with an external script is more or less what i do now.
Maybe providing a management command to allow the update from a file containing environment variables could be useful. Just thinking of some way to reduce the manual work that users need to perform to get started (and make easier having several instances of the same project).

@Akay7
Copy link
Contributor

Akay7 commented Jun 8, 2016

I'm prefer copy minimal instance of DB to new instances of project, other users can copy tokens in different way.
And really put tokens trough django admin interface much more friendly, then put something in environment variable and then start manage command.
Anyway that's just my point of view and you can choose your own way. I think that project doesn't need any script which will do special things with tokens, and that is not issue.

@getup8
Copy link
Contributor

getup8 commented Jun 10, 2016

Yeah @cdvv7788 I agree with you, an optional utility would be nice. I ended up doing exactly as you describe:

  1. Store the keys as env variables
  2. Read in and set keys in settings
  3. Set up a manage command to populate the DB here's a gist

I also understand your point though @Akay7, most users are probably doing this once or twice in the admin and don't have to worry about it again.

@andresgz
Copy link
Contributor

I support this idea. In my opinion I would expect to be able to add the tokens from settings and Allauth is able to read it directly from there without adding it to the Django Admin.

@pennersr
Copy link
Owner

See #125 for background info

@stantond
Copy link
Contributor

stantond commented Jun 27, 2016

Although it's perhaps best practice to store these keys as environment variables, I suspect that a number of django-allauth users would not know this, and would then put their keys in the settings file, worsening their security.

What about a configuration option, SOCIAL_ACCOUNT_KEYS_IN_ENVIRONMENT, where False stores these in a local JSON file, getting less experienced developers into a better habit, and True looks for the keys in the environment?

@andresgz
Copy link
Contributor

@stringsonfire I don't see it necessary. If you add settings variables, the developer should be the one who chooses how to store the information. Why to use a JSON file when you can do it in the settings file directly?

@stantond
Copy link
Contributor

stantond commented Jun 27, 2016

@andresgz Because your settings files are in your repo, any keys that may be stored in them will be clearly readable for anyone who has (or gains) access to your repo, creating an unnecessary security vulnerability.

By moving these keys to environment variables or a separate file not in your repo (added to your .gitignore), someone with malicious intent having or gaining access to your repository represents less of a threat.

Check out Two Scoops of Django, well worth a read. I only suggest a JSON file as they suggest it's the most appropriate format if you can't use environment variables.

@pennersr
Copy link
Owner

I am closing this thread. As indicated, the motivation can be found over at #125.

Note that I agree that keeping the keys in the DB is not everybody's cup of tea. At some point in time, I would like to revisit this and allow for reading the SocialApp related config from the settings. Then, everybody is free to do as they please, environment based config is them simply a matter of FOO_SETTING = os.environ.get('FOO_SETTING', ''). But, we're not there yet...

@stantond
Copy link
Contributor

@pennersr if you agree in principle, but don't have the time to do this, should this be added to a Todo section in the docs, welcoming anyone to work on it and preventing duplicate feature requests in future?

@pennersr
Copy link
Owner

I closed it because this is not easily changed without breaking compatibility in a bad way. So this is definitely not something high on the priority list, but let's indeed reopen so that people have something to vote on.

Thoughts:

  • SocialToken has a foreign key to SocialApp. If SocialApp were to be dropped, what to do with the foreign key: null=True ?
  • A workaround for the point above would be to keep SocialApp around, but with blank client/secret/key values. Not quite elegant though...

@pennersr pennersr reopened this Jun 27, 2016
@pennersr pennersr changed the title What is the motivation to hold the data for socialaccounts in a model instead of the settings? Support for storing app keys in settings Jun 27, 2016
@cdvv7788
Copy link
Contributor Author

@pennersr as i initially proposed, we could add a management command to populate socialapps from settings data. We could keep current behavior without breaking anything, and still make it easier to keep that data in the settings. This could work until we decide if we move forward to a full settings setup.

@pennersr
Copy link
Owner

I would prefer to focus this ticket on the "full settings setup" -- I feel adding a management command is a bit too much of a temporary/kludgy solution...

@cdvv7788
Copy link
Contributor Author

In that case, i vote for the removal of the model. Keeping deprecated pieces around does not feel right.

@jamesaud
Copy link

would much prefer this. Loading via environment variables is done for many api keys that I use, it makes it confusing to have to add via django admin

@Lamelos
Copy link

Lamelos commented Sep 15, 2017

I created a fork which implements automatic model creation/retrival (using django get_or_create) using provider specific settings. It seems to work, but I can understand that this implementation isn't preferred as it potentially leaves rogue SocialApp models.

Anyway, for people who desperately want/need to use environment variables for secret/application id, take a look here: https://github.com/Lamelos/django-allauth

@konstin
Copy link

konstin commented Feb 2, 2018

I'm using django-environ, so I'd also like to store the settings in the dotenv.

For now I'm using a script that I wrote based on @getup8's: register_social_accounts.py. I'd really like to see something like that included until there is a purely settings-based solution.

@iMerica
Copy link

iMerica commented Apr 1, 2018

Reasons why this request is important:

  • Storing a full text FB API key in the database is a not a good security practice.
  • Storing config/settings settings in the DB is not 12-Factor or Idiomatic to Django norms. App keys are "settings". In Django settings go in DJANGO_SETTINGS_MODULE.
  • This makes automating the provisioning of new app instances very difficult.
  • Why have a model/table just for storing a single row? (if using just fb for auth)?

@acdameli
Copy link

Considering the following:

  1. ability to rotate keys
  2. ability to separate development from management
  3. scaling across multiple domains

I get that it's not idiomatic for these values to be pulled from the DB. However, given that once allauth is configured a user of the site can manage auth methods (rotating keys, adding new and removing old) is a huge benefit.

If you wanted to create a large offering of social integrations across multiple sites run by the same django installation you would end up with a deeply nested dict in your settings (aka: a nightmare when you need to rotate that one key... or maybe it's an old key that two domains use, need to find it in multiple places).

I'm only partially convinced about the argument that we need to prevent devs from exposing their secret values by accidentally committing them to a repo (good thought, not a great reason for design decisions to me, the naive approach seems to be "create a setting and a management command to set up the models you need" so the current implementation doesn't really prevent someone potentially committing these secrets into a repo).

I'm more interested in encrypting secrets/tokens. User tokens are one thing (yes, it's embarrassing to be hacked and expose user tokens), but the app secrets are a whole other thing.

# socialaccount/models.py
class SocialApp(models.Model):
...
    secret = app_settings.SECRET_FIELD_CLASS(verbose_name=_('secret key'),
                              max_length=191,
                              help_text=_('API secret, client secret, or'
                              ' consumer secret'))
...
class SocialToken(models.Model):
...
    token_secret = app_settings.SECRET_FIELD_CLASS(
        blank=True,
        verbose_name=_('token secret'),
        help_text=_(
            '"oauth_token_secret" (OAuth1) or refresh token (OAuth2)'))
 ...

# socialaccount/app_settings.py
class AppSettings(object):
...
    @property
    def SECRET_FIELD_CLASS(self):
        return self._setting('SECRET_FIELD_CLASS', CharField)
...

The above code intends to allow one to override the SOCIALACCOUNT_SECRET_FIELD_CLASS with something like encrypted_model_fields.fields.EncryptedCharField or a similar implementation, however I'm struggling to define this appropriately in my django settings file.

@iMerica
Copy link

iMerica commented Mar 22, 2019

If secrets are important, they should not be in the DB in plain text.

Securing secrets is not a new problem that needs to be solved with custom encrypted fields in the database. We now have lots of better, more secure options at our disposal:

When you store secret values in the database, everyone has the same poor security for their secrets.

When you store values in the runtime environment (file in the file system, env variables, etc), you're allowing people who do take security seriously to follow the best practices for secrets management.

@acdameli
Copy link

I agree with using a more secure data store but I think what this suggests is tying this package into some additional storage dependency.

Either every sensitive field is stored elsewhere (modify the ORM to load field X from storage mechanism Y). Or store this entire model in another system entirely (second ORM for the subset of models requiring this level of security).

An encrypted field solution is trying to be a minimally invasive change. Maybe specific fields from some other storage mechanism is not as complex as I think but I feel like moving into a separate storage mechanism for this is ballooning the solution.

@pennersr
Copy link
Owner

See 9f65d3c:

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

No branches or pull requests