Closed ferringb closed 6 years ago
Generally agree, but there are parts of this that should be fixed in the transport package.
agreed regarding pushing this more into the transport layer- while we're at it, we should also eliminate the File.Set() logic for cert/keys and move it in there to remove the potential permissions race if umask is set to something insane.
Fix certmgr ensure to only trigger actions when content has changed.
First, CA.loaded was removed because it wasn't used for anything.
Second, CA.Load() was gutted so it no longer writes the CA to disk- that's CA.Refresh()'s job, and it's critical that Refresh() handle this since manager instances need to know if actions should be triggered.
Third, strip space from the CA when we read it from disk- we seem to be picking up a trailing newline from WriteFile (which is good for humans) but it breaks our byte comparison for what we receive from the remote.
The test case to trigger this basically comes down to just having an action in place, and invoking
certmgr ensure --debug
twice; the first run may update things as needed, but the second run should not trigger an action, but would.With these logic changes, this no longer happens.
Fix certmgr refreshing logic for when the spec has been updated.
This sequence would fail and result in they key existing, but the cert missing. Another
certmgr ensure
invocation would be required to repair it.The cause is that certmgr would try and force cfssl to update the cert/key via unlink'ing the cert. The problem here is that cfssl maintains in memory timestamps to determine when something should be renewed- and as far as it was concerned, the cert was still valid, thus wouldn't regenerate it. The underlying provider layer also wouldn't force a refresh since it thinks the content is still on disk.
Thus this approach; reach in and reset timestamps in cfssl so that when it's asked to refresh, it thinks a refresh is due- and actually does the refresh.