Output a warning from the launch script if the application will run as root #10275

obfischer · 2017-09-12T20:33:34Z

The user used in the launch script to run the application is now configurable if needed. Closes #10273.

kartoffelsup · 2017-09-12T21:17:55Z

...ing-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script

+if [[ -z "{{runUser}}" ]]; then
+    # Determine the user to run as if we are root
+    # shellcheck disable=SC2012
+    [[ $(id -u) == "0" ]] && run_user=$(ls -ld "$jarfile" | awk '{print $3}')


What if root is the owner of the jar file? Might be good to print a warning if someone configures the run as user to root?

I thought the same but I didn't want to address this in this PR. As a running an program as root is a potential security problem. We could issues an warning or abort the script. But this would be breaking change for existing users who updated.

I see to possible solutions:

Print a simple warning while starting the program.

Refuse to start the program if it would run as root unless the user set runUser=root explicitly.

WDYT?

I think a warning is the way to go (as you stated it might break something otherwise). I think the Apache Webserver does the same (probably a couple of others as well)

Ok, I will update the PR but tomorrow. It is already later for me... Ok?

I'm not in the position to (and I never would) demand anything from you :) plus we're in the same time zone ;)

obfischer · 2017-09-13T21:26:28Z

...ing-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script

+fi
+
+# Issue an warning if the application will run as root
+[[ $(id -u) == "0" ]] && { echoRed "Application is running as root (UID 0)."; }


Now also a warning will be printed if the effective user at runtime is root.

wilkinsona

Thanks for the PR, particularly as it's your first contribution to Spring Boot. This looks like a good addition to make. I've left a few, mostly minor, comments. Would you mind taking a look and, if you agree, updating the PR?

wilkinsona · 2017-09-14T05:52:52Z

spring-boot-docs/src/main/asciidoc/deployment.adoc

@@ -780,6 +780,12 @@ for Gradle and to `${project.name}` for Maven.
 |The default value for the name of the pid file in `PID_FOLDER`. Only valid for an
 `init.d` service.

+|`runUser`
+| The user under which the application will run under at runtime. If not set


No need for the second "under" in this sentence

wilkinsona · 2017-09-14T05:53:15Z

spring-boot-docs/src/main/asciidoc/deployment.adoc

@@ -780,6 +780,12 @@ for Gradle and to `${project.name}` for Maven.
 |The default value for the name of the pid file in `PID_FOLDER`. Only valid for an
 `init.d` service.

+|`runUser`
+| The user under which the application will run under at runtime. If not set
+  the application will run under the id of user who runs the script. If


Add "the" so it read "the id of the user"?

wilkinsona · 2017-09-14T05:54:26Z

...ing-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script

+fi
+
+# Issue an warning if the application will run as root
+[[ $(id -u) == "0" ]] && { echoRed "Application is running as root (UID 0)."; }


I think this should be echoYellow rather than echoRed. The latter is only used in the script when there's an error and the script will exit with a non-zero exit code.

And maybe also re-phrase to something like
"[WARN] Application is running as root (UID 0). This is considered insecure."
to emphasize that it is a warning and not just a yellow colored information :)
Also, shouldn't this be:

[[ $(id -u $run_user) == "0" ]] ...

?

obfischer · 2017-09-14T20:14:45Z

I applied all suggested changes.

obfischer · 2017-09-18T17:41:38Z

Is there anything that needs still to be done?

wilkinsona · 2017-09-18T17:53:29Z

No, I don't think so. Thanks again for the PR.

kartoffelsup · 2017-09-18T18:03:06Z

...ing-boot-loader-tools/src/main/resources/org/springframework/boot/loader/tools/launch.script

+fi
+
+# Issue an warning if the application will run as root
+[[ $(id -u) == "0" ]] && { echoYellow "[WARN] Application is running as root (UID 0). This is considered insecure."; }


Shouldn't this be $(id -u $run_user)?

Well spotted, @kartoffelsup. I think you're right. Thank you.

That raises the issue of adding some tests for this which I overlook when I said that I didn't think that there was anything left to be done.

@obfischer do you have the time to look at adding some tests please? I'm happy to guide you through the process as needed.

Yes, of course. Which tests are missing from your perspective?

A couple of tests would be good:

Verify that the warning is generated when the application will run as root

Verify that the warning is not generated when the application will run as another user

I think the correct module for such tests will be spring-boot-launch-script-tests?

Yes, that's right. Thanks.

I will start now with it.

wilkinsona · 2017-10-24T12:32:00Z

@obfischer Any luck with writing those tests?

obfischer · 2017-10-24T13:01:43Z

Not until now. I have a hugh workload until the end of the month.

wilkinsona · 2017-10-24T13:04:39Z

Understood. Thanks. There's no major rush from our perspective.

obfischer · 2017-11-17T19:35:29Z

Hi @wilkinsona, I am trying to run the integration tests via mvn -P docker clean install but all tests are failing. The readme states that native Docker on macOS is not support and I need the Docker Toolbox. As far as I know the Docker Toolbox is not available anymore.

Any idea?

wilkinsona · 2017-11-18T12:35:45Z

It has been quite some time since I set Docker up on my Mac. Does the Docker Toolbox installation available here not work?

obfischer · 2017-11-19T21:16:43Z

To stupid to Google. I will check it out.

obfischer · 2017-12-03T13:06:33Z

The testsetup is now running on my machine.

obfischer · 2017-12-09T16:29:13Z

@wilkinsona I updated the existing tests. Currently there is still one missing test I hope to write it during the next days.

wilkinsona · 2017-12-11T11:55:56Z

@obfischer Thanks for the updates

obfischer · 2017-12-11T12:25:58Z

Hi @wilkinsona I added all positive and negative tests and eleminated found bugs. Could you please review them if you have time.

obfischer · 2017-12-21T22:19:21Z

Hi @wilkinsona is there a chance that this PR will be accepted for the 1.5 branch? And is it planned to have a new 1.5.x release in the next time? Please let me know if something could be done by me.

wilkinsona · 2017-12-21T22:50:25Z

No, sorry. Given that this is an enhancement, it will have to go into 2.0 at the earliest. I’ll target it at 2.0.0.RC1 and see what we can do.

wilkinsona · 2018-02-02T16:03:11Z

Thanks.

Sorry, but I don't find that argument sufficiently compelling to merge this. The proposal leaves users with multiple ways of doing the same thing and that's something that we try to avoid. We already document some advice on securing a jar so that it cannot be manipulated or replaced.

If you prefer your approach to the documented approach, I'd recommend that you consider building your jar with a custom script.

obfischer · 2018-02-02T16:08:49Z

Ok.

kartoffelsup · 2018-02-02T17:13:17Z

@wilkinsona will the warning be added?

obfischer · 2018-02-03T11:40:50Z

Would be nice to have a least a warning.

wilkinsona · 2018-02-05T11:36:21Z

@obfischer Are you happy for us to extract just that part of the proposed changes from this PR?

obfischer · 2018-02-05T12:02:51Z

Yes, I will do that. Update this PR or should I open a new one?

wilkinsona · 2018-02-05T12:04:34Z

Thanks. Let's use this one. I've reopened it.

wilkinsona · 2018-05-14T16:36:08Z

@obfischer Are you still interested in slimming this PR down to the piece that logs the warning?

obfischer · 2018-05-15T22:13:52Z

Hi @wilkinsona, yes. Give me a last chance until next Monday. It is a official holiday here in Germany.

…ning as root.

obfischer · 2018-08-10T23:27:03Z

Hi @wilkinsona I changed my PR as you requested.

wilkinsona · 2018-08-13T06:46:27Z

Thanks very much.

wilkinsona · 2018-09-11T11:20:57Z

@obfischer Thank you very much for making your first contribution to Spring Boot. The proposed changes have now been merged into master.

See gh-10275

* gh-10275: Polish "Issue a warning from launch script when app will run as root" Issue a warning from launch script when app will run as root

obfischer · 2018-09-16T11:41:48Z

Thank you for your support!

snicoll mentioned this pull request Sep 12, 2017

Make run_user for the launch script configurable #10273

Closed

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 12, 2017

kartoffelsup reviewed Sep 12, 2017

View reviewed changes

obfischer commented Sep 13, 2017

View reviewed changes

wilkinsona requested changes Sep 14, 2017

View reviewed changes

wilkinsona added priority: normal type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 18, 2017

wilkinsona added this to the 2.0.0.M5 milestone Sep 18, 2017

kartoffelsup reviewed Sep 18, 2017

View reviewed changes

wilkinsona removed this from the 2.0.0.M5 milestone Sep 18, 2017

philwebb force-pushed the master branch from dc8c2d0 to 33edfda Compare October 5, 2017 03:19

wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Oct 24, 2017

wilkinsona removed the status: waiting-for-feedback We need additional information before we can continue label Dec 21, 2017

wilkinsona added this to the 2.0.0.RC1 milestone Dec 21, 2017

wilkinsona reopened this Feb 5, 2018

wilkinsona added type: enhancement A general enhancement status: waiting-for-feedback We need additional information before we can continue priority: normal and removed status: declined A suggestion or change that we don't feel we should currently apply labels Feb 5, 2018

philwebb removed the priority: normal label Mar 21, 2018

philwebb force-pushed the master branch from 86d82f9 to 7b120c1 Compare May 4, 2018 23:38

[#10273] Launch script issues now a warning if the application is run…

9d0780c

…ning as root.

wilkinsona changed the title ~~The user used in the launch script to run the application is now configurable.~~ Output a warning from the launch script if the application will run as root Aug 13, 2018

wilkinsona removed the status: waiting-for-feedback We need additional information before we can continue label Aug 13, 2018

wilkinsona added this to the Backlog milestone Aug 13, 2018

wilkinsona approved these changes Aug 13, 2018

View reviewed changes

wilkinsona pushed a commit that referenced this pull request Sep 11, 2018

Issue a warning from launch script when app will run as root

94ebd33

See gh-10275

wilkinsona closed this in 1332fcb Sep 11, 2018

wilkinsona added a commit that referenced this pull request Sep 11, 2018

Merge pull request #10275 from Oliver B. Fischer

0fe6010

* gh-10275: Polish "Issue a warning from launch script when app will run as root" Issue a warning from launch script when app will run as root

wilkinsona modified the milestones: 2.1.x, 2.1.0.M3 Sep 11, 2018

Output a warning from the launch script if the application will run as root #10275

Output a warning from the launch script if the application will run as root #10275

Conversation

obfischer commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilkinsona left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartoffelsup Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

obfischer commented Sep 14, 2017

obfischer commented Sep 18, 2017

wilkinsona commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obfischer Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilkinsona commented Oct 24, 2017

obfischer commented Oct 24, 2017

wilkinsona commented Oct 24, 2017

obfischer commented Nov 17, 2017

wilkinsona commented Nov 18, 2017

obfischer commented Nov 19, 2017

obfischer commented Dec 3, 2017

obfischer commented Dec 9, 2017

wilkinsona commented Dec 11, 2017

obfischer commented Dec 11, 2017

obfischer commented Dec 21, 2017

wilkinsona commented Dec 21, 2017

wilkinsona commented Feb 2, 2018

obfischer commented Feb 2, 2018

kartoffelsup commented Feb 2, 2018

obfischer commented Feb 3, 2018

wilkinsona commented Feb 5, 2018

obfischer commented Feb 5, 2018

wilkinsona commented Feb 5, 2018

wilkinsona commented May 14, 2018

obfischer commented May 15, 2018

obfischer commented Aug 10, 2018

wilkinsona commented Aug 13, 2018

wilkinsona commented Sep 11, 2018

obfischer commented Sep 16, 2018

wilkinsona left a comment •

edited

Loading

kartoffelsup Sep 14, 2017 •

edited

Loading

obfischer Sep 18, 2017 •

edited

Loading