Open Stack

Excerpts from Sean Dague's message of 2013-12-04 10:51:16 -0800:
> On 12/04/2013 11:56 AM, Ben Nemec wrote:
> > On 2013-12-04 06:07, Sean Dague wrote:
> >> On 12/03/2013 11:21 PM, Clint Byrum wrote:
> >>> Excerpts from Sean Dague's message of 2013-12-03 16:05:47 -0800:
> >>>> On 12/03/2013 06:13 PM, Ben Nemec wrote:
> >>>>> On 2013-12-03 17:09, Sean Dague wrote:
> >>>>>> On 12/03/2013 05:50 PM, Mark McLoughlin wrote:
> >>>>>>> On Tue, 2013-12-03 at 16:23 -0600, Ben Nemec wrote:
> >>>>>>>> On 2013-12-03 15:56, Sean Dague wrote:
> >>>>>>>>> This cinder patch - https://review.openstack.org/#/c/48935/
> >>>>>>>>>
> >>>>>>>>> Is blocked on failing upgrade because the updated oslo
> >>>>>>>>> lockutils won't
> >>>>>>>>> function until there is a specific configuration variable added
> >>>>>>>>> to the
> >>>>>>>>> cinder.conf.
> >>>>>>>>>
> >>>>>>>>> That work around is proposed here -
> >>>>>>>>> https://review.openstack.org/#/c/52070/3
> >>>>>>>>>
> >>>>>>>>> However I think this is exactly the kind of forward breaks that we
> >>>>>>>>> want
> >>>>>>>>> to prevent with grenade, as cinder failing to function after a
> >>>>>>>>> rolling
> >>>>>>>>> upgrade because a config item wasn't added is exactly the kind
> >>>>>>>>> of pain
> >>>>>>>>> we are trying to prevent happening to ops.
> >>>>>>>>>
> >>>>>>>>> So the question is, how is this done correctly so that a default
> >>>>>>>>> can be
> >>>>>>>>> set in the cinder code for this value, and it not require a config
> >>>>>>>>> change to work?
> >>>>>>>
> >>>>>>> You're absolutely correct, in principle - if the default value for
> >>>>>>> lock_path worked for users before, we should absolutely continue to
> >>>>>>> support it.
> >>>>>>>
> >>>>>>>> I don't know that I have a good answer on how to handle this,
> >>>>>>>> but for
> >>>>>>>> context this change is the result of a nasty bug in lockutils that
> >>>>>>>> meant
> >>>>>>>> external locks were doing nothing if lock_path wasn't set. 
> >>>>>>>> Basically
> >>>>>>>> it's something we should never have allowed in the first place.
> >>>>>>>>
> >>>>>>>> As far as setting this in code, it's important that all of the
> >>>>>>>> processes
> >>>>>>>> for a service are using the same value to avoid the same bad
> >>>>>>>> situation
> >>>>>>>> we were in before.  For tests, we have a lockutils wrapper
> >>>>>>>> (https://github.com/openstack/oslo-incubator/blob/master/openstack/common/lockutils.py#L282)
> >>>>>>>>
> >>>>>>>> that sets an environment variable to address this, but that only
> >>>>>>>> works if all of the processes are going to be spawned from within
> >>>>>>>> the same wrapper, and I'm not sure how secure that is for
> >>>>>>>> production
> >>>>>>>> deployments since it puts all of the lock files in a temporary
> >>>>>>>> directory.
> >>>>>>>
> >>>>>>> Right, I don't think the previous default really "worked" - if
> >>>>>>> you used
> >>>>>>> the default, then external locking was broken.
> >>>>>>>
> >>>>>>> I suspect most distros do set a default - I see RDO has this in its
> >>>>>>> default nova.conf:
> >>>>>>>
> >>>>>>>   lock_path = /var/lib/nova/tmp
> >>>>>>>
> >>>>>>> So, yes - this is all terrible.
> >>>>>>>
> >>>>>>> IMHO, rather than raise an exception we should log a big fat warning
> >>>>>>> about relying on the default and perhaps just treat the lock as an
> >>>>>>> in-process lock in that case ... since that's essentially what it
> >>>>>>> was
> >>>>>>> before, right?
> >>>>>>
> >>>>>> So a default of lock_path = /tmp will work (FHS says that path has
> >>>>>> to be
> >>>>>> there), even if not optimal. Could we make it a default value like
> >>>>>> that
> >>>>>> instead of the current default which is null (and hence the problem).
> >>>>>
> >>>>> IIRC, my initial fix was something similar to that, but it got shot
> >>>>> down
> >>>>> because putting the lock files in a known world writeable location
> >>>>> was a
> >>>>> security issue.
> >>>>>
> >>>>> Although maybe if we put them in a subdirectory of /tmp and ensured
> >>>>> that
> >>>>> the permissions were such that only the user running the service could
> >>>>> use that directory, it might be acceptable?  We could still log a
> >>>>> warning if we wanted.
> >>>>>
> >>>>> This seems like it would have implications for people running services
> >>>>> on Windows too, but we can probably find a way to make that work if we
> >>>>> decide on a solution.
> >>>>
> >>>> How is that a security issue? Are the lock files being written with
> >>>> some
> >>>> sensitive data in them and have g or o permissions on? The sticky bit
> >>>> (+t) on /tmp will prevent other users from deleting the file.
> >>>>
> >>>
> >>> Right, but it won't prevent users from creating a symlink with the same
> >>> name.
> >>>
> >>> ln -s /var/lib/nova/instances/xxxxx/image.raw /tmp/well.known.location
> >>>
> >>> Now when you do
> >>>
> >>> with open('/tmp/well.known.location', 'w') as lockfile:
> >>>   lockfile.write('Stuff')
> >>>
> >>> Nova has just truncated the image file and written 'Stuff' to it.
> >>
> >> So that's the generic case (and the way people often write this). But
> >> the oslo lockutils code doesn't work that way. While it does open the
> >> file for write, it does not actually write, it's using fcntl to hold
> >> locks. That's taking a data structure on the fd in kernel memory (IIRC),
> >> so it correctly gives it up if the process crashes.
> >>
> >> I'm not saying there isn't some other possible security vulnerability
> >> here as well, but it's not jumping out at me. So I'd love to understand
> >> that, because if we can close that exposure we can provide a working
> >> default, plus a strong recommendation for how to do that *right*. I'd be
> >> totally happy with printing WARNING level at startup if lock_path = /tmp
> >> that this should be adjusted.
> > 
> > Full disclosure: I don't claim to be a security expert, so take my
> > thoughts on this with a grain of salt.
> > 
> > tldr: I still don't see a way to do this without breaking _something_.
> > 
> > Unfortunately, while we don't actually write to the file, just opening
> > it for write access truncates it.  So there remains the issue Clint
> > raised if someone can make that file a link.  I suppose we could check
> > for the bad things people might do, but I'm pretty sure that way lies
> > madness.
> 
> Fair point, more coffee before I make statements like that I guess :).
> 
> Follow up question: could we change it to open in "a" mode instead.
> Still requires write access, but won't truncate.
> 

That would still allow them to DOS you by holding the locks
indefinitely.

> > As a followup to my suggestion to have lockutils create something like
> > /tmp/oslo-lock-path and make it accessible only to the owner, I think
> > there's still a problem because a malicious user could create that
> > directory ahead of time and when OpenStack is run for the first time it
> > would happily use the existing directory and potentially overwrite any
> > linked files in it.  There might be ways to avoid that, but I suspect
> > they would be racy (which I believe is why you're not supposed to use
> > tmp paths that are known ahead of time).  Granted, I'm not sure you
> > should be installing to a system that already has malicious users on it,
> > but I suspect security people would be less than thrilled with that answer.
> > 
> > So the workable options that I see right now are to have it break on
> > upgrade if lock_path is not set, or continue to break external locking
> > by having it fall back to process locks when lock_path is not set, as
> > Mark suggested.  I'm not wild about either so I'd love to hear if anyone
> > has a third option.
> 
> Honestly, I'd love us to be clever and figure out a not dangerous way
> through this, even if unwise (where we can yell at the user in the LOGs
> loudly, and fail them in J if lock_dir=/tmp) that lets us progress
> through this while gracefully bringing configs into line.
>

I'm not very clever, so I tend to be afraid of clever solutions. :-P