From 0c1847d94fa8bd81661e7461dd9874f4eef6bf59 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 16:59:39 +0300 Subject: [PATCH] Rewrite the CSV file through a private copy next to it and never replace it with a fragment Update and delete rebuilt the CSV in a File.createTempFile in the system temp directory - readable by other local users while the whole file, passwords included, was written there - and moved it over the CSV unconditionally, so a rewrite that failed half-way replaced the data with the rows written so far, and the CSV came back with the temp file's permissions instead of its own. The copy is now created next to the CSV with owner-only permissions, takes over the CSV's permissions and replaces it atomically, and only when the rewrite completed; otherwise it is deleted. The write lock is released in its own finally so that a failing close() can not keep it. The directory a connector bundle is expanded into is created with Files.createTempDirectory, owner-only on POSIX file systems. --- OpenICF-csvfile-connector/pom.xml | 6 +- .../openicf/csvfile/CSVFileConnector.java | 140 +++++++++++------- .../openicf/csvfile/RewriteSafetyTest.java | 137 +++++++++++++++++ .../local/LocalConnectorInfoManagerImpl.java | 12 +- .../api/LocalConnectorInfoManagerTests.java | 46 ++++++ 5 files changed, 273 insertions(+), 68 deletions(-) create mode 100644 OpenICF-csvfile-connector/src/test/java/org/forgerock/openicf/csvfile/RewriteSafetyTest.java diff --git a/OpenICF-csvfile-connector/pom.xml b/OpenICF-csvfile-connector/pom.xml index 78a1cfb4..daece0a5 100644 --- a/OpenICF-csvfile-connector/pom.xml +++ b/OpenICF-csvfile-connector/pom.xml @@ -14,7 +14,7 @@ * own identifying information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2011-2016 ForgeRock AS. - * Portions Copyrighted 2018-2024 3A Systems, LLC + * Portions Copyrighted 2018-2026 3A Systems, LLC */ --> @@ -46,10 +46,6 @@ super-csv 2.3.1 - - commons-io - commons-io - diff --git a/OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java b/OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java index f0a2b0a2..f41c13df 100644 --- a/OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java +++ b/OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java @@ -14,16 +14,21 @@ * Copyright 2015-2016 ForgeRock AS * Portions Copyright 2011 Viliam Repan * Portions Copyright 2011 Radovan Semancik + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.openicf.csvfile; import java.io.BufferedReader; +import java.io.Closeable; import java.io.File; import java.io.FileNotFoundException; import java.io.FileReader; import java.io.FileWriter; import java.io.IOException; +import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -45,7 +50,6 @@ import java.util.regex.Pattern; import org.identityconnectors.common.Base64; -import org.apache.commons.io.FileUtils; import org.identityconnectors.common.logging.Log; import org.identityconnectors.common.security.GuardedString; import org.identityconnectors.common.security.SecurityUtil; @@ -1003,14 +1007,15 @@ private Uid doCreate(Set attributes, OperationOptions options) { } catch (IOException e) { throw new ConnectorException("Failed to create object", e); } finally { - if (mapWriter != null) { - try { + try { + if (mapWriter != null) { mapWriter.close(); - } catch (IOException e) { - log.error(e, "Failed to close CSV file after create"); } + } catch (Exception e) { + log.error(e, "Failed to close CSV file after create"); + } finally { + lock.unlock(); } - lock.unlock(); } return uid; @@ -1025,12 +1030,13 @@ private void doDelete(Uid uid, OperationOptions options) { ICsvMapReader reader = null; ICsvMapWriter writer = null; File tmp = null; + boolean rewritten = false; final WriteLock lock = fileNameToLockMap.get(csvFilePath).writeLock(); lock.lock(); try { reader = new CsvMapReader(new FileReader(config.getCsvFile()), csvPreference); - tmp = File.createTempFile("csvfile", "tmp"); + tmp = createRewriteFile(); writer = new CsvMapWriter(new FileWriter(tmp), csvPreference); final CellProcessor[] processors = getProcessors(header); @@ -1046,10 +1052,10 @@ private void doDelete(Uid uid, OperationOptions options) { } } if (!found) { - tmp.delete(); throw new UnknownUidException("Object for uid " + uid.toString() + " does not exist"); } totalRowCount.put(csvFilePath, totalRowCount.get(csvFilePath) - 1); + rewritten = true; } catch (FileNotFoundException e) { log.error(e, "File {0} does not exist!", config.getCsvFile().toString()); throw new ConnectorIOException("File " + config.getCsvFile().toString() + " does not exist", e); @@ -1057,31 +1063,13 @@ private void doDelete(Uid uid, OperationOptions options) { log.error(e, "Error reading from {0}!", config.getCsvFile().toString()); throw new ConnectorIOException("Error reading from file " + config.getCsvFile().toString(), e); } finally { - if (reader != null) { - try { - reader.close(); - } catch (Exception e) { - log.error(e, "Error closing file reader"); - } - } - if (writer != null) { - try { - writer.close(); - } catch (Exception e) { - log.error(e, "Error closing file writer"); - } - } - if (tmp != null && tmp.exists()) { - try { - if (config.getCsvFile().getAbsoluteFile().exists()) { - config.getCsvFile().getAbsoluteFile().delete(); - } - FileUtils.moveFile(tmp.getAbsoluteFile(), config.getCsvFile().getAbsoluteFile()); - } catch (Exception e) { - log.error(e, "Error renaming file"); - } + try { + closeQuietly(reader, "reader"); + closeQuietly(writer, "writer"); + finishRewrite(tmp, rewritten); + } finally { + lock.unlock(); } - lock.unlock(); } } @@ -1098,12 +1086,13 @@ private Uid doUpdate(UpdateType type, Uid uid, Set attributes, Operat ICsvMapReader reader = null; ICsvMapWriter writer = null; File tmp = null; + boolean rewritten = false; final WriteLock lock = fileNameToLockMap.get(csvFilePath).writeLock(); lock.lock(); try { reader = new CsvMapReader(new FileReader(config.getCsvFile()), csvPreference); - tmp = File.createTempFile("csvfile", "tmp"); + tmp = createRewriteFile(); writer = new CsvMapWriter(new FileWriter(tmp), csvPreference); writer.writeHeader(header); @@ -1129,6 +1118,7 @@ private Uid doUpdate(UpdateType type, Uid uid, Set attributes, Operat if (updated == null) { throw new UnknownUidException("Uid " + uid.getUidValue() + " does not exist"); } + rewritten = true; } catch (FileNotFoundException e) { log.error(e, "File {0} does not exist!", config.getCsvFile().toString()); throw new ConnectorIOException("File " + config.getCsvFile().toString() + " does not exist", e); @@ -1136,34 +1126,72 @@ private Uid doUpdate(UpdateType type, Uid uid, Set attributes, Operat log.error(e, "Error reading from {0}!", config.getCsvFile().toString()); throw new ConnectorIOException("Error reading from file " + config.getCsvFile().toString(), e); } finally { - if (reader != null) { - try { - reader.close(); - } catch (Exception e) { - log.error(e, "Error closing file reader"); - } + try { + closeQuietly(reader, "reader"); + closeQuietly(writer, "writer"); + finishRewrite(tmp, rewritten); + } finally { + lock.unlock(); } - if (writer != null) { - try { - writer.close(); - } catch (Exception e) { - log.error(e, "Error closing file writer"); - } + } + + return updated; + } + + /** + * Creates the file a rewritten copy of the CSV is assembled in: next to + * the CSV, so that the copy can take its place with a rename on the same + * file system, and readable by its owner only, so that the data is not + * exposed to other local users while it is being written. + */ + private File createRewriteFile() throws IOException { + File csv = config.getCsvFile().getAbsoluteFile(); + return Files.createTempFile(csv.getParentFile().toPath(), csv.getName() + ".", ".tmp") + .toFile(); + } + + /** + * Puts the rewritten copy in place of the CSV when the rewrite completed, + * keeping the CSV's permissions; a copy of a rewrite that failed half-way + * is discarded so that the CSV stays as it was. + */ + private void finishRewrite(File tmp, boolean rewritten) { + if (tmp == null) { + return; + } + if (!rewritten) { + if (!tmp.delete() && tmp.exists()) { + log.warn("Could not delete {0}", tmp); } - if (tmp != null) { - try { - if (config.getCsvFile().getAbsoluteFile().exists()) { - config.getCsvFile().getAbsoluteFile().delete(); - } - FileUtils.moveFile(tmp.getAbsoluteFile(), config.getCsvFile().getAbsoluteFile()); - } catch (Exception e) { - log.error(e, "Error renaming file"); - } + return; + } + Path csv = config.getCsvFile().getAbsoluteFile().toPath(); + try { + try { + Files.setPosixFilePermissions(tmp.toPath(), Files.getPosixFilePermissions(csv)); + } catch (UnsupportedOperationException e) { + // not a POSIX file system: the copy inherits the directory's ACL } - lock.unlock(); + try { + Files.move(tmp.toPath(), csv, StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE); + } catch (AtomicMoveNotSupportedException e) { + Files.move(tmp.toPath(), csv, StandardCopyOption.REPLACE_EXISTING); + } + } catch (IOException e) { + throw new ConnectorIOException("Failed to replace " + csv + " with its rewritten copy " + + tmp, e); } + } - return updated; + private static void closeQuietly(Closeable closeable, String what) { + if (closeable != null) { + try { + closeable.close(); + } catch (Exception e) { + log.error(e, "Error closing file {0}", what); + } + } } private String getAttributeValue(Attribute attr) { diff --git a/OpenICF-csvfile-connector/src/test/java/org/forgerock/openicf/csvfile/RewriteSafetyTest.java b/OpenICF-csvfile-connector/src/test/java/org/forgerock/openicf/csvfile/RewriteSafetyTest.java new file mode 100644 index 00000000..51698fee --- /dev/null +++ b/OpenICF-csvfile-connector/src/test/java/org/forgerock/openicf/csvfile/RewriteSafetyTest.java @@ -0,0 +1,137 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package org.forgerock.openicf.csvfile; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.fail; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.Collections; +import java.util.Set; + +import org.identityconnectors.framework.common.objects.Attribute; +import org.identityconnectors.framework.common.objects.AttributeBuilder; +import org.identityconnectors.framework.common.objects.ObjectClass; +import org.identityconnectors.framework.common.objects.Uid; +import org.testng.SkipException; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +/** + * Update and delete rewrite the whole CSV file through a temporary copy. + * That copy must not weaken the permissions of the data file, and a rewrite + * that fails half-way must not replace the data file with the fragment. + */ +public class RewriteSafetyTest { + + private static final String HEADER = "firstName,uid,lastName,password\n"; + private static final String VILO = "\"viliam\",\"vilo\",\"repan\",\"Z29vZA==\"\n"; + private static final String MALFORMED = "\"too\",\"few\"\n"; + private static final String MISO = "\"michal\",\"miso\",\"kovac\",\"Z29vZA==\"\n"; + + private File csv; + private CSVFileConnector connector; + + @BeforeMethod + public void before() throws Exception { + csv = File.createTempFile("rewrite", ".csv"); + write(HEADER + VILO + MISO); + + CSVFileConfiguration config = new CSVFileConfiguration(); + config.setCsvFile(csv); + config.setHeaderUid("uid"); + config.setHeaderPassword("password"); + + connector = new CSVFileConnector(); + connector.init(config); + } + + @AfterMethod + public void after() { + connector.dispose(); + connector = null; + csv.delete(); + } + + @Test + public void updateKeepsFilePermissions() throws Exception { + Set ownerOnly = restrictToOwner(); + + connector.update(ObjectClass.ACCOUNT, new Uid("vilo"), lastName("updated"), null); + + assertEquals(Files.getPosixFilePermissions(csv.toPath()), ownerOnly); + } + + @Test + public void deleteKeepsFilePermissions() throws Exception { + Set ownerOnly = restrictToOwner(); + + connector.delete(ObjectClass.ACCOUNT, new Uid("vilo"), null); + + assertEquals(Files.getPosixFilePermissions(csv.toPath()), ownerOnly); + } + + @Test + public void failedUpdateLeavesFileUntouched() throws Exception { + String original = HEADER + VILO + MALFORMED + MISO; + write(original); + try { + connector.update(ObjectClass.ACCOUNT, new Uid("vilo"), lastName("updated"), null); + fail("Expected the malformed row to fail the update"); + } catch (RuntimeException expected) { + assertEquals(read(), original, "the data file was replaced by the partial rewrite"); + } + } + + @Test + public void failedDeleteLeavesFileUntouched() throws Exception { + String original = HEADER + VILO + MALFORMED + MISO; + write(original); + try { + connector.delete(ObjectClass.ACCOUNT, new Uid("vilo"), null); + fail("Expected the malformed row to fail the delete"); + } catch (RuntimeException expected) { + assertEquals(read(), original, "the data file was replaced by the partial rewrite"); + } + } + + private Set restrictToOwner() throws Exception { + if (!FileSystems.getDefault().supportedFileAttributeViews().contains("posix")) { + throw new SkipException("POSIX file permissions are not supported here"); + } + Set ownerOnly = PosixFilePermissions.fromString("rw-------"); + Files.setPosixFilePermissions(csv.toPath(), ownerOnly); + return ownerOnly; + } + + private static Set lastName(String value) { + return Collections.singleton(AttributeBuilder.build("lastName", value)); + } + + private void write(String content) throws Exception { + Files.write(csv.toPath(), content.getBytes(StandardCharsets.UTF_8)); + } + + private String read() throws Exception { + return new String(Files.readAllBytes(csv.toPath()), StandardCharsets.UTF_8); + } +} diff --git a/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java b/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java index 445606b0..ab0bd34b 100644 --- a/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java +++ b/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java @@ -21,6 +21,7 @@ * ==================== * Portions Copyrighted 2010-2015 ForgeRock AS. * Portions Copyrighted 2010-2014 Tirasa. + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.framework.impl.api.local; @@ -32,6 +33,7 @@ import java.lang.annotation.Annotation; import java.net.URISyntaxException; import java.net.URL; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -492,13 +494,9 @@ private File getBundleTempDir() throws IOException { if (!tempDir.canWrite()) { throw new IOException("Temporary directory " + tempDir + " is read/only"); } - File candidate; - do { - candidate = new File(tempDir, "bundle-" + nextRandom()); - } while (candidate.exists()); - if (!candidate.mkdir()) { - throw new IOException("Temporary directory " + tempDir + " is read/only"); - } + // created with a unique name and, on POSIX file systems, readable + // by the owner only: it holds the bundle's libraries + final File candidate = Files.createTempDirectory(tempDir.toPath(), "bundle-").toFile(); candidate.deleteOnExit(); _bundleTempDir = candidate; return candidate; diff --git a/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java b/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java index 881c07d2..2e7c918b 100644 --- a/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java +++ b/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java @@ -19,13 +19,19 @@ * enclosed by brackets [] replaced by your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" * ==================== + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.framework.impl.api; +import java.io.File; import java.net.URL; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.attribute.PosixFilePermissions; import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import org.identityconnectors.common.Version; @@ -49,7 +55,9 @@ import org.identityconnectors.framework.common.objects.Uid; import org.identityconnectors.framework.api.operations.batch.BatchBuilder; import org.identityconnectors.framework.api.operations.batch.BatchTask; +import org.identityconnectors.framework.impl.api.local.LocalConnectorInfoManagerImpl; import org.testng.Assert; +import org.testng.SkipException; import org.testng.annotations.Test; import static org.testng.Assert.assertEquals; @@ -86,6 +94,44 @@ public void testCheckVersion() throws Exception { } } + /** + * The directory a bundle is expanded into (its lib/ and native/ entries) + * lives in java.io.tmpdir and must not be readable by other local users. + */ + @Test + public void testBundleTempDirectoryIsPrivate() throws Exception { + if (!FileSystems.getDefault().supportedFileAttributeViews().contains("posix")) { + throw new SkipException("POSIX file permissions are not supported here"); + } + File tmpDir = new File(System.getProperty("java.io.tmpdir")); + Set before = bundleTempDirs(tmpDir); + + // a manager of its own: the cached one has expanded the bundles already + new LocalConnectorInfoManagerImpl(getTestBundles(), ConnectorInfoManagerFactory.class + .getClassLoader()); + + Set created = bundleTempDirs(tmpDir); + created.removeAll(before); + assertFalse(created.isEmpty(), "no bundle temp directory was created in " + tmpDir); + for (String name : created) { + assertEquals(Files.getPosixFilePermissions(new File(tmpDir, name).toPath()), + PosixFilePermissions.fromString("rwx------"), name); + } + } + + private static Set bundleTempDirs(File tmpDir) { + Set names = new HashSet(); + String[] entries = tmpDir.list(); + if (entries != null) { + for (String name : entries) { + if (name.startsWith("bundle-") && new File(tmpDir, name).isDirectory()) { + names.add(name); + } + } + } + return names; + } + /** * To be overridden by subclasses to get different ConnectorInfoManagers *