Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

47 HFT WinCred persistence bug #124

Merged
merged 3 commits into from
Jul 14, 2020
Merged

47 HFT WinCred persistence bug #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12fc843
Fixed issue of not being able to find HFT-created Windows host creds
sgnn7 Jul 13, 2020
81e5f2b
Add warning when Linux identity is unable to be found on the machine
sgnn7 Jul 13, 2020
79554a2
Ensure that we don't break when Windows registry is missing our keys
sgnn7 Jul 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
v6-compatible APIs for CA chain retrieval.
[cyberark/conjur-puppet#44](https://github.com/org/repo/issues/44)

### Fixed
- Fix windows credential search for HFT-created identities.
[cyberark/conjur-puppet#47](https://github.com/org/repo/issues/47)
- Fix windows registry exceptions on new HFT-based hosts
[cyberark/conjur-puppet#112](https://github.com/org/repo/issues/112)

## [2.0.3] - 2020-05-10
### Changed
- We now encode the variable id before retrieving it from Conjur v5.
Expand Down
17 changes: 13 additions & 4 deletions lib/conjur/puppet_module/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,21 @@ def from_registry
unless Puppet.features.microsoft_windows?

require 'win32/registry'
c = Win32::Registry::HKEY_LOCAL_MACHINE.open(REG_KEY_NAME) do |reg|
# Convert registry value names from camel case to underscores
# e.g. ApplianceUrl => appliance_url
reg.map { |name, _type, data| [name.gsub(/(.)([A-Z])/, '\1_\2').downcase, data] }.to_h

c = {}
begin
Win32::Registry::HKEY_LOCAL_MACHINE.open(REG_KEY_NAME) do |reg|
# Convert registry value names from camel case to underscores
# e.g. ApplianceUrl => appliance_url
c = reg.map { |name, _type, data| [name.gsub(/(.)([A-Z])/, '\1_\2').downcase, data] }.to_h
end
rescue
Puppet.warning "Agent’s registry did not contain path #{REG_KEY_NAME}. If this is the " +
"first time using HFTs on this node, this is expected behavior."
end

c['ssl_certificate'] ||= File.read c['cert_file'] if c['cert_file']

c
end
end
Expand Down
25 changes: 21 additions & 4 deletions lib/conjur/puppet_module/identity.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@ def from_file(uri, config)
when 'password'
password = value if found
end

return [login, password] if login && password
end

warn "Could not find conjur authentication info for host '#{uri}'" if not found
end
end

Expand All @@ -41,10 +44,24 @@ def from_wincred(uri)

require 'wincred/wincred'

WinCred.enumerate_credentials
.select { |cred| cred[:target].start_with?(uri.to_s) || cred[:target] == uri.host }
.map { |cred| [cred[:username], cred[:value].force_encoding('utf-16le').encode('utf-8')] }
.first
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}" || \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely an improvement!

  1. 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.
  2. I'm not sure (right this second) why we need this criteria cred[:target].start_with?(uri.to_s)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

cred[:target] == uri.host
end

if matching_creds.empty?
Puppet.warning "Couldn't find any pre-populated Conjur credentials in WinCred " +
"storage for #{uri}"
return []
end

# We select the first one if there's multiple matches
matching_cred = matching_creds.first

Puppet.debug "Using Conjur credential '#{matching_cred[:target]}' for identity"
[matching_cred[:username], matching_cred[:value].force_encoding('utf-16le').encode('utf-8')]
end
end
end
Expand Down