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

Use native OpenSSL methods to automatically determine the PKey #189

Closed
wants to merge 1 commit into from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented May 22, 2024

This came up in #187 (comment) but it's probably also needed for #167.

One thing to note is that we may have more problems in the future. For example, on my Fedora I don't appear to be allowed to generate any DSA key in the default SSL policy. I imagine future enterprise distros will follow this exampe.

@ekohl ekohl force-pushed the use-better-openssl-methods branch from 54fc649 to 3f01ccb Compare May 22, 2024 14:04
Comment on lines +16 to +20
options[:dsa_paramgen_bits] = resource[:size] if resource[:size]
when :rsa
OpenSSL::PKey::RSA.new(resource[:size])
options[:rsa_keygen_bits] = resource[:size] if resource[:size]
when :ec
OpenSSL::PKey::EC.new(resource[:curve]).generate_key
else
raise Puppet::Error,
"Unknown authentication type '#{resource[:authentication]}'"
options[:ec_paramgen_curve] = resource[:curve] if resource[:curve]
Copy link
Member Author

Choose a reason for hiding this comment

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

These options were found via man openssl genpkey.

@@ -7,7 +7,7 @@
require 'openssl'
describe 'The POSIX provider for type cert_file' do
before do
test_keys = OpenSSL::PKey::RSA.new(2049)
Copy link
Member Author

Choose a reason for hiding this comment

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

2049 bits was very specific. I wonder if that was intentional.

Copy link
Member

Choose a reason for hiding this comment

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

Probably a typo.

Note: using short keys in unit tests when a lot of keys have to be generated can speed up the tests significantly. Is it worth here?

@ekohl
Copy link
Member Author

ekohl commented May 22, 2024

So generate_key is too new. I'll need to rethink this a bit for older OpenSSL versions.

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

So generate_key is too new. I'll need to rethink this a bit for older OpenSSL versions.

IIRC this new API had no equivalent before, and for older Ruby we must use OpenSSL::PKey::Foo.new.

Maybe we can keep the code as it is today for generate key, but already move to agnostic key type loading?

@@ -7,7 +7,7 @@
require 'openssl'
describe 'The POSIX provider for type cert_file' do
before do
test_keys = OpenSSL::PKey::RSA.new(2049)
Copy link
Member

Choose a reason for hiding this comment

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

Probably a typo.

Note: using short keys in unit tests when a lot of keys have to be generated can speed up the tests significantly. Is it worth here?

@ekohl ekohl force-pushed the use-better-openssl-methods branch from 3f01ccb to 37e2ce8 Compare July 18, 2024 17:42
@ekohl ekohl mentioned this pull request Jul 18, 2024
case resource[:authentication]
when :dsa
OpenSSL::PKey::DSA.new(resource[:size])
options[:dsa_paramgen_bits] = resource[:size] if resource[:size]
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it time to drop DSA? We need to do a major release anyway and there's a reason why modern distributions don't support it anymore or block it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. #222

@ekohl
Copy link
Member Author

ekohl commented Jul 18, 2024

New attempt in #223. That now passes on Fedora 40.

@ekohl
Copy link
Member Author

ekohl commented Jul 18, 2024

Replaced by #167.

@ekohl ekohl closed this Jul 18, 2024
@ekohl ekohl deleted the use-better-openssl-methods branch July 18, 2024 22:44
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

Successfully merging this pull request may close these issues.

3 participants