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

Add ssl support to the control_cli #2052

Merged
merged 1 commit into from Nov 2, 2019

Conversation

composerinteralia
Copy link
Contributor

@composerinteralia composerinteralia commented Oct 24, 2019

If I have a server running:

bundle exec bin/puma test/rackup/hello.ru \
  --control-url="ssl://127.0.0.1:9000?key=examples/puma/puma_keypair.pem&cert=examples/puma/cert_puma.pem" \
  --control-token="token"

This commit will allow me to restart that server (or run any other
control cli command) with:

bundle exec bin/pumactl restart \
  --control-url="ssl://127.0.0.1:9000" \
  --control-token="token"

Before this commit the pumactl restart command would have raised an
error: "Invalid scheme: ssl"

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to HISTORY.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] to all commit messages.
  • I have added 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 commit messages.
  • 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.

@@ -141,6 +141,12 @@ def send_request

# create server object by scheme
server = case uri.scheme
when "ssl"
require 'openssl'
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'm not totally sure if this was the right thing to have done here. I started out with:

require 'puma/puma_http11'
require 'puma/minissl'
MiniSSL.check
MiniSSL::Socket.new(
  TCPSocket.new(uri.host, uri.port),
  MiniSSL::Engine.client
)

but that was raising an "Unknown OpenSSL error: 2" at

snprintf(msg, sizeof(msg), "Unknown OpenSSL error: %d", ssl_err);
and both my C and SSL knowledge are probably a bit too limited to know what to do with that without some help.

MiniSSL::Socket doesn't have a read method either, so there would still be some things to work out if that is the path we should go down.


assert_kind_of Thread, t.join, "server didn't stop"
def assert_app_running(host)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a useful abstraction, because you're not actually asserting that "an app" is "running,", you're asserting that a puma server running the "embedded app" config file returns a 200 when connected to.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't wait_booted perform a check something like this, as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. This code didn't seem important to what either test was actually testing, so I tried to hide it away in a helper method. Digging further, I see now that wait_booted was waiting for the embedded app to boot, but not for the control app. Connecting to the "embedded app" took long enough that the control app was booted by the time we needed it. sleep 0.1 would also have worked.

I updatedwait_booted (used only in these two tests) to wait for the control app instead, and then got rid of the "embedded app" assertion for the new spec. I kept the extra assertions it in the existing spec in case they are testing something that isn't covered elsewhere.

If I have a server running:

```sh
bundle exec bin/puma test/rackup/hello.ru \
  --control-url="ssl://127.0.0.1:9000?key=examples/puma/puma_keypair.pem&cert=examples/puma/cert_puma.pem" \
  --control-token="token"
```

This commit will allow me to restart that server (or run any other
control cli command) with:

```sh
bundle exec bin/pumactl restart \
  --control-url="ssl://127.0.0.1:9000" \
  --control-token="token"
```

Before this commit the pumactl restart command would have raised an
error: `"Invalid scheme: ssl"`
@nateberkopec nateberkopec merged commit 7256d79 into puma:master Nov 2, 2019
@composerinteralia composerinteralia deleted the control-cli-ssl branch November 2, 2019 20:58
This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants