From fc88dfe1a6890776e3ee3c2593e02f79a9a255df Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Thu, 20 Oct 2016 16:36:35 +0200 Subject: [PATCH] CertificateTool must not generate world redeable files (elastic/elasticsearch#3810) This commit changes the permissions of the files generated by the certgen tool to 600 (like syskeygen does) Original commit: elastic/x-pack-elasticsearch@bca74e9c9284880c408e41128141a611d606f12b --- .../xpack/ssl/CertificateTool.java | 12 +++- .../xpack/ssl/CertificateToolTests.java | 69 +++++++++++++++++-- .../org/elasticsearch/xpack/ssl/instances.yml | 14 ---- 3 files changed, 75 insertions(+), 20 deletions(-) delete mode 100644 elasticsearch/src/test/resources/org/elasticsearch/xpack/ssl/instances.yml diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/ssl/CertificateTool.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/ssl/CertificateTool.java index 1ce14b5f154..7ef2b966042 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/ssl/CertificateTool.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/ssl/CertificateTool.java @@ -23,6 +23,7 @@ import org.elasticsearch.common.ParseFieldMatcherSupplier; import org.elasticsearch.common.Strings; import org.elasticsearch.common.network.InetAddresses; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; @@ -34,6 +35,8 @@ import org.elasticsearch.xpack.XPackPlugin; import javax.security.auth.x500.X500Principal; import java.io.OutputStream; import java.io.Reader; +import java.nio.file.attribute.PosixFileAttributeView; +import java.nio.file.attribute.PosixFilePermission; import java.security.cert.Certificate; import java.io.IOException; import java.io.OutputStreamWriter; @@ -371,10 +374,17 @@ public class CertificateTool extends SettingCommand { ZipOutputStream zipOutputStream = new ZipOutputStream(outputStream, StandardCharsets.UTF_8); JcaPEMWriter pemWriter = new JcaPEMWriter(new OutputStreamWriter(zipOutputStream, StandardCharsets.UTF_8))) { writer.write(zipOutputStream, pemWriter); + + // set permissions to 600 + PosixFileAttributeView view = Files.getFileAttributeView(file, PosixFileAttributeView.class); + if (view != null) { + view.setPermissions(Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)); + } + success = true; } finally { if (success == false) { - Files.delete(file); + Files.deleteIfExists(file); } } } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java index 31d5788c774..7c13b81a90f 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java @@ -5,6 +5,9 @@ */ package org.elasticsearch.xpack.ssl; +import com.google.common.jimfs.Configuration; +import com.google.common.jimfs.Jimfs; +import org.apache.lucene.util.IOUtils; import org.bouncycastle.asn1.ASN1String; import org.bouncycastle.asn1.DEROctetString; import org.bouncycastle.asn1.pkcs.Attribute; @@ -27,15 +30,19 @@ import org.elasticsearch.xpack.XPackPlugin; import org.elasticsearch.xpack.ssl.CertificateTool.CAInfo; import org.elasticsearch.xpack.ssl.CertificateTool.CertificateInformation; import org.elasticsearch.xpack.ssl.CertificateTool.Name; +import org.junit.After; import javax.security.auth.x500.X500Principal; +import java.io.IOException; import java.io.Reader; import java.net.InetAddress; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.nio.file.FileSystem; import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermission; import java.security.KeyPair; import java.security.PrivateKey; import java.security.cert.Certificate; @@ -51,6 +58,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -63,6 +71,23 @@ import static org.hamcrest.Matchers.instanceOf; */ public class CertificateToolTests extends ESTestCase { + private FileSystem jimfs; + + private Path initTempDir() throws Exception { + Configuration conf = Configuration.unix().toBuilder().setAttributeViews("posix").build(); + jimfs = Jimfs.newFileSystem(conf); + Path tempDir = jimfs.getPath("temp"); + IOUtils.rm(tempDir); + Files.createDirectories(tempDir); + return tempDir; + } + + @After + public void tearDown() throws Exception { + IOUtils.close(jimfs); + super.tearDown(); + } + public void testOutputDirectory() throws Exception { Environment env = new Environment(Settings.builder().put("path.home", createTempDir()).build()); Path outputDir = createTempDir(); @@ -138,7 +163,8 @@ public class CertificateToolTests extends ESTestCase { } public void testParsingFile() throws Exception { - Path instanceFile = getDataPath("instances.yml"); + Path tempDir = initTempDir(); + Path instanceFile = writeInstancesTo(tempDir.resolve("instances.yml")); Collection certInfos = CertificateTool.parseFile(instanceFile); assertEquals(4, certInfos.size()); @@ -166,9 +192,9 @@ public class CertificateToolTests extends ESTestCase { } public void testGeneratingCsr() throws Exception { - Path tempDir = createTempDir(); + Path tempDir = initTempDir(); Path outputFile = tempDir.resolve("out.zip"); - Path instanceFile = getDataPath("instances.yml"); + Path instanceFile = writeInstancesTo(tempDir.resolve("instances.yml")); Collection certInfos = CertificateTool.parseFile(instanceFile); assertEquals(4, certInfos.size()); @@ -176,6 +202,11 @@ public class CertificateToolTests extends ESTestCase { CertificateTool.generateAndWriteCsrs(outputFile, certInfos, randomFrom(1024, 2048)); assertTrue(Files.exists(outputFile)); + Set perms = Files.getPosixFilePermissions(outputFile); + assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_READ)); + assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_WRITE)); + assertEquals(perms.toString(), 2, perms.size()); + FileSystem fileSystem = FileSystems.newFileSystem(new URI("jar:" + outputFile.toUri()), Collections.emptyMap()); Path zipRoot = fileSystem.getPath("/"); @@ -201,9 +232,9 @@ public class CertificateToolTests extends ESTestCase { } public void testGeneratingSignedCertificates() throws Exception { - Path tempDir = createTempDir(); + Path tempDir = initTempDir(); Path outputFile = tempDir.resolve("out.zip"); - Path instanceFile = getDataPath("instances.yml"); + Path instanceFile = writeInstancesTo(tempDir.resolve("instances.yml")); Collection certInfos = CertificateTool.parseFile(instanceFile); assertEquals(4, certInfos.size()); @@ -219,6 +250,11 @@ public class CertificateToolTests extends ESTestCase { CertificateTool.generateAndWriteSignedCertificates(outputFile, certInfos, caInfo, keysize, days); assertTrue(Files.exists(outputFile)); + Set perms = Files.getPosixFilePermissions(outputFile); + assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_READ)); + assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_WRITE)); + assertEquals(perms.toString(), 2, perms.size()); + FileSystem fileSystem = FileSystems.newFileSystem(new URI("jar:" + outputFile.toUri()), Collections.emptyMap()); Path zipRoot = fileSystem.getPath("/"); @@ -421,4 +457,27 @@ public class CertificateToolTests extends ESTestCase { } while (valid == false); return name; } + + /** + * Writes the description of instances to a given {@link Path} + */ + private Path writeInstancesTo(Path path) throws IOException { + Iterable instances = Arrays.asList( + "instances:", + " - name: \"node1\"", + " ip:", + " - \"127.0.0.1\"", + " dns: \"localhost\"", + " - name: \"node2\"", + " filename: \"node2\"", + " ip: \"::1\"", + " - name: \"node3\"", + " filename: \"node3\"", + " - name: \"CN=different value\"", + " filename: \"different file\"", + " dns:", + " - \"node4.mydomain.com\""); + + return Files.write(path, instances, StandardCharsets.UTF_8); + } } diff --git a/elasticsearch/src/test/resources/org/elasticsearch/xpack/ssl/instances.yml b/elasticsearch/src/test/resources/org/elasticsearch/xpack/ssl/instances.yml deleted file mode 100644 index c6ad9682079..00000000000 --- a/elasticsearch/src/test/resources/org/elasticsearch/xpack/ssl/instances.yml +++ /dev/null @@ -1,14 +0,0 @@ -instances: - - name: "node1" - ip: - - "127.0.0.1" - dns: "localhost" - - name: "node2" - filename: "node2" - ip: "::1" - - name: "node3" - filename: "node3" - - name: "CN=different value" - filename: "different file" - dns: - - "node4.mydomain.com"