47 HFT WinCred persistence bug #124
Conversation
|
@sgnn7 before this wraps up, can you please file an issue to add tests for this and include it in the epic? |
lib/conjur/puppet_module/config.rb
Outdated
| c = reg.map { |name, _type, data| [name.gsub(/(.)([A-Z])/, '\1_\2').downcase, data] }.to_h | ||
| end | ||
| rescue | ||
| Puppet.notice "Agent's registry did not contain path #{REG_KEY_NAME}. This is normal " + |
There was a problem hiding this comment.
I'm not sure this is always normal e.g. when this error is thrown when using HFTs for the (n+1)-th time
There was a problem hiding this comment.
Sure - that's why the disclaimer is there for "when using HFTs for the first time on a node" on the second line. I'm not sure we can improve this too much since it's an edge case where this condition is not a very suspicious state on the node.
| Puppet.debug "Finding Conjur credentials in WinCred storage for uri: #{uri}" | ||
| matching_creds = WinCred.enumerate_credentials.select do |cred| | ||
| cred[:target].start_with?(uri.to_s) || \ | ||
| cred[:target] == "#{uri.host}:#{uri.port}" || \ |
There was a problem hiding this comment.
This is definitely an improvement!
- I worry that this is still brittle since the regex in
init.ppisn't parsing the URL and might not gethostorhost+port. - I'm not sure (right this second) why we need this criteria
cred[:target].start_with?(uri.to_s)
There was a problem hiding this comment.
I worry that this is still brittle since the regex in init.pp isn't parsing the URL and might not get host or host+port.
It's brittle for sure but keep in mind that for this we are explicitly just fixing the HFT usage which has a very predictable credential name pattern that it will save to the cred manager.
I'm not sure (right this second) why we need this criteria cred[:target].start_with?(uri.to_s)
I fully agree and I think it's effectively doing just a bad version of <scheme>://<host>:<port> comparison which is the pattern we use in README but I explicitly didn't want to possibly change the behavior for current users/setups.
There was a problem hiding this comment.
I think we should create an issue to make this more robust. It applies to any scenario where credentials are committed on the agent coming from the puppet server (including HFTs)
This code previously searched for windows credentials that did not match saved ones when using host-factory tokens on windows (namely `<host>:<port>`), causing Windows hosts to keep recreating their host identity with the HF token. This would result in inability to fetch secrets once the HFT token would expire but with this fix we find our host credentials in the WinCred storage and can avoid recreating the host identity.
This warning will help troubleshoot machines with better error logging for when we have a *nix `conjur.conf` file but the matching identity cannot be found.
sgnn7
force-pushed
the
47-hft-wincred-persistence-bug
branch
from
57d1d6b to
7982925
Compare
This situation is common when using HFTs where the registry will be empty initially. Here we just ensure that errors result in no fatal paths to make sure that HFTs can work properly.
sgnn7
force-pushed
the
47-hft-wincred-persistence-bug
branch
from
7982925 to
79554a2
Compare
Agreed! Created the improvements issue as a tracker for that under #127 |
What does this PR do?
hiera-backed on newly-provisioned machines causes errors #112)What ticket does this PR close?
Connected to #47
Connected to #112
Checklists
Change log
Test coverage
Documentation
READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or