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.

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

Dead code #173

Open
rtib opened this issue Dec 18, 2023 · 1 comment
Open

Dead code #173

rtib opened this issue Dec 18, 2023 · 1 comment

Comments

@rtib
Copy link
Contributor

rtib commented Dec 18, 2023

I was reviewing the below code, because of suspected failing its purpose:

ini_file = Puppet::Util::IniConfig::PhysicalFile.new(resource[:template])
if (req_ext = ini_file.get_section('req_ext'))
if (value = req_ext['subjectAltName']) && (value.delete(' ').gsub(%r{^"|"$}, '') != alt_name.delete(' ').gsub(%r{^"|"$}, '').gsub('IPAddress', 'IP'))
return false
end
elsif (req_dn = ini_file.get_section('req_distinguished_name'))
if (value = req_dn['commonName']) && (value != cdata['CN'])
return false
end
end

My analysis unveiled, that neither the if nor the elsif branches are ever executed. This is because the ini_file's initialize method does not read and parse the file itself, thus each call to its get_section method is returning nil.

An instance of Puppet::Util::IniConfig::PhysicalFile would need the invocation of its read method to read and parse the file, but that's never happen. Using this class to parse an OpenSSL config file constructed by this module's templates/cert.cnf.epp would, however, end up raising IniParseError with the message Property with key HOME outside of a section from puppet/util/inifile.rb#L185. AFAIK, there is no specification for the format of ini files, however, Microsoft and OpenSSL seem to treat them differently. Nevertheless, Puppet::Util::IniConfig::PhysicalFile.parse method does not support keys before the first section header, thus it seems not suitable for parsing OpenSSL style config files.

In that sense the old_cert_is_equal method of the x509_cert openssl provider would need to be reviewed.

@ekohl
Copy link
Member

ekohl commented Jul 19, 2024

I agree that's surprising to say the least. Just because there's a SAN doesn't mean there is no CN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants