Open Stack

On 12/09/2013 11:38 AM, Clint Byrum wrote:
> Excerpts from Sean Dague's message of 2013-12-09 08:17:45 -0800:
>> 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."
>>
> 
> Things I'd like to see OpenStack do in the short term, ranked in ascending
> order of importance:
> 
> 4) Upgrade smoothly.
> 3) Scale.
> 2) Help users manage external risks.
> 1) Not do what Sean described above.
> 
> I mean, how can we even suggest silently destroying integrity?
> 
> I suggest merging Sean's patch and putting a warning in the release
> notes that running without setting this will be deprecated in the next
> release. If that is what this is preventing this debate should not have
> happened, and I sincerely apologize for having delayed it. I believe my
> mistake was assuming this was something far more trivial than "without
> this patch we destroy users' data".
> 
> I thought we were just talking about making upgrades work. :-P

Honestly, I haven't looked exactly how bad the corruption would be. But
we are locking to handle something around simultaneous db access in
cinder, so I'm going to assume the worst here.

	-Sean

-- 
Sean Dague
http://dague.net