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

8245095: Implementation of JEP 408: Simple Web Server #5505

Closed
wants to merge 24 commits into from

Conversation

FrauBoes
Copy link
Member

@FrauBoes FrauBoes commented Sep 14, 2021

This change implements a simple web server that can be run on the command-line with java -m jdk.httpserver.

This is facilitated by adding an entry point for the jdk.httpserver module, an implementation class whose main method is run when the above command is executed. This is the first such module entry point in the JDK.

The server is a minimal HTTP server that serves the static files of a given directory, similar to existing alternatives on other platforms and convenient for testing, development, and debugging.

Additionally, a small API is introduced for programmatic creation and customization.

Testing: tier1-3.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8245095: Implementation of JEP 408: Simple Web Server

Reviewers

Contributors

  • Julia Boes <jboes@openjdk.org>
  • Chris Hegarty <chegar@openjdk.org>
  • Michael McMahon <michaelm@openjdk.org>
  • Daniel Fuchs <dfuchs@openjdk.org>
  • Jan Lahoda <jlahoda@openjdk.org>
  • Ivan Šipka <isipka@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5505/head:pull/5505
$ git checkout pull/5505

Update a local copy of the PR:
$ git checkout pull/5505
$ git pull https://git.openjdk.java.net/jdk pull/5505/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5505

View PR using the GUI difftool:
$ git pr show -t 5505

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5505.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 14, 2021

👋 Welcome back jboes! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2021

@FrauBoes The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build core-libs net labels Sep 14, 2021
@FrauBoes FrauBoes marked this pull request as ready for review Sep 14, 2021
@openjdk openjdk bot added the rfr label Sep 14, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 14, 2021

Copy link
Member

@magicus magicus left a comment

Build changes look good.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2021

@FrauBoes This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8245095: Implementation of JEP 408: Simple Web Server

Co-authored-by: Julia Boes <jboes@openjdk.org>
Co-authored-by: Chris Hegarty <chegar@openjdk.org>
Co-authored-by: Michael McMahon <michaelm@openjdk.org>
Co-authored-by: Daniel Fuchs <dfuchs@openjdk.org>
Co-authored-by: Jan Lahoda <jlahoda@openjdk.org>
Co-authored-by: Ivan Šipka <isipka@openjdk.org>
Reviewed-by: ihse, jlaskey, michaelm, chegar, dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 201 new commits pushed to the master branch:

  • d548f2f: 8274346: Support for additional content in an app-image.
  • a619f89: 8274721: UnixSystem fails to provide uid, gid or groups if no username is available
  • 1afddb2: 8275322: Change nested classes in java.management to static nested classes
  • 45ebf85: 8275333: Print count in "Too many recored phases?" assert
  • ebb1363: 8251513: Code around Parse::do_lookupswitch/do_tableswitch should be cleaned up
  • bb7dacd: 8275334: Move class loading Events to a separate section in hs_err files
  • 3150069: 8271949: dumppath in -XX:FlightRecorderOptions does not affect
  • bfcf6a2: 8275277: assert(dest_attr.is_in_cset() == (obj->forwardee() == obj)) failed: Only evac-failed objects must be in the collection set here but is not
  • 96fef40: 8189591: No way to locally suppress doclint warnings
  • 7fc3a8d: 8275149: (ch) ReadableByteChannel returned by Channels.newChannel(InputStream) throws ReadOnlyBufferException
  • ... and 191 more: https://git.openjdk.java.net/jdk/compare/b1b66965f1ec6eae547cc4f70f8271bd39ded6da...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 14, 2021
Copy link
Member

@JimLaskey JimLaskey left a comment

Very nice. LGTM.

@dfuch
Copy link
Member

@dfuch dfuch commented Sep 15, 2021

/csr needed

@openjdk openjdk bot added the csr label Sep 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

@dfuch has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@FrauBoes please create a CSR request for issue JDK-8245095. This pull request cannot be integrated until the CSR request is approved.

@openjdk openjdk bot removed the ready label Sep 15, 2021
* Usage: java -m jdk.httpserver [-b bind address] [-p port] [-d directory]
* [-o none|info|verbose] [-h to show options]
* Options:
* -b, --bind-address - Address to bind to. Default: 0.0.0.0 (all interfaces).
Copy link
Member

@jaikiran jaikiran Sep 16, 2021

Choose a reason for hiding this comment

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

I understand that the purpose of this simple server is for development and testing only. But even then, for security considerations, would it be more appropriate to default the bind address to a loopback address instead of making it accessible potentially to entire public? In the past, application servers which used to bind to all interfaces by default have now moved to using the loopback address as a default to avoid such accidental exposing of the server.

Copy link
Member Author

@FrauBoes FrauBoes Sep 17, 2021

Choose a reason for hiding this comment

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

We did consider defaulting to the loopback address, but this would limit the usefulness of the server too much in the default configuration as it can only be accessed from localhost. The goal of this JEP is an out-of-the-box web server with easy setup, so in this case we favour usability. The purpose of a web server is to make things accessible on the web so it is assumed that the developer is familiar with the terms this comes with.

The concern of accidental exposure is alleviated by the informative output printed at start up, e.g.

Serving /current/directory and subdirectories on 0.0.0.0:8000
http://123.456.7.891:8000/ ...

Considering your point though, we can spell out all interfaces and describe the URL more clearly:

Serving /current/directory and subdirectories on 0.0.0.0 (all interfaces) port 8000
Localhost URL: http://123.456.7.891:8000/ ...

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 16, 2021

Mailing list message from Bernd Eckenfels on core-libs-dev:

I also wonder if it makes sense to either only serve files with public permissions, or at least Filter some critical files like .ssh/* and *.jks. Those command-line servers are often started ?accidentially? in the home directory.

--
http://bernd.eckenfels.net
________________________________
Von: net-dev <net-dev-retn at openjdk.java.net> im Auftrag von Jaikiran Pai <jpai at openjdk.java.net>
Gesendet: Thursday, September 16, 2021 4:08:46 PM
An: build-dev at openjdk.java.net <build-dev at openjdk.java.net>; core-libs-dev at openjdk.java.net <core-libs-dev at openjdk.java.net>; net-dev at openjdk.java.net <net-dev at openjdk.java.net>
Betreff: Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v3]

On Thu, 16 Sep 2021 10:14:47 GMT, Julia Boes <jboes at openjdk.org> wrote:

This change implements a simple web server that can be run on the command-line with `java -m jdk.httpserver`.

This is facilitated by adding an entry point for the `jdk.httpserver` module, an implementation class whose main method is run when the above command is executed. This is the first such module entry point in the JDK.

The server is a minimal HTTP server that serves the static files of a given directory, similar to existing alternatives on other platforms and convenient for testing, development, and debugging.

Additionally, a small API is introduced for programmatic creation and customization.

Testing: tier1-3.

Julia Boes has updated the pull request incrementally with one additional commit since the last revision:

correct path handling

src/jdk.httpserver/share/classes/module-info.java line 55:

53: * [-o none|info|verbose] [-h to show options]
54: * Options:
55: * -b, --bind-address - Address to bind to. Default: 0.0.0.0 (all interfaces).

I understand that the purpose of this simple server is for development and testing only. But even then, for security considerations, would it be more appropriate to default the bind address to a loopback address instead of making it accessible potentially to entire public? In the past, application servers which used to bind to all interfaces by default have now moved to using the loopback address as a default to avoid such accidental exposing of the server.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5505

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 17, 2021

Mailing list message from Jaikiran Pai on build-dev:

Hello Julia,

On 17/09/21 3:14 pm, Julia Boes wrote:

On Thu, 16 Sep 2021 14:05:52 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

Julia Boes has updated the pull request incrementally with one additional commit since the last revision:

correct path handling
src/jdk.httpserver/share/classes/module-info.java line 55:

53: * [-o none|info|verbose] [-h to show options]
54: * Options:
55: * -b, --bind-address - Address to bind to. Default: 0.0.0.0 (all interfaces).
I understand that the purpose of this simple server is for development and testing only. But even then, for security considerations, would it be more appropriate to default the bind address to a loopback address instead of making it accessible potentially to entire public? In the past, application servers which used to bind to all interfaces by default have now moved to using the loopback address as a default to avoid such accidental exposing of the server.
We did consider defaulting to the loopback address, but this would limit the usefulness of the server too much in the default configuration as it can only be accessed from localhost. The goal of this JEP is an out-of-the-box web server with easy setup, so in this case we favour usability. The purpose of a web server is to make things accessible on the web so it is assumed that the developer is familiar with the terms this comes with.

The concern of accidental exposure is alleviated by the informative output printed at start up, e.g.
```~ $ java-sb -m jdk.httpserver
Serving /current/directory and subdirectories on 0.0.0.0:8000
http://123.456.7.891:8000/ ...

I think this is still a really big risk. I say this based on some of my
past experience with application servers (JBoss AS) where in older
releases it used to do this same thing of binding to 0.0.0.0 by default
and how that had lead to numerous (production) instances ending up being
vulnerable. In the case there, the management console ended up being
exposed and almost anyone over the internet could just access it to
shutdown the server (through a JMX MBean).

In the case of this simple server being proposed, I think it's a lot
more riskier because unlike in the case of the application servers where
the server would have preventive measures that wouldn't allow local
filesystem access, the current server being proposed will end up
exposing the user's local filesystem to the internet. It's my opinion
and experience that log messages no matter how much they scream out,
won't prevent this default out of the box usage.

I'm not saying 0.0.0.0 should be disabled, but instead, IMO it should be
the user who should explicitly use -b 0.0.0.0 to do that, so that they
are at least responsible and aware of what they are doing.

-Jaikiran

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 17, 2021

Mailing list message from Jaikiran Pai on core-libs-dev:

Hello Bernd,

On 17/09/21 3:37 am, Bernd Eckenfels wrote:

I also wonder if it makes sense to either only serve files with public permissions, or at least Filter some critical files like .ssh/* and *.jks.

FWIW - From what I can see in the proposed implementation as well as the
JEP text, hidden files and symbolic links aren't served. So it should
prevent listing/serving .ssh/ directory.

-Jaikiran

@jaikiran
Copy link
Member

@jaikiran jaikiran commented Sep 23, 2021

Thanks for sharing your experience on this, it's appreciated. 0.0.0.0 is common default for Apache httpd [1], Ngnix [2], the Python web server [3]. This being said, I want to make sure we're taking the right decision here so let me seek some further advice on this.
[1] http://httpd.apache.org/docs/2.4/bind.html
[2] https://docs.nginx.com/nginx/admin-guide/web-server/web-server/
[3] https://github.com/python/cpython/blob/3.4/Lib/http/server.py

Further review concluded that a default binding to 0.0.0.0 creates too a high level of exposure, particularly for a low-threshold utility like this server. Binding to an unrestricted address is a known way for attackers to launch a Denial-of-Service attack, classified by MITRE as CWE-1327 [1]. We therefore update the default binding to the loopback address and amend the help output with information on how to bind to 0.0.0.0

Thank you Julia for considering this input and coordinating the change.

@FrauBoes
Copy link
Member Author

@FrauBoes FrauBoes commented Sep 28, 2021

7f99447 updates the output to:

  • use the ipv4/ipv6 specific loopback address,
  • include a line on how to bind to all interfaces if no bind address is specified,
  • include the option help text in the case where an argument is missing.

dfuch
dfuch approved these changes Sep 28, 2021
Copy link
Member

@dfuch dfuch left a comment

Thanks Julia, the new changes look good to me.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 28, 2021

Mailing list message from Bernd Eckenfels on core-libs-dev:

Just a nit, but how about:

* For all interfaces use ?-b ???0.0.0.0? (IPv4) or ?-b ::? (IPv6)

Instead of:

* For 0.0.0.0 (all interfaces) use -b 0.0.0.0 or -b ::0

In the usage?

(I think ?::? is canon?)

Gruss
Bernd

--
http://bernd.eckenfels.net
________________________________
Von: net-dev <net-dev-retn at openjdk.java.net> im Auftrag von Daniel Fuchs <dfuchs at openjdk.java.net>
Gesendet: Tuesday, September 28, 2021 12:20:11 PM
An: build-dev at openjdk.java.net <build-dev at openjdk.java.net>; core-libs-dev at openjdk.java.net <core-libs-dev at openjdk.java.net>; net-dev at openjdk.java.net <net-dev at openjdk.java.net>
Betreff: Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v7]

On Tue, 28 Sep 2021 10:08:29 GMT, Julia Boes <jboes at openjdk.org> wrote:

This change implements a simple web server that can be run on the command-line with `java -m jdk.httpserver`.

This is facilitated by adding an entry point for the `jdk.httpserver` module, an implementation class whose main method is run when the above command is executed. This is the first such module entry point in the JDK.

The server is a minimal HTTP server that serves the static files of a given directory, similar to existing alternatives on other platforms and convenient for testing, development, and debugging.

Additionally, a small API is introduced for programmatic creation and customization.

Testing: tier1-3.

Julia Boes has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits:

- use ipv4/ipv6 specific loopback address and add add how-to output for all interfaces
- Merge branch 'master' into simpleserver
- change default bind address from anylocal to loopback
- address PR comments
- Merge branch 'master' into simpleserver
- Merge remote-tracking branch 'origin/simpleserver' into simpleserver
- Merge branch 'master' into simpleserver
- refactor isHidden,isReadable,isSymlink checks and cleanup tests
- Merge branch 'master' into simpleserver
- check isHidden, isSymlink, isReadable for all path segments
- ... and 10 more: https://git.openjdk.java.net/jdk/compare/87998565...7f99447

Thanks Julia, the new changes look good to me.

-------------

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5505

@FrauBoes
Copy link
Member Author

@FrauBoes FrauBoes commented Sep 29, 2021

* For all interfaces use "-b 0.0.0.0" (IPv4) or "-b ::" (IPv6)

Instead of:

* For 0.0.0.0 (all interfaces) use -b 0.0.0.0 or -b ::0

In the usage?

(I think ?::? is canon?)

Good point, "::" is recommended, e.g. in RFC5952 [1].
I think IPv4/IPv6 is obvious though, so I suggest:

* For all interfaces use "-b 0.0.0.0" or "-b ::"

[1] https://datatracker.ietf.org/doc/html/rfc5952

dfuch
dfuch approved these changes Sep 29, 2021
@openjdk openjdk bot added ready and removed csr labels Oct 18, 2021
@FrauBoes
Copy link
Member Author

@FrauBoes FrauBoes commented Oct 18, 2021

/contributor add jboes
/contributor add chegar
/contributor add michaelm
/contributor add dfuchs
/contributor add jlahoda
/contributor add isipka

@openjdk
Copy link

@openjdk openjdk bot commented Oct 18, 2021

@FrauBoes
Contributor Julia Boes <jboes@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 18, 2021

@FrauBoes
Contributor Chris Hegarty <chegar@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 18, 2021

@FrauBoes
Contributor Michael McMahon <michaelm@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 18, 2021

@FrauBoes
Contributor Daniel Fuchs <dfuchs@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 18, 2021

@FrauBoes
Contributor Jan Lahoda <jlahoda@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 18, 2021

@FrauBoes
Contributor Ivan Šipka <isipka@openjdk.org> successfully added.

@FrauBoes
Copy link
Member Author

@FrauBoes FrauBoes commented Oct 19, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2021

Going to push as commit 9d191fc.
Since your change was applied there have been 209 commits pushed to the master branch:

  • 947d52c: 8255898: Test java/awt/FileDialog/FilenameFilterTest/FilenameFilterTest.java fails on Mac OS
  • a03119c: 8275436: [BACKOUT] JDK-8271949 dumppath in -XX:FlightRecorderOptions does not affect
  • bad75e6: 8275150: URLClassLoaderTable should store OopHandle instead of Handle
  • 72a976e: 8266936: Add a finalization JFR event
  • bcbe384: 8269175: [macosx-aarch64] wrong CPU speed in hs_err file
  • 426bcee: 8275360: Use @OverRide in javax.annotation.processing
  • 4d383b9: 8275298: Remove unnecessary weak_oops_do call in adjust weak roots phase
  • fb8e5cf: 8275368: Correct statement of kinds of elements Processor.process operates over
  • d548f2f: 8274346: Support for additional content in an app-image.
  • a619f89: 8274721: UnixSystem fails to provide uid, gid or groups if no username is available
  • ... and 199 more: https://git.openjdk.java.net/jdk/compare/b1b66965f1ec6eae547cc4f70f8271bd39ded6da...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 19, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2021

@FrauBoes Pushed as commit 9d191fc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build core-libs integrated net