Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() by joachimdraeger · Pull Request #25254 · elastic/elasticsearch

joachimdraeger · 2017-06-15T13:22:02Z

Moved SocketAccess.doPrivileged up the stack to DefaultS3OutputStream in repository-S3 plugin to avoid SecurityException by Streams.copy(). A plugin is only allowed to use its own jars when performing privileged operations. The S3 client might open a new Socket on close(). #25192

… in repository-S3 plugin to avoid SecurityException by Streams.copy(). A plugin is only allowed to use its own jars when performing privileged operations. The S3 client might open a new Socket on close(). elastic#25192

elasticmachine · 2017-06-15T13:22:04Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Tim-Brooks · 2017-06-20T20:32:12Z

Thanks @joachimdraeger. I'll take a look at this.

Is this the only place where this occurs? I see that we close those S3OutputStream without protection in other places without protection:

try (OutputStream stream = createOutput(blobName)) {
            Streams.copy(inputStream, stream);
}

Maybe we should also override close(). And wrap the super.close() call in a doPrivileged block?

Tim-Brooks · 2017-06-20T20:34:11Z

@elasticmachine please test this

Tim-Brooks · 2017-06-20T20:34:54Z

...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/DefaultS3OutputStream.java

+        });
+    }
+
+    private void flushPriveleged(byte[] bytes, int off, int len, boolean closing) throws IOException {


Spelling: flushPrivileged

Tim-Brooks · 2017-06-20T20:38:41Z

Maybe we should also override close(). And wrap the super.close() call in a doPrivileged block?

Hmm. Now that I look at the source for S3OutputStream, I see that all the calls route through the flush method.

public abstract void flush(byte[] bytes, int off, int len, boolean closing) throws IOException;

So I think your change will be sufficient.

jasontedor · 2017-06-20T20:41:28Z

@tbrooks8 See #25206 for why we came to the approach here.

joachimdraeger · 2017-06-21T14:00:03Z

I added poc code for testing socket permissions. Test fails without the fix.

joachimdraeger · 2017-06-21T14:02:55Z

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java

+
+    private void openSocket() {
+        try {
+            Socket socket = new Socket("localhost", 9200);


Are there any guidelines which ports could be used in unit/integration tests? I think I wasn't able to use any other port than 9200 without changing further permissions.

joachimdraeger · 2017-06-21T16:36:12Z

Just found out about MockSocket, I can probably make this work nicely now.

joachimdraeger · 2017-06-21T17:16:43Z

@jasontedor, I added the Socket tests and changed it to use MockSocket and a random port. I think this is in-line with similar tests now.

jasontedor · 2017-06-21T17:21:39Z

Yes, mock socket is the way to go. I'll look closely soon.

jasontedor · 2017-06-23T11:26:52Z

test this please

joachimdraeger · 2017-06-26T15:30:02Z

Did you forget to tag elasticmachine, @jasontedor?

s1monw · 2017-06-26T15:34:33Z

@elasticmachine test this please

jasontedor · 2017-06-27T00:49:20Z

@joachimdraeger No, that's not necessary. In fact, I had checked on the status of the build previously and it was green. What happened here is that the cleanup of the PR builds is quite aggressive, they only stay outstanding for a few days (to be precise, I think that it's three). Thus, the build was cleaned up sometime over the weekend and that leads to the PR status no longer showing up on the PR. Thankfully, @s1monw kicked off another build and we see here that we are still green.

jasontedor

Thanks @joachimdraeger, this generally looks good but I left some more thorough comments.

jasontedor · 2017-06-27T00:49:46Z

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java

+        this.mockSocketPort = mockSocketPort;
+    }
+
+    // simulate a socket connection to check that SocketAccess is used correctly


Can you elaborate a bit more why this necessary, exactly what we are simulating here?

jasontedor · 2017-06-27T00:50:26Z

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java

+    // simulate a socket connection to check that SocketAccess is used correctly
+    private void simulateS3SocketConnection() {
+        try {
+            Socket socket = new Socket(InetAddress.getByName("127.0.0.1"), mockSocketPort);


Maybe wrap this in a try-with-resources instead?

jasontedor · 2017-06-27T00:50:35Z

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java

+            Socket socket = new Socket(InetAddress.getByName("127.0.0.1"), mockSocketPort);
+            socket.close();
+        } catch (IOException e) {
+            throw new RuntimeException(e);


How about throwing UncheckedIOException instead?

jasontedor · 2017-06-27T00:52:56Z

...repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreContainerTests.java

+    public static void openMockSocket() throws IOException {
+        socket = new MockServerSocket(0, 50, InetAddress.getByName("127.0.0.1"));
+        new Thread(() -> {
+            while (!socket.isClosed()) {


It's not obvious to me what is going on here, can you please explain?

jasontedor · 2017-06-27T00:53:04Z

...repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreContainerTests.java

+                try {
+                    socket.accept();
+                } catch (IOException e) {
+                    logger.log(Level.ERROR, e);


I don't understand this?

jasontedor · 2017-06-27T00:53:38Z

...repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreContainerTests.java

+    @BeforeClass
+    public static void openMockSocket() throws IOException {
+        socket = new MockServerSocket(0, 50, InetAddress.getByName("127.0.0.1"));
+        new Thread(() -> {


This thread can leak and fail the test, I think that you need to clean it up (join on it in tear down).

…bStoreContainerTests and further improvements.

joachimdraeger · 2017-07-03T16:20:35Z

Thanks for the review @jasontedor, I implemented the suggested changes and hope that the additional documentation makes it clearer.

jasontedor

LGTM, I left one more request though. Did you want to take another look @tbrooks8?

jasontedor · 2017-07-03T16:31:56Z

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/MockAmazonS3.java

-            Socket socket = new Socket(InetAddress.getByName("127.0.0.1"), mockSocketPort);
-            socket.close();
+        try (Socket socket = new Socket(InetAddress.getByName("127.0.0.1"), mockSocketPort)) {
+            Assert.assertTrue(socket.isConnected()); // NOOP to keep static analysis happy


We tend to static import these assert methods; would you mind doing the same here so this can only be assertTrue without the class qualifier?

Sure, no problem

Tim-Brooks

I mean this all looks alright to me. I just kind of have a question about how we handle the server socket that we must open for this test.

Tim-Brooks · 2017-07-03T16:38:36Z

...repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobStoreContainerTests.java

+                    // Accept connections from MockAmazonS3.
+                    mockS3ServerSocket.accept();
+                } catch (IOException e) {
+                    logger.log(Level.ERROR, e);


I think that this will generate an error in the logs most of the time when this test ends. As we will call close and I think accept throws an exception when the socket is closed. @jasontedor - is this alright?

That's a good point @tbrooks8, we should not do this.

I'll remove it

joachimdraeger · 2017-07-05T09:27:24Z

Thanks for the review @tbrooks8. Could you have another look?

Tim-Brooks

LGTM

joachimdraeger · 2017-07-07T13:58:54Z

Ready to merge @jasontedor ?

* master: Remove deprecated created and found from index, delete and bulk (elastic#25516) fix testEnsureVersionCompatibility for 5.5.0 release fix Version.v6_0_0 min compatibility version to 5.5.0 Add bwc indices for 5.5.0 Add v5_5_1 constant [DOCS] revise high level client Search Scroll API docs (elastic#25599) Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (elastic#24437) Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() (elastic#25254) [Tests] Add tests for CompletionSuggestionBuilder#build() (elastic#25575)

* master: (42 commits) Harden global checkpoint tracker Remove deprecated created and found from index, delete and bulk (elastic#25516) fix testEnsureVersionCompatibility for 5.5.0 release fix Version.v6_0_0 min compatibility version to 5.5.0 Add bwc indices for 5.5.0 Add v5_5_1 constant [DOCS] revise high level client Search Scroll API docs (elastic#25599) Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (elastic#24437) Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() (elastic#25254) [Tests] Add tests for CompletionSuggestionBuilder#build() (elastic#25575) Enable cross-setting validation [Docs] Fix typo in bootstrap-checks.asciidoc (elastic#25597) Index ids in binary form. (elastic#25352) bwc checkout should fetch from all remotes IndexingIT should check for global checkpoints regardless of master version [Tests] Add tests for PhraseSuggestionBuilder#build() (elastic#25571) Remove unused class MinimalMap (elastic#25590) [Docs] Document Scroll API for Java High Level REST Client (elastic#25554) Disable date field mapping changing (elastic#25285) Allow BWC Testing against a specific branch (elastic#25510) ...

jasontedor · 2017-07-08T11:46:32Z

Thanks for merging @tbrooks8.

joachimdraeger mentioned this pull request Jun 15, 2017

Fix SecurityException in repository-s3 plugin #25206

Closed

PoC for checking SocketPetmissions in repository-s3 plugin

e924786

clintongormley requested a review from Tim-Brooks June 20, 2017 12:18

Tim-Brooks requested changes Jun 20, 2017

View reviewed changes

joachimdraeger added 2 commits June 21, 2017 10:27

Fix spelling

a136d93

Merge branch 's3-socket-perm-tests' into s3-security-exception-flush

22774be

joachimdraeger commented Jun 21, 2017

View reviewed changes

fix api checks

e47d360

Use MockSocket to simulate S3 connections

f9fe3b9

jasontedor requested changes Jun 27, 2017

View reviewed changes

More documentation to clarify the role of using a MockSocket in S3Blo…

bd47ee9

…bStoreContainerTests and further improvements.

jasontedor approved these changes Jul 3, 2017

View reviewed changes

Tim-Brooks reviewed Jul 3, 2017

View reviewed changes

Removed obsolete IOException in test, static import of assertTrue

90ea0ad

Tim-Brooks approved these changes Jul 5, 2017

View reviewed changes

Tim-Brooks merged commit 1ff2c13 into elastic:master Jul 7, 2017

clintongormley added :Plugin Repository S3 >bug v6.0.0 labels Jul 10, 2017

Tim-Brooks mentioned this pull request Jul 10, 2017

intermittent SecurityException when creating s3-repository snapshots #25192

Closed

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

clintongormley added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository S3 labels Feb 14, 2018

Conversation

joachimdraeger commented Jun 15, 2017

Uh oh!

elasticmachine commented Jun 15, 2017

Uh oh!

Tim-Brooks commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tim-Brooks commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Jun 20, 2017

Uh oh!

jasontedor commented Jun 20, 2017

Uh oh!

joachimdraeger commented Jun 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joachimdraeger commented Jun 21, 2017

Uh oh!

joachimdraeger commented Jun 21, 2017

Uh oh!

jasontedor commented Jun 21, 2017

Uh oh!

jasontedor commented Jun 23, 2017

Uh oh!

joachimdraeger commented Jun 26, 2017

Uh oh!

s1monw commented Jun 26, 2017

Uh oh!

jasontedor commented Jun 27, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joachimdraeger commented Jul 3, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joachimdraeger commented Jul 5, 2017

Uh oh!

Tim-Brooks left a comment

Choose a reason for hiding this comment

Uh oh!

joachimdraeger commented Jul 7, 2017

Uh oh!

jasontedor commented Jul 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Tim-Brooks commented Jun 20, 2017 •

edited

Loading