openstack team mailing list archive

[Gabriel Hurley <Gabriel.Hurley@xxxxxxxxxx>]

That's looking pretty good, Sascha. May I suggest:

1. Instead of using the temporary lockfile, let's actually move slightly back towards your original approach and check for the existence of a known secret key file (added to .gitignore of course) which the get_secret_key function can check for.

2. Split the code that generates the secret key into a separate function generate_secret_key which can be used at will.

2. In the run_tests.sh script (meant for developers, really not used in production deployments), have it check for a valid secret key in either the settings file itself or the known secret key file as part of the sanity check for the environment. If a valid secret key is not available, have it ask the user if they'd like to automatically generate a secret key (using generate_secret_key). Make sure this also respects the -q/--quiet flag for non-interactive runs like Jenkins.

3. Drop lines 25-29 in the local_settings.py.example in your branch, and uncomment line 31. I liked removing the example SECRET_KEY that exists in the example settings file like in your first patch. Even leaving it in as a comment encourages people to simply uncomment it and use the recycled insecure key. Let's take that back out and just keep your explanatory text, which is spot-on.

4. Modify the get_secret_key function so that if a valid key is not available (e.g. this is production and we didn't use run_tests.sh to install the environment) it dies with a very explanatory error message indicating that a secret key must be set.

I know that's a lot to take in, but I think it walks the cleanest line between making it super-easy for developers while making sure everyone is aware of what's going on.

If you'll do that and push the review to Gerrit I'll be totally in support of it.

Thanks!

    - Gabriel

> -----Original Message-----
> From: openstack-bounces+gabriel.hurley=nebula.com@xxxxxxxxxxxxxxxxxxx
> [mailto:openstack-
> bounces+gabriel.hurley=nebula.com@xxxxxxxxxxxxxxxxxxx] On Behalf Of
> Sascha Peilicke
> Sent: Tuesday, June 19, 2012 2:04 AM
> To: openstack@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Openstack] [Blueprint automatic-secure-key-generation]
> Automatic SECURE_KEY generation
> 
> On 06/19/2012 04:55 AM, Paul McMillan wrote:
> >> Ah, you're thinking of a setup where there are multiple dashboard VMs
> >> behind a load-balancer serving requests. Indeed, there the dashboard
> >> instances should either share the SECRET_KEY or the load-balancer has
> >> to make sure that all requests of a given session are redirected to
> >> the same dashboard instance.
> >
> > I'm concerned that anything which automatically generates a secret key
> > will cause further problems down the line for other users. For
> > example, you've clearly experienced what happens when you use more
> > than one worker and generate a per-process key. Imagine trying to
> > debug that same problem on a multi-system cloud (with a load balancer
> > that usually routes people to the same place, but not always). If you
> > aren't forced to learn about this setting during deployment, you are
> > faced with a nearly impossible problem of "users just sometimes get
> logged out".
> >
> > I feel like this patch is merely kicking the can down the road just a
> > little past where your particular project needs it to be, without
> > thinking about the bigger picture.
> I'm sorry about that, but that was definitely not the intent. Inherently you
> are right, there are just far to many possible setups to get them all right. Thus
> it's a valid choice to defer such decisions to the one doing the setup. But
> trying to ease the process can't be that wrong either. That's the whole point
> why distributions don't only provide packages that merely include pristine
> upstream tarballs. We try (and sometimes fail to) provide useful defaults that
> are at least useful to get started.
> 
> >
> > I'm sure you're not seriously suggesting that a large-scale production
> > deployment of openstack will be served entirely by a single point of
> > failure dashboard server.
> >
> >> But shouldn't local_settings.py still take preference over settings.py?
> >> Thus the admin could still set a specific SECRET_KEY in
> >> local_settings.py regardless of the default (auto-generated) one. So
> >> I only would have to fix the patch by not removing the documentation
> >> about SECRET_KEY from local_settings.py, right?
> >
> > I agree with Gabriel. Horizon should ship with no secret key (so it
> > won't start until you set one). At most, it should automatically
> > generate a key on a per-process basis, or possibly as part of
> > run_tests, so that getting started with development is easy. Under no
> > circumstances should it attempt to read the mind of an admin doing a
> > production deployment, because it will invariably do the wrong thing
> > more often than the right. As a security issue, it's important that
> > admins READ THE DOCUMENTATION. Preventing the project from starting
> > until they address the issue is one good way.
> Ok, I've adjusted the patch to reflect that. So there's no default anymore but
> some more documentation about the options instead:
> 
> https://github.com/saschpe/horizon/compare/bp/automatic-secure-key-
> generation
> 
> This should now match Gabriel's proposal, so would that be ok for you?
> 
> >> Unfortunately, this is only relevant for securing production
> >> deployments. Nobody cares if a developer instance is setup securely
> >> ;-)
> >
> > I beg to differ. Opening trivially exploitable holes in development
> > machines (especially laptops) can be extremely damaging. (you did read
> > the docs about the consequences of disclosing a secret key, right?)
> Actually, this statement wasn't meant to be overly serious, therefore the
> smiley. I only tried to clarify that "production" was the primary concern, sorry
> again.
> 
> > If we don't force developers and end-users to read the documentation,
> > particularly around security features, they will get them wrong. "Best
> > Guess" security isn't a business I want to be in.
> No doubts about that and still they do. Hopefully, the current patch is
> acceptable to you know.
> --
> With kind regards,
> Sascha Peilicke
> SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nuernberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer HRB 16746 (AG Nürnberg)
>