Open Stack

On 12/06/2013 05:40 PM, Ben Nemec wrote:
> On 2013-12-06 16:30, Clint Byrum wrote:
>> Excerpts from Ben Nemec's message of 2013-12-06 13:38:16 -0800:
>>>
>>>
>>> On 2013-12-06 15:14, Yuriy Taraday wrote:
>>>
>>> > Hello, Sean.
>>> >
>>> > I get the issue with upgrade path. User doesn't want to update
>>> config unless one is forced to do so.
>>> > But introducing code that weakens security and let it stay is an
>>> unconditionally bad idea.
>>> > It looks like we have to weigh two evils: having troubles upgrading
>>> and lessening security. That's obvious.
>>> >
>>> > Here are my thoughts on what we can do with it:
>>> > 1. I think we should definitely force user to do appropriate
>>> configuration to let us use secure ways to do locking.
>>> > 2. We can wait one release to do so, e.g. issue a deprecation
>>> warning now and force user to do it the right way later.
>>> > 3. If we are going to do 2. we should do it in the service that is
>>> affected not in the library because library shouldn't track releases
>>> of an application that uses it. It should do its thing and do it
>>> right (secure).
>>> >
>>> > So I would suggest to deal with it in Cinder by importing
>>> 'lock_path' option after parsing configs and issuing a deprecation
>>> warning and setting it to tempfile.gettempdir() if it is still None.
>>>
>>> This is what Sean's change is doing, but setting lock_path to
>>> tempfile.gettempdir() is the security concern.
>>
>> Yuriy's suggestion is that we should let Cinder override the config
>> variable's default with something insecure. Basically only deprecate
>> it in Cinder's world, not oslo's. That makes more sense from a library
>> standpoint as it keeps the library's expected interface stable.
> 
> Ah, I see the distinction now.  If we get this split off into
> oslo.lockutils (which I believe is the plan), that's probably what we'd
> have to do.
> 
>>>
>>> Since there seems to be plenty of resistance to using /tmp by default,
>>> here is my proposal:
>>>
>>> 1) We make Sean's change to open files in append mode. I think we can
>>> all agree this is a good thing regardless of any config changes.
>>>
>>> 2) Leave lockutils broken in Icehouse if lock_path is not set, as I
>>> believe Mark suggested earlier. Log an error if we find that
>>> configuration. Users will be no worse off than they are today, and if
>>> they're paying attention they can get the fixed lockutils behavior
>>> immediately.
>>
>> Broken how? Broken in that it raises an exception, or broken in that it
>> carries a security risk?
> 
> Broken as in external locks don't actually lock.  If we fall back to
> using a local semaphore it might actually be a little better because
> then at least the locks work within a single process, whereas before
> there was no locking whatsoever.

Right, so broken as in "doesn't actually locks, potentially completely
scrambles the user's data, breaking them forever."

	-Sean

-- 
Sean Dague
http://dague.net