[squid-dev] [PATCH] convert digest_nonce_h to MEMPROXY_CLASS

[Amos Jeffries]

On 28/09/2015 6:40 a.m., Kinkie wrote:
> On Sun, Sep 27, 2015 at 12:51 PM, Amos Jeffries <squid3 at treenet.co.nz> wrote:
>> On 27/09/2015 10:30 p.m., Kinkie wrote:
>>> .. and remove one XXX in the process.
>>> Not much to add besides the subject.
>>>
>>> build- and lightly run-tested, seems to be ok.
>>> Big-context patch attached.
>>>
>>
>> Two big problems stand right out.
>>
>> * The static random and distribution objects were specificaly internal
>> to a function in order so that they got initialized on first use, but
>> did not get initialized if never used.
>>
>> That is somewhat important to a) save the machine available entropy bits
>> for other useswhen Digest is not configured, and b) decrease the
>> probability that the workers initialize them within the same second -
>> they only get initialized on first traffic request handled, not during
>> the startup second. And so the seeding can survive fork()'ing.
> 
> Whoops, I had misinterpreted the intention in this code as stated in
> the first comment in authenticateDigestNonceNew. Is that not
> intentional?
> But if the problem is unique seeding, isn't it also useful to mix time
> with a known-unique quantity, e.g. the PID?

Not really, that increases the chance of seed collisions between
seconds.  NOW() + (PID + n) == (NOW() + n) + PID

Since PID values loop we end up with different (cyclic) periods of time
sharing seeds with identical values. Whereas with just time every second
is a new seed, and we only have to worry about avoiding seeding twice in
one second.

If the particular seconds was a big issue between workers we could just
dig for deeper resolution of time to seed with.


> In the meantime I've moved these data members as static members of
> digest_nonce_data::refreshRandomData() and referenced that from the
> ctor .
> 

Okay. Good.

>> * the contents of authenticateDigestNonceNew() should be in the nonce
>> object ctor, or at least a method called by it.
> 
> Are you sure? It seems to me that it's actually the other way around:
> authenticateDigestNonceNew is the only place where digest_nonce_data
> gets instantiated, I've factored the POD instantiation code out of
> digest_nonce_data to the ctor. The code could certainly be improved,
> e.g. by moving the nonces registry and the nonce base64 encoding as a
> static member of digest_nonce_h . It may be argued that the comment
> block in authenticateDigestNonceNew could be moved elsewhere, I don't
> really know what could be the best location for it tho.

I believe the RBC /**/ block is related to the ctor initialization
values which authenticateDigestNonceNew() as a whole was creating. The
choice of initializer values is part of that, the randomness objects and
while loop was enforcing the criteria.
 Which is why I think the whole of that functions body need to be ctor.
So it stays together as a unit constructing a nonce object. With the RBC
comment retained.


The //NP: block I added is specifically about the std:: random objects
and their use of time(). It should stay above them in their new home.


> 
>> * the contents of authenticateDigestNonceDelete() should be the nonce
>> objects dtor.
> 
> Not unless we move the cache in the nonce_data class. It's doable but
> I consider it out of scope for this effort. I've added an XXX about
> that.

The latest changes to the bug4190 cache give it an agnostic key. I'm
thinking long term the nonce cache can just re-use that cache code by
passing the nonce (or whatever) as the key.

Amos