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

Allow PKCS8 EC private keys to be loaded #61

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow PKCS8 EC private keys to be loaded #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
- Added support for RFC5424 structured data [#67](https://github.com/logstash-plugins/logstash-output-syslog/pull/67)
- The SNI (Server Name Indication) extension is now used when connecting to syslog server with TLS and `host` is set to FQDN (Fully Qualified Domain Name) [#66](https://github.com/logstash-plugins/logstash-output-syslog/pull/66)
- Add support for CRL to check for the server certificate is revocation status [#62](https://github.com/logstash-plugins/logstash-output-syslog/pull/62)
- Support loading of PKCS8 EC private keys [#61](https://github.com/logstash-plugins/logstash-output-syslog/pull/61)

## 3.0.5
- Docs: Set the default_codec doc attribute.
Expand Down
2 changes: 1 addition & 1 deletion lib/logstash/outputs/syslog.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ def setup_ssl
require "openssl"
ssl_context = OpenSSL::SSL::SSLContext.new
ssl_context.cert = OpenSSL::X509::Certificate.new(File.read(@ssl_cert))
ssl_context.key = OpenSSL::PKey::RSA.new(File.read(@ssl_key),@ssl_key_passphrase)
ssl_context.key = OpenSSL::PKey::read(File.read(@ssl_key),@ssl_key_passphrase)
Copy link
Contributor

Choose a reason for hiding this comment

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

@given that you have added all the TLS test scaffolding in #62 do you think that, after the merge on main branch of that PR and rebase of this, could we add 2 tests here, one for RSA key and the other for Elliptic Cryptography here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes absolutely, let's do that.

Copy link
Contributor Author

@tsaarni tsaarni Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The diff looks bit confusing, but this is what was done:

I added RSA and EC test but at the same time I needed to remove test for invalid client private key, which was previously added in TLS tests. I now realized that OpenSSL::PKey::read() does not raise error when given invalid PEM file, unlike OpenSSL::PKey::RSA.new did. Instead, it returns instance of OpenSSL::PKey::RSA even though invalid.pem clearly is not an RSA key.

When I fixed support for EC pkey to PKey::read() in jruby-openssl, I did not check this. Maybe it lacks negative checks.

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 that it worthwhile to open an issue in https://github.com/jruby/jruby-openssl repository, accepting silently an improper file is misleading.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the difference is that the read method instantiate the RSA key
https://github.com/jruby/jruby-openssl/blob/19135259f121f9c9e95ee7f0b4c830f9267680e0/src/main/java/org/jruby/ext/openssl/PKey.java#L124

instead of the initiliaze method
https://github.com/jruby/jruby-openssl/blob/19135259f121f9c9e95ee7f0b4c830f9267680e0/src/main/java/org/jruby/ext/openssl/PKeyRSA.java#L223

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my test case I used input that does not even have valid PEM block inside, so I would have expected that even PEM block parsing should fail here https://github.com/jruby/jruby-openssl/blob/19135259f121f9c9e95ee7f0b4c830f9267680e0/src/main/java/org/jruby/ext/openssl/PKey.java#L117 ->
https://github.com/jruby/jruby-openssl/blob/19135259f121f9c9e95ee7f0b4c830f9267680e0/src/main/java/org/jruby/ext/openssl/x509store/PEMInputOutput.java#L317
Requires some additional debugging on jruby-openssl to find out what is going on there...

I've created new issue jruby/jruby-openssl#285. I added also the CA case which I noticed earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oks, let's see what they answer and then I'll merge this.

if @ssl_verify
cert_store = OpenSSL::X509::Store.new
# Load the system default certificate path to the store
Expand Down
3 changes: 3 additions & 0 deletions spec/fixtures/certs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,6 @@ subject: cn=client
issuer: cn=ca
key_type: RSA
---
subject: cn=client-ec
issuer: cn=ca
key_type: EC
5 changes: 5 additions & 0 deletions spec/fixtures/client-ec-key.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
-----BEGIN PRIVATE KEY-----
MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg6P7i1NqXVKChh8dR
pqHcCSwlxDjKoaDBGiYzWHgy5vqhRANCAAQSX1YGFCuXL7f5Utp5X45+h7ixghyQ
vhYfT4gY6M31DAUaf59DENYUZ36k4IYrWP6lU/ChBH0Mlntjb1TCD+Tw
-----END PRIVATE KEY-----
13 changes: 13 additions & 0 deletions spec/fixtures/client-ec.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
-----BEGIN CERTIFICATE-----
MIICCjCB86ADAgECAggXhLgPAPW4dzANBgkqhkiG9w0BAQsFADANMQswCQYDVQQD
EwJjYTAeFw0yMzA5MTQwODU1MzRaFw0yNDA5MTMwODU1MzRaMBQxEjAQBgNVBAMT
CWNsaWVudC1lYzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABBJfVgYUK5cvt/lS
2nlfjn6HuLGCHJC+Fh9PiBjozfUMBRp/n0MQ1hRnfqTghitY/qVT8KEEfQyWe2Nv
VMIP5PCjMzAxMA4GA1UdDwEB/wQEAwIFoDAfBgNVHSMEGDAWgBRNukfgtxJMkwu7
XMvQ8ETWqi5BVTANBgkqhkiG9w0BAQsFAAOCAQEAP+HsEKYA2d6kCAH/JJSpxMnP
gwMfjDkmV1bMguYSoOv8fbD17WqpyRojhi+THInP6ggXhJW0Zbz6UNy2GHXtO4+o
OGLKI2FMUnaLRDMF4NL//FcC1unRQxyw8HQ2oMPNtWVEoo8KURLe0IW2q9/afT89
59RAZYxizFKSWcoIQGeCoyWzVIa/E+MB4cFKgpTF3zkxr6uWJvXYYwkVtzknsGvW
v0c2h2Ck//kuQatJSZQpbMaYMEE2480VnwskiOTu1ltxrmcQxz5P0g1zcjEnKQAm
kB3ENdewzHIq8yaybbf+a/WCsNyyEjKPOsSWeElk77v719B24x1HqkV8FW/eRA==
-----END CERTIFICATE-----
29 changes: 21 additions & 8 deletions spec/outputs/syslog_tls_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,29 +109,42 @@
context "read PEM" do
let(:options) { { "host" => "localhost", "port" => port, "protocol" => "ssl-tcp", "ssl_verify" => true } }

context "invalid client certificate" do
context "RSA certificate and private key" do
let(:options ) { super().merge(
"ssl_cert" => File.join(FIXTURES_PATH, "invalid.pem"),
"ssl_cert" => File.join(FIXTURES_PATH, "client.pem"),
"ssl_key" => File.join(FIXTURES_PATH, "client-key.pem"),
"ssl_cacert" => File.join(FIXTURES_PATH, "ca.pem"),
"ssl_crl" => File.join(FIXTURES_PATH, "ca-crl.pem")
) }

it "register raises error" do
expect { subject.register }.to raise_error(OpenSSL::X509::CertificateError, /malformed PEM data/)
it "register succeeds" do
expect { subject.register }.not_to raise_error
end
end

context "invalid client private key" do
context "EC certificate and private key" do
let(:options ) { super().merge(
"ssl_cert" => File.join(FIXTURES_PATH, "client.pem"),
"ssl_key" => File.join(FIXTURES_PATH, "invalid.pem"),
"ssl_cert" => File.join(FIXTURES_PATH, "client-ec.pem"),
"ssl_key" => File.join(FIXTURES_PATH, "client-ec-key.pem"),
"ssl_cacert" => File.join(FIXTURES_PATH, "ca.pem"),
"ssl_crl" => File.join(FIXTURES_PATH, "ca-crl.pem")
) }

it "register succeeds" do
expect { subject.register }.not_to raise_error
end
end

context "invalid client certificate" do
let(:options ) { super().merge(
"ssl_cert" => File.join(FIXTURES_PATH, "invalid.pem"),
"ssl_key" => File.join(FIXTURES_PATH, "client-key.pem"),
"ssl_cacert" => File.join(FIXTURES_PATH, "ca.pem"),
"ssl_crl" => File.join(FIXTURES_PATH, "ca-crl.pem")
) }

it "register raises error" do
expect { subject.register }.to raise_error(OpenSSL::PKey::RSAError, /Neither PUB key nor PRIV key/)
expect { subject.register }.to raise_error(OpenSSL::X509::CertificateError, /malformed PEM data/)
end
end

Expand Down