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 HTTP::Server#bind(URI) #6500

Merged
merged 4 commits into from Aug 8, 2018

Conversation

Projects
None yet
8 participants
@straight-shoota
Contributor

straight-shoota commented Aug 7, 2018

This PR was originally included in #5776 but it had been removed as a separate feature. Now that #5959 and #5960 are merged, it is ready to be reviewed. The original description can be found in #2735 (comment)

The URI can be parsed from a string and contains the protocol and address to bind to (plus optional additional configuration for SSL context). This allows for a uniform and easily exchangeable specification in config files.

server.bind "tcp://127.0.0.1:80"
server.bind "unix:///tmp/server.sock"
server.bind "ssl://127.0.0.1:443?key=private.key&cert=certificate.cert&ca=ca.crt"

Along the way this also adds Socket::{IP,UNIX,}Address.parse and OpenSSL::SSL::Context::{Client,Server}.parse to implement the individual component parsers.

# It expects the URI to include `<scheme>://<path>` where `scheme` as well
# as any additional URI components (such as `fragment` or `query`) are ignored.
#
# If `host` is not empty, it will be prepended to `path` to form a relatve

This comment has been minimized.

@r00ster91

r00ster91 Aug 7, 2018

Contributor

Typo: "relatve"

# Parses a `Socket::IPAddress` from an URI.
#
# It expects the URI to include `<scheme>://<host>:<port>` where `scheme` as
# well asa ny additional URI components (such as `path` or `query`) are ignored.

This comment has been minimized.

@r00ster91

r00ster91 Aug 7, 2018

Contributor

Typo: "asa ny"

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-parse-address branch from 8ce0453 to 9e23371 Aug 7, 2018

address = Socket::IPAddress.parse(uri)
context = OpenSSL::SSL::Context::Server.parse(HTTP::Params.parse(uri.query || ""))
bind_ssl(address, context)

This comment has been minimized.

@bcardiff

bcardiff Aug 7, 2018

Member

Checking for flag?(:without_openssl) is missing here. At least with a duplication of the Unsupported socket type message or something more specific.

#
# context = OpenSSL::SSL::Context::Client.parse({"key" => "private.key", "cert" => "certificate.crt", "ca" => "ca.pem"})
# ```
def self.parse(params) : self

This comment has been minimized.

@bcardiff

bcardiff Aug 7, 2018

Member

I would expect T.parse to be from String -> T. I would suggest T.from_hash for this method.

bind(URI.parse(uri))
end
# ditto

This comment has been minimized.

@bcardiff

bcardiff Aug 7, 2018

Member

Warn: mixed ditto and :ditto: in the PR

# Socket::IPAddress.parse("tcp://127.0.0.1:8080") # => Socket::IPAddress.new("127.0.0.1", 8080)
# Socket::IPAddress.parse("udp://[::1]:8080") # => Socket::IPAddress.new("::1", 8080)
# ```
def self.parse(uri : URI) : IPAddress

This comment has been minimized.

@bcardiff

bcardiff Aug 7, 2018

Member

Somehow I'm ok with T.parse: URI -> T 😹 , but not with Hash.

This comment has been minimized.

@straight-shoota

straight-shoota Aug 7, 2018

Contributor

URI is essentially a structured string...

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-parse-address branch from 9e23371 to d517ed6 Aug 7, 2018

#
# Params:
#
# * `key` *(required*): Path to private key file. See `#private_key=`.

This comment has been minimized.

@Sija

Sija Aug 7, 2018

Contributor

*(required*) -> (*required*)
ditto below

straight-shoota added some commits Aug 7, 2018

Add HTTP::Server#bind(URI)
The URI can be parsed from a string and contains the protocol and address to bind to (plus optional additional configuration for SSL context). This allows for a uniform and easily exchangable specificitation in config files.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-parse-address branch from d517ed6 to 0620467 Aug 7, 2018

@bcardiff bcardiff referenced this pull request Aug 7, 2018

Closed

Update to crystal 0.26.0 #479

@sdogruyol

Thank you @straight-shoota 👍

@sdogruyol sdogruyol merged commit 928a738 into crystal-lang:master Aug 8, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sdogruyol sdogruyol added this to the 0.26.0 milestone Aug 8, 2018

@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/http-server-parse-address branch Aug 8, 2018

@bcardiff bcardiff referenced this pull request Aug 8, 2018

Closed

Update to crystal 0.26 #922

@vladfaust vladfaust referenced this pull request Aug 9, 2018

Closed

Update to new Crystal #31

1 of 3 tasks complete
@bcardiff

This comment has been minimized.

Member

bcardiff commented Aug 10, 2018

@straight-shoota

The sample in the PR description mention ssh:// as a protocol, but it should be ssl:// probably.
The port used is not the default 443, although any port is supported using defaults seems better for lazy copy/pasting.

server.bind "ssl://127.0.0.1:443?key=private.key&cert=certificate.cert&ca=ca.crt

Or am I missing something?

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 10, 2018

You're absolute right.

@jhass

This comment has been minimized.

Member

jhass commented Aug 10, 2018

Bold proposal: Lets ditch ssl:// and only support tls://

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 10, 2018

All the classes in Crystal are called SSL... so I'd at least keep support for ssl://. But adding tls:// and using it as default (in examples) sounds good.

@jhass

This comment has been minimized.

Member

jhass commented Aug 10, 2018

No, they're called OpenSSL, because they follow the name of the library they wrap. On the other hand we already renamed Http's ssl flags to tls at some point.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 10, 2018

No, I mean inside the OpenSSL namespace (which is obviously named after the library), there is OpenSSL::SSL.

But I'd suggest to discuss this further in a new dedicated issue...

@jhass

This comment has been minimized.

Member

jhass commented Aug 10, 2018

I wouldn't be too opposed to renaming that to TLS too :P

@RX14

This comment has been minimized.

Member

RX14 commented Aug 11, 2018

OpenSSL namespace will eventually go and be replaced with a TLS namespace with a much better API - which can be supported by libraries which aren't openssl. So +1 to supporting tls but supporting ssl too is fine.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 12, 2018

I'll work on that. Waiting for #6530 to be merged.

@crisward

This comment has been minimized.

Contributor

crisward commented Aug 14, 2018

I realise this has already been merged, but FWIW I would have expected to be able to bind to do server.bind "http://... and server.bind "https://... (as aliases for tcp and ssl). Apologies if this has already been discussed somewhere else. Just saw this in the 0.26 release post. Congrats BTW on the new release!

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 14, 2018

No, this wasn't discussed before explicitly. And I admit that this might look a bit surprising at first.

The reason is that HTTP and HTTPS are application protocols - and HTTP::Server naturally speaks HTTP(S) on all connections. server.bind "http://[...]" would only tell to use the HTTP protocol (which is obvious since this is an HTTP server).

The URIs used by bind are meant to specify the transport protocol (TCP, TCP+TLS or UNIX socket).
This is probably most clear when looking at tls://127.0.0.1:443?key=private.key&cert=certificate.cert&ca=ca.crt. This URI configures a TCP+TLS server and is not an HTTP resource.

The same transport protocol schemes are also used by similar applications such as Puma or Nginx.

@crisward

This comment has been minimized.

Contributor

crisward commented Aug 14, 2018

Wasn't aware of other servers using these protocols, makes sense. Thanks for the clarification.

omarroth added a commit to omarroth/crystal that referenced this pull request Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment