Tighten up write permissions in Docker image #70635

pugnascotia · 2021-03-22T13:22:44Z

Recursively remove write access from the bin, jdk, lib and
modules directories, since this access is not required, and removing
it makes it harder to exploit other issues in an ES distribution.

Recursively remove write access from the `bin`, `jdk`, `lib` and `modules` directories, since this access is not required, and removing it makes it harder to exploit other issues in an ES distribution.

elasticmachine · 2021-03-22T13:22:48Z

Pinging @elastic/es-delivery (Team:Delivery)

droberts195

LGTM assuming it doesn't cause any failures.

If it turns out something legitimately needed write access to one of these files or directories then we should look to make the minimum necessary subset of files or directories writable again.

rjernst · 2021-03-22T20:32:34Z

Instead of giving the entire ES install permissions and then restricting a fixed set from write permissions, can we instead explicitly give write permissions to the dirs that actually need it? Otherwise any additions (or even renames) of these directories in the future would need corresponding changes here which would be easy to miss.

mark-vieira · 2021-03-22T23:03:36Z

Instead of giving the entire ES install permissions and then restricting a fixed set from write permissions, can we instead explicitly give write permissions to the dirs that actually need it?

This seems reasonable. How difficult would this be?

pugnascotia · 2021-03-23T13:40:40Z

I pushed a change so that all permissions are explicitly set under /usr/share/elasticsearch, is that what you had in mind?

rjernst · 2021-03-23T15:43:48Z

I was thinking more about keeping the original permissions, and just changing the owner so it is root like rpm/deb have.

pugnascotia · 2021-03-24T15:17:35Z

@rjernst see what you think of the latest changes.

rjernst

Thanks @pugnascotia. I have a few questions.

rjernst · 2021-03-25T03:18:09Z

distribution/docker/src/docker/Dockerfile

-    ln -sf /etc/pki/ca-trust/extracted/java/cacerts /usr/share/elasticsearch/jdk/lib/security/cacerts
+    ln -sf /etc/pki/ca-trust/extracted/java/cacerts /usr/share/elasticsearch/jdk/lib/security/cacerts && \\
+    chmod 0755 /usr/share/elasticsearch && \\
+    chgrp 1000 bin && \\


Why does bin need to be group owned by the running user? The files all are o+rx right?

rjernst · 2021-03-25T03:19:38Z

distribution/docker/src/docker/Dockerfile

+    ln -sf /etc/pki/ca-trust/extracted/java/cacerts /usr/share/elasticsearch/jdk/lib/security/cacerts && \\
+    chmod 0755 /usr/share/elasticsearch && \\
+    chgrp 1000 bin && \\
+    chgrp -R 1000 config data logs plugins


Why does the plugins dir need to be grouped for the running user?

rjernst · 2021-03-25T03:20:32Z

qa/os/src/test/java/org/elasticsearch/packaging/util/FileMatcher.java

-    public static final Set<PosixFilePermission> p755 = fromString("rwxr-xr-x");
-    public static final Set<PosixFilePermission> p750 = fromString("rwxr-x---");
-    public static final Set<PosixFilePermission> p660 = fromString("rw-rw----");
+    public static final Set<PosixFilePermission> p444 = fromString("r--r--r--");


This is now unused right?

…rms-in-docker-image

pugnascotia · 2021-04-14T15:31:44Z

I realised that I had changed the image to use a default group of elasticsearch, but that would break the current guidance to use GID 0 for bind-mounted volumes (which e.g. OpenShift does), and probably be considered a breaking change.

Instead, the image still uses group root but now also uses user root as well. Since the image runs as elasticsearch, that should make it more difficult to trample files via an exploit, in conjunction with the tightened permissions.

pugnascotia · 2021-04-15T13:24:31Z

I went back over these changes again, and basically tried to lock things down as much as possible, while making it possible to install plugins without jumping through ridiculous hoops. See what you think.

pugnascotia · 2021-04-19T10:28:56Z

@elasticmachine update branch

pugnascotia · 2021-05-04T12:34:02Z

@elasticmachine update branch

rjernst

I have a few more questions, and I'm not sure all my previous comments were addressed.

rjernst · 2021-05-14T17:32:51Z

distribution/docker/src/docker/Dockerfile

+    find . -type d -exec chmod 0555 {} + && \\
+    find . -type f -exec chmod 0444 {} + && \\
+    chmod 0555 bin/* jdk/bin/* jdk/lib/jspawnhelper modules/x-pack-ml/platform/linux-\$(arch)/bin/* && \\
+    chmod 0775 config config/jvm.options.d data logs plugins && \\


Including plugins here seems problematic. They are code, and should therefore not be writeable by the user that runs elasticsearch.

I remember we discussed the plugins a while ago in #69533. Being able to install a plugin while running Elasticsearch as a non-root user (FWIW: ideally without the need to --group-add 0) is vital for Cloud deployments.

rjernst · 2021-05-14T17:33:47Z

distribution/docker/src/docker/Dockerfile

-    find . -type f -exec chmod o+r {} +
+    find . -type d -exec chmod 0555 {} + && \\
+    find . -type f -exec chmod 0444 {} + && \\
+    chmod 0555 bin/* jdk/bin/* jdk/lib/jspawnhelper modules/x-pack-ml/platform/linux-\$(arch)/bin/* && \\


The correct files already are executable in the original archive that is extracted. Why was the executable bit removed in the first place? Seems like we should just keep it as it was, instead of trying to reform what is executable here.

rjernst · 2021-05-14T17:34:58Z

distribution/docker/src/docker/Dockerfile

@@ -287,8 +285,7 @@ RUN ${package_manager} update --setopt=tsflags=nodocs -y && \\

 RUN groupadd -g 1000 elasticsearch && \\
    adduser -u 1000 -g 1000 -G 0 -d /usr/share/elasticsearch elasticsearch && \\
-    chmod 0775 /usr/share/elasticsearch && \\
-    chown -R 1000:0 /usr/share/elasticsearch
+    chown -R 0:0 /usr/share/elasticsearch


The rpm/deb packages setup ownership so that the group is the elasticsearch user, which is readable but not writable. Can we just keep the original permissions defined in the distribution and fix the ownership?

rjernst · 2021-05-14T17:37:03Z

qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java

@@ -147,7 +147,7 @@ public void test041AmazonCaCertsAreInTheKeystore() {
    public void test042KeystorePermissionsAreCorrect() throws Exception {
        waitForElasticsearch(installation);

-        assertPermissionsAndOwnership(installation.config("elasticsearch.keystore"), p660);
+        assertPermissionsAndOwnership(installation.config("elasticsearch.keystore"), "elasticsearch", "root", p660);


This seems backwards. Shouldn't the group be elasticsearch and the owner be root?

The startup process creates this if necessary at startup, so it has to be elasticsearch:root.

…rms-in-docker-image

pugnascotia · 2021-07-13T13:42:40Z

I've picked up this work again, and I've been scratching my head over what we're trying to do and what are the constraints. I think it would helpful to lay these out:

Docker images are immutable, therefore we don't need ES files to be writable e.g. for an upgrade. On the contrary, most files that we ship in the image should never be overwritten.
On a multi-user system, it is appropriate to install ES as root:elasticsearch. However in the Docker world, it is essentially a single-user system - although popular images like postgres and redis run as root by default and then drop to another user, because our 7.x images also do this they are routinely flagged in security scans
We recommend using GID 0 for sharing files via bind-mounts (OpenShift also recommends this, for example). Using an ownership of root:elasticsearch in the container would complicates this, versus root:elasticsearch

To sum up, I'm don't believe reusing the archive permissions is desirable.

What I've tried to do here is use root:root user for immutable files and directories, and use elasticsearch:root when they are writable. Rather than trying to tweak file permissions here and there, I decided that it was clearer to simply reset them all to known values that are more appropriate for the Docker environment.

Plugins complicate the picture, because not only does the executing user need write access to plugins, but it also (technically) needs write access to bin because plugins can install CLI tools. Essentially, the line between installing ES and running ES becomes non-existent, so we don't install as root (for example) and run as elasticsearch.

…mage' into restrict-write-perms-in-docker-image

…rms-in-docker-image

elasticsearchmachine · 2021-07-28T15:13:30Z

Hi @pugnascotia, I've created a changelog YAML for you.

Explicitly set permissions for all files in the Elasticsearch home directory to the minimum required set, and change ownership to `root:root` where possible.

pugnascotia · 2021-08-02T14:45:24Z

Backported to 7.x in c04dc4c.

This reverts commit c04dc4c.

mark-vieira · 2021-08-02T22:38:23Z

FYI, I've reverted the backport to 7.x in 8effc56. It was causing some packaging test failures: https://gradle-enterprise.elastic.co/s/kvmrl5ln52pv6

This re-applies commit 8effc56.

pugnascotia · 2021-08-03T09:35:12Z

Bah, sorry about that. New backport in #76006. FWIW, I did run the DockerTests before pushing, but I forgot about the KeystoreManagementTests.

* Reapply "Tighten up write permissions in Docker image (#70635)" This re-applies commit 8effc56. * Fix perms check for Docker in KeystoreManagementTests

Tighten up write permissions in Docker image

9ef552b

Recursively remove write access from the `bin`, `jdk`, `lib` and `modules` directories, since this access is not required, and removing it makes it harder to exploit other issues in an ES distribution.

pugnascotia added >enhancement :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 v7.13.0 labels Mar 22, 2021

pugnascotia requested review from mark-vieira and droberts195 March 22, 2021 13:22

elasticmachine added the Team:Delivery Meta label for Delivery team label Mar 22, 2021

droberts195 approved these changes Mar 22, 2021

View reviewed changes

Make file permissions settings completely explicit

000e018

Keep the file ownership as similar as possible to rpm/deb

0327e44

pugnascotia added 2 commits March 24, 2021 20:20

Formatting

6217e57

Test fixes

a1bf983

rjernst reviewed Mar 25, 2021

View reviewed changes

pugnascotia added 4 commits April 14, 2021 13:44

Merge remote-tracking branch 'upstream/master' into restrict-write-pe…

6b6b847

…rms-in-docker-image

Revert some changes that would have been considered breaking

e13ae49

More tweaks

4a0482d

Formatting

12e455a

mark-vieira added >enhancement and removed >enhancement labels Apr 14, 2021

Revert back to manualy hardening permissions

4ed7857

Formatting

01d3a6b

Merge branch 'master' into restrict-write-perms-in-docker-image

735a20c

pugnascotia added v7.14.0 and removed v7.13.0 labels Apr 21, 2021

Merge branch 'master' into restrict-write-perms-in-docker-image

45e7c91

rjernst reviewed May 14, 2021

View reviewed changes

pugnascotia mentioned this pull request Jun 9, 2021

Tighten up write permissions in Docker image #73942

Merged

mark-vieira added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

pugnascotia added 2 commits July 13, 2021 13:32

Merge remote-tracking branch 'upstream/master' into restrict-write-pe…

c5675be

…rms-in-docker-image

Tweaks

c8a617d

pugnascotia added 2 commits July 13, 2021 14:44

Merge remote-tracking branch 'origin/restrict-write-perms-in-docker-i…

53c07d6

…mage' into restrict-write-perms-in-docker-image

Merge remote-tracking branch 'upstream/master' into restrict-write-pe…

ba89e79

…rms-in-docker-image

Update docs/changelog/70635.yaml

31acf7b

pugnascotia merged commit 3740f67 into elastic:master Aug 2, 2021

pugnascotia deleted the restrict-write-perms-in-docker-image branch August 2, 2021 13:25

pugnascotia added a commit that referenced this pull request Aug 2, 2021

Tighten up write permissions in Docker image (#70635)

c04dc4c

Explicitly set permissions for all files in the Elasticsearch home directory to the minimum required set, and change ownership to `root:root` where possible.

mark-vieira added a commit that referenced this pull request Aug 2, 2021

Revert "Tighten up write permissions in Docker image (#70635)"

8effc56

This reverts commit c04dc4c.

pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Aug 3, 2021

Reapply "Tighten up write permissions in Docker image (elastic#70635)"

6d0102e

This re-applies commit 8effc56.

pugnascotia mentioned this pull request Aug 3, 2021

Tighten up write permissions in Docker image (#70635) #76006

Merged

mark-vieira added v8.0.0-alpha1 and removed v8.0.0 labels Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tighten up write permissions in Docker image #70635

Tighten up write permissions in Docker image #70635

pugnascotia commented Mar 22, 2021

elasticmachine commented Mar 22, 2021

droberts195 left a comment

rjernst commented Mar 22, 2021

mark-vieira commented Mar 22, 2021

pugnascotia commented Mar 23, 2021

rjernst commented Mar 23, 2021

pugnascotia commented Mar 24, 2021

rjernst left a comment

rjernst Mar 25, 2021

rjernst Mar 25, 2021

rjernst Mar 25, 2021

pugnascotia commented Apr 14, 2021

pugnascotia commented Apr 15, 2021

pugnascotia commented Apr 19, 2021

pugnascotia commented May 4, 2021

rjernst left a comment

rjernst May 14, 2021

mieciu Jun 8, 2021 •

edited

Loading

rjernst May 14, 2021

rjernst May 14, 2021

rjernst May 14, 2021

pugnascotia Jul 28, 2021

pugnascotia commented Jul 13, 2021

elasticsearchmachine commented Jul 28, 2021

pugnascotia commented Aug 2, 2021

mark-vieira commented Aug 2, 2021

pugnascotia commented Aug 3, 2021

Tighten up write permissions in Docker image #70635

Tighten up write permissions in Docker image #70635

Conversation

pugnascotia commented Mar 22, 2021

elasticmachine commented Mar 22, 2021

droberts195 left a comment

Choose a reason for hiding this comment

rjernst commented Mar 22, 2021

mark-vieira commented Mar 22, 2021

pugnascotia commented Mar 23, 2021

rjernst commented Mar 23, 2021

pugnascotia commented Mar 24, 2021

rjernst left a 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

pugnascotia commented Apr 14, 2021

pugnascotia commented Apr 15, 2021

pugnascotia commented Apr 19, 2021

pugnascotia commented May 4, 2021

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mieciu Jun 8, 2021 • 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

Choose a reason for hiding this comment

pugnascotia commented Jul 13, 2021

elasticsearchmachine commented Jul 28, 2021

pugnascotia commented Aug 2, 2021

mark-vieira commented Aug 2, 2021

pugnascotia commented Aug 3, 2021

mieciu Jun 8, 2021 •

edited

Loading