(PUP-7283) Disable CRL checking when fetching the CRL #5725
Conversation
|
CLA signed by all contributors. |
lib/puppet/context.rb
Outdated
| @@ -40,6 +40,7 @@ def pop | |||
| if @stack[-1][0] == 0 | |||
| raise(StackUnderflow, "Attempted to pop, but already at root of the context stack.") | |||
| else | |||
| @id -= 1 | |||
There was a problem hiding this comment.
Was this just a bug that already existed? This changes us from always having unique context ID in the stack to possibly having duplicates. I'm not super familiar with the system - is that safe?
There was a problem hiding this comment.
The @id field isn't directly exposed and you can only really observe it by stepping into the class via the debugger. I assumed the id was meant to indicate the context depth but it could be a unique identifier. Is that more helpful, identifying the context or stack depth?
But since this isn't exposed anywhere I can drop this commit and we can pretend this question never came up.
There was a problem hiding this comment.
¯_(ツ)_/¯. I looked further at it and agree its innocuous.
There was a problem hiding this comment.
I'm pretty sure @id is meant to be a unique id for things in the stack, not the stack depth. Although it isn't exposed outside of this class, I'm thinking we shouldn't modify it's behavior, especially since it's not meaningful to this PR.
| @@ -7,4 +7,16 @@ class Puppet::SSL::CertificateRevocationList::Rest < Puppet::Indirector::REST | |||
| use_server_setting(:ca_server) | |||
| use_port_setting(:ca_port) | |||
| use_srv_service(:ca) | |||
|
|
|||
| def find(request) | |||
There was a problem hiding this comment.
Would we ever execute a find via the file terminus and run into the same problem?
There was a problem hiding this comment.
We have to have this guard clause because we might need to make network operations in order to configure SSL, which causes infinite recursion. With the file terminus the file is there or it's not, and we won't recurse.
|
@adrienthebo are you able to do a local acceptance run against this? |
|
@MosesMendoza I've run the tests locally and I don't get the infinite recursion that we did last time. However I was getting a failure on the autosign command tests but that was also failing against the 4.9.4 tag, so I don't know if it's related. I'm going to investigate more and see if the failure is related or due to some precondition that another test was satisfying. |
|
@branan @MosesMendoza I've been getting a lot of failures when running beaker locally, and they generally seem unrelated to the changes I've made. The autosigning failure that I've seen is perplexing but it happens on master and my branch, so I'm baffled. May we try merging this and see if it works? If nothing else the infinite recursion looks fixed. (I'd also like to get this wrapped up before the end of the sprint tomorrow) |
|
@adrienthebo sounds good. puppet/master tests are currently red but good to merge when green. |
|
Thank you! |
| @@ -7,4 +7,16 @@ class Puppet::SSL::CertificateRevocationList::Rest < Puppet::Indirector::REST | |||
| use_server_setting(:ca_server) | |||
| use_port_setting(:ca_port) | |||
| use_srv_service(:ca) | |||
|
|
|||
| def find(request) | |||
| if request.method == :find && !Puppet::FileSystem.exist?(Puppet[:hostcrl]) | |||
There was a problem hiding this comment.
Is the request.method == :find check redundant?
There was a problem hiding this comment.
Thanks for catching that, I originally implemented this in #network where this check was needed. When I shifted the implementation I didn't drop this check. It's gone now.
…factor-ssl-validators"" This reverts commit 01d5d51.
The poor lifecycle management of Puppet's SSL implementation can lead to cases where trying to fetch the CRL causes us to try to fetch the CRL. This commit makes Puppet resemble Ouroboros a bit less by specifically disabling CRL checking when we're fetching the CRL.
|
@MosesMendoza it looks like the puppet master component tests are passing, can this get merged today? |
|
Since this has a thumb from @MosesMendoza pending a green master, master is green, and I am also 👍 : I'm hitting the big green button. Nobody can stop me. NOBODY! MUAHAHAHAHAHAHAHA! |
When we're fetching the CRL for the first time and a CRL hasn't been provided out of band, we need to disable CRL checking until that request has completed.
I've also included a graphical representation of the indirector for anyone interested.