On Sep 18, 2009, at 11:33 AM, Brice Figureau wrote:
>
> On 18/09/09 20:28, Luke Kanies wrote:
>> On Sep 18, 2009, at 11:09 AM, Brice Figureau wrote:
>>
>>> On 18/09/09 19:32, Luke Kanies wrote:
>>>> On Sep 18, 2009, at 9:37 AM, Markus Roberts wrote:
>>>>
>>>>>> I think I remember the following cause for the bug:
>>>>> * 0.24 generates a CA whose CN=$ca_fqdn.
>>>>> * upgrade to 0.25
>>>>> * 0.25 client connects to master, ask for "ca" cert
>>>>> * master send cert whose CN=$ca_fqdn
>>>>> * Clients wants to locally write it as $ca_fqdn.pem, so
>>>>> thinks it is a normal cert, not a CA.
>>>>> * Clients can not authenticate the server because
>>>>> there is no "ca.pem" file.
>>>>>
>>>>> But if Luke found his patch fixes #2617, it might be that I didn't
>>>>> get
>>>>> exactly the issue and the above is wrong.
>>>>>
>>>>> Another possibility would be that our testing was flawed (I don't
>>>>> think it was, but it's a logical possibility). If that's the
>>>>> case,
>>>>> the most obvious difference would be that we were testing on a
>>>>> single machine--this could conceivably lead to the client seeing
>>>>> some file that was "on the master" if they shared a confdir, or
>>>>> some
>>>>> other such mechanism.
>>>>>
>>>>> Probably the easiest way to rule out all these possibilities in
>>>>> one
>>>>> go is to try it in an environment that was known to fail for you
>>>>> with 25.0; if it still fails, we haven't fixed the problem,
>>>>> while if
>>>>> it no longer exhibits the symptoms the chances are good that the
>>>>> ticket is fixed.
>>>> Ok, I looked into this more closely. The following code fixes it
>>>> for
>>>> me:
>>> Were you able to reproduce the issue I was seeing?
>>
>> Was that separate from the main issue?
>
> No, I was seeing exactly #2617 and #2619. I meant in fact: were you
> able
> to get locally a scenario matching the one I outlined in my previous
> e-mail. I guess the answer is yes, based on your last patch.
>
>>>> diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/
>>>> rest.rb
>>>> index e1ee89f..ec3f70f 100644
>>>> --- a/lib/puppet/indirector/rest.rb
>>>> +++ b/lib/puppet/indirector/rest.rb
>>>> @@ -66,7 +66,11 @@ class Puppet::Indirector::REST <
>>>> Puppet::Indirector::Terminus
>>>> end
>>>>
>>>> def find(request)
>>>> - deserialize network(request).get(indirection2uri(request),
>>>> headers)
>>>> + return nil unless result =
>>>> deserialize(network(request).get(indirection2uri(request),
>>>> headers))
>>>> + unless result.name == request.key
>>>> + result.name = request.key
>>>> + end
>>>> + result
>>>> end
>>>>
>>>> def search(request)
>>> Ouch, that's a central change for sure :-)
>>>
>>>> In other words, when we find a cert via REST, we're converting it
>>>> to
>>>> the PEM-formatted cert, with just the cert name, which may or may
>>>> not
>>>> be equal to the name we asked for. This basically just makes sure
>>>> that we return an instance whose name matches the requested name,
>>>> regardless of what's used internally.
>>> Indeed that should fix the issue I saw.
>>>
>>>> I think this is actually the right behaviour, because it allows
>>>> some
>>>> separation between internals and the envelope (which separation is
>>>> the
>>>> problem here).
>>> Yes, I do think this is right.
>>>
>>>> The only question is whether this is the right
>>>> behaviour for everything, and it's a potentially large change in
>>>> behaviour, at least in terms of the number of classes it could
>>>> affect.
>>> I'm afraid the change is a little bit too central, and it is
>>> difficult
>>> to evaluate the side effects.
>>>
>>> Wouldn't it be better to just make this change in the
>>> certifiicate_authority/rest.rb file?
>>
>> It's actually in certificate/rest.rb, but...
>
> Ah, ok. The above patch is in indirector/rest so for every
> indirection,
> hence my question.
Sorry; my point was that the patch should be for certificate/rest.rb,
not ca/rest.rb.
>
>> I'm of two minds. On the one hand, that does keep the change to a
>> minimum, but on the other hand, it opens us up to exactly this kind
>> of
>> bug in the future.
>>
>> Maybe two patches? One for 0.25.x with the minimal change, and one
>> for master that fixes it for everyone?
>
> Sounds the best to me. It leaves us a whole major release cycle to
> detect misbehaviour and side effects and at the same time fixes the
> issue for 0.25.x.
Right. You should see a patch in a minute.
--
Is life worth living? That is a question for an embryo, not a man.
--Samuel Butler
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
