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

Fail hard if SSL certs or keys are invalid #2848

Merged
merged 2 commits into from
Apr 2, 2022

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Mar 27, 2022

Description

Previously if an invalid SSL cert or key (e.g. blank file, mismatching
private and public key, etc.) were specified, Puma would quietly
accept them, bind to the port, and mysteriously fail with an error
only after a client initiates a connection:

SSL error, peer: ::1, peer cert: : #<Puma::MiniSSL::SSLError: OpenSSL error: error:1417A0C1:SSL routines:tls_post_process_client_hello:no shared cipher - 193>

We now check the OpenSSL return values and raise an exception if the
cert or key cannot be loaded. For example:

puma/lib/puma/minissl.rb:330:in `initialize': SSL_CTX_use_certificate_chain_file: error in file '/tmp/cert.txt': error:0909006C:PEM routines:get_name:no start line (Puma::MiniSSL::SSLError)

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

Previously if an invalid SSL cert or key (e.g. blank file, mismatching
private and public key, etc.) were specified, Puma would quietly
accept them, bind to the port, and mysteriously fail with an error
only after a client initiates a connection:

```
SSL error, peer: ::1, peer cert: : #<Puma::MiniSSL::SSLError: OpenSSL error: error:1417A0C1:SSL routines:tls_post_process_client_hello:no shared cipher - 193>
```

We now check the OpenSSL return values and raise an exception if the
cert or key cannot be loaded.
@MSP-Greg MSP-Greg changed the title Fail hard if OpenSSL cannot load cert or key Fail hard if SSL certs or keys are invalid Mar 27, 2022
@dentarg
Copy link
Member

dentarg commented Mar 27, 2022

Does it make sense to add a test to the test suite for this?

@MSP-Greg
Copy link
Member

@dentarg @stanhu

Maybe something like the following added to TestPumaServerSSL in test_puma_server_ssl.rb?

  unless Puma.jruby?
    def test_invalid_cert
      assert_raises(Puma::MiniSSL::SSLError) do
        start_server { |ctx| ctx.cert = __FILE__ }
      end
    end

    def test_invalid_key
      assert_raises(Puma::MiniSSL::SSLError) do
        start_server { |ctx| ctx.key = __FILE__ }
      end
    end

    def test_invalid_cert_pem
      assert_raises(Puma::MiniSSL::SSLError) do
        start_server { |ctx|
          ctx.instance_variable_set(:@cert, nil)
          ctx.cert_pem = 'Not a valid pem'
        }
      end
    end

    def test_invalid_key_pem
      assert_raises(Puma::MiniSSL::SSLError) do
        start_server { |ctx|
          ctx.instance_variable_set(:@key, nil)
          ctx.key_pem = 'Not a valid pem'
        }
      end
    end

    def test_invalid_ca
      assert_raises(Puma::MiniSSL::SSLError) do
        start_server { |ctx|
          ctx.ca = __FILE__
        }
      end
    end
  end

All five fail on master, pass with the PR. I can push a commit...

@stanhu - thanks for the PR, input testing is always good. After your previous PR, I was thinking the same thing (raise on invalid)...

@stanhu
Copy link
Contributor Author

stanhu commented Mar 28, 2022

Great idea on the test!

@nateberkopec nateberkopec merged commit db751ba into puma:master Apr 2, 2022
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Apr 3, 2022
* Fail hard if OpenSSL cannot load cert or key

Previously if an invalid SSL cert or key (e.g. blank file, mismatching
private and public key, etc.) were specified, Puma would quietly
accept them, bind to the port, and mysteriously fail with an error
only after a client initiates a connection:

```
SSL error, peer: ::1, peer cert: : #<Puma::MiniSSL::SSLError: OpenSSL error: error:1417A0C1:SSL routines:tls_post_process_client_hello:no shared cipher - 193>
```

We now check the OpenSSL return values and raise an exception if the
cert or key cannot be loaded.

* Add SSL invalid tests

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
@MSP-Greg MSP-Greg mentioned this pull request Apr 4, 2022
7 tasks
nateberkopec pushed a commit that referenced this pull request Aug 22, 2022
* Fail hard if OpenSSL cannot load cert or key

Previously if an invalid SSL cert or key (e.g. blank file, mismatching
private and public key, etc.) were specified, Puma would quietly
accept them, bind to the port, and mysteriously fail with an error
only after a client initiates a connection:

```
SSL error, peer: ::1, peer cert: : #<Puma::MiniSSL::SSLError: OpenSSL error: error:1417A0C1:SSL routines:tls_post_process_client_hello:no shared cipher - 193>
```

We now check the OpenSSL return values and raise an exception if the
cert or key cannot be loaded.

* Add SSL invalid tests

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
* Fail hard if OpenSSL cannot load cert or key

Previously if an invalid SSL cert or key (e.g. blank file, mismatching
private and public key, etc.) were specified, Puma would quietly
accept them, bind to the port, and mysteriously fail with an error
only after a client initiates a connection:

```
SSL error, peer: ::1, peer cert: : #<Puma::MiniSSL::SSLError: OpenSSL error: error:1417A0C1:SSL routines:tls_post_process_client_hello:no shared cipher - 193>
```

We now check the OpenSSL return values and raise an exception if the
cert or key cannot be loaded.

* Add SSL invalid tests

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants