On Dec 16, 2009, at 7:49 PM, Markus Roberts wrote: >>> * The expiration of a cached certificate is the earlier of the >>> envelope >>> expiration and the certificate's expiration, as opposed to just >>> overriding >>> the cache value >> >> Have you tested this in the real world? That is, default expiration >> is usually something like 30 minutes; is this value stored in the >> cert >> somehow such that a cert is considered expired even though it's valid >> for five years? > > The "expiration" getter is only ever called by the caching stuff, at > least as far as I can find after doing an extensive code search. Yes, > it's possible that somebody computes :expression somehow and sends it > to a certificate, but that would be aberrant behaviour and ought to be > corrected. For that matter, even confounding the cache's expiration > attribute with the certificate's "content.not_after" value could be > considered a bug (an analog to the use of "alias" to mean both host > alias and resource alias).
I guess my question on the expiration is, will the caching system destroy certs that are 31 minutes old but don't actually expire for 5 years (minus 31 minutes)? I can't imagine how the certs could be storing the expiration data unless you're now serializing them rather than storing them as plain PEM files, but if they're not storing it, then this is essentially pointless. In terms of conflating them purposes, I think it actually makes sense, inasmuch as it makes sense to abuse the concept of a cache the way we are. The cert really should be considered valid until that 'not_after' value gets triggered, and it should be discarded after it. Given that we're already abusing the concept of cache, this seems essentially appropriate. >>> * Telling the cache to expire an item now removes it from the >>> cache if >>> possible, rather than just setting an expiration date in the past >>> and >>> hoping that somebody notices. >> >> I'm confused as to why these are relevant. It sounds like you're >> triggering expiration; couldn't you just as easily trigger removal >> via >> 'destroy'? > > You can't directly access the cache from outside, so calling destroy > directly on the cache isn't an option, and it doesn't seem like it's > worth puncturing the abstraction for. Calling while > Certificate.destroy() while you're trying to get the certificate > causes stack overflow, and attempting to untangle that got messier > than I felt was worth it. Right - this is the key I was missing: It's the cache, not the normal terminus. >> Once again, this is also at least two significantly different changes >> - you've done a significant refactor and changed behaviour, and I >> can't at all see which is which. > > I enumerated the behaviour changes in the commit message. The > refactoring was all small scale (mostly replacing control structures > with boolean & list operators to clear out the clutter). Excluding > the tests, the changes break down roughly as: > > Soft --> Hard cache expiration 4 lines > Expire certs with the wrong key 5 lines > Removing previous signed keys 4 lines > Message enhancements 3 lines > Comment changes 4 lines > Removing stray whitespace, dead vars, 3 lines > Code smell reduction 41 lines removes, 11 > added, 8 changed > > The code smell reductions are all semantically neutral, and are for > the most part locally verifiable; while I can see from a minimal patch > prospective that they aren't core to making the patch, we've had an > intermittent policy of "fix things when you see them" and it seems > like the most efficient way to get them fixed. > > If you insist, I can split them out, but it seems pointless. I won't insist for this patch, but in general, I think this kind of mixing makes it much harder to read a given patch. For instance, say this commit introduces a bug - we have to work much harder to figure out whether the bug is a result of a refactor vs. the bugfix you're implementing. I still agree on fixing things when you see them, and really, that's gotten more important now that more people are involved more often, but if possible I'd like them to be pulled into a separate commit. -- Life is like playing a violin in public and learning the instrument as one goes on. -- 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.
