This is an automated email from the ASF dual-hosted git repository.
sijie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 8cc967c ISSUE #1049: Add PERMITTED_STARTUP_USERS to limit bookie
startup users
8cc967c is described below
commit 8cc967c9e8dee2e9225075b4c615307389df0057
Author: Samuel Just <[email protected]>
AuthorDate: Sun Jan 28 01:34:26 2018 -0800
ISSUE #1049: Add PERMITTED_STARTUP_USERS to limit bookie startup users
Starting the bookie as the wrong user (such as root) can cause problems
as the on disk files may end up with the wrong permissions. Add
PERMITTED_STARTUP_USERS to protect against this kind of operational
error.
(bug W-3599751)
Signed-off-by: Dustin Castor <dcastorsalesforce.com>
[Adapted to current code]
Signed-off-by: Samuel Just <sjustsalesforce.com>
Author: Samuel Just <[email protected]>
Reviewers: Enrico Olivelli <[email protected]>, Sijie Guo
<[email protected]>
This closes #1065 from athanatos/forupstream/permitted-startup-users,
closes #1049
---
.../bookkeeper/conf/AbstractConfiguration.java | 18 +++-
.../org/apache/bookkeeper/proto/BookieServer.java | 26 ++++++
.../bookie/BookieInitializationTest.java | 101 +++++++++++++++++++++
3 files changed, 144 insertions(+), 1 deletion(-)
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
index ef8e908..e772c79 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
@@ -22,7 +22,6 @@ import static
org.apache.bookkeeper.conf.ClientConfiguration.CLIENT_AUTH_PROVIDE
import java.net.URL;
import java.util.Iterator;
import java.util.List;
-
import javax.net.ssl.SSLEngine;
import org.apache.bookkeeper.feature.Feature;
@@ -121,6 +120,9 @@ public abstract class AbstractConfiguration<T extends
AbstractConfiguration>
// Kluge for compatibility testing. Never set this outside tests.
public static final String LEDGER_MANAGER_FACTORY_DISABLE_CLASS_CHECK =
"ledgerManagerFactoryDisableClassCheck";
+ // Validate bookie process user
+ public static final String PERMITTED_STARTUP_USERS =
"permittedStartupUsers";
+
protected AbstractConfiguration() {
super();
if (READ_SYSTEM_PROPERTIES) {
@@ -130,6 +132,20 @@ public abstract class AbstractConfiguration<T extends
AbstractConfiguration>
}
/**
+ * Limit who can start the application to prevent future permission errors.
+ */
+ public void setPermittedStartupUsers(String s) {
+ setProperty(PERMITTED_STARTUP_USERS, s);
+ }
+
+ /**
+ * Get array of users specified in this property.
+ */
+ public String[] getPermittedStartupUsers() {
+ return getStringArray(PERMITTED_STARTUP_USERS);
+ }
+
+ /**
* You can load configurations in precedence order. The first one takes
* precedence over any loaded later.
*
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
index 6186e44..602134a 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
@@ -22,10 +22,13 @@ package org.apache.bookkeeper.proto;
import static org.apache.bookkeeper.bookie.BookKeeperServerStats.BOOKIE_SCOPE;
import static org.apache.bookkeeper.bookie.BookKeeperServerStats.SERVER_SCOPE;
+import static
org.apache.bookkeeper.conf.AbstractConfiguration.PERMITTED_STARTUP_USERS;
import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.net.UnknownHostException;
+import java.security.AccessControlException;
+import java.util.Arrays;
import org.apache.bookkeeper.bookie.Bookie;
import org.apache.bookkeeper.bookie.BookieCriticalThread;
import org.apache.bookkeeper.bookie.BookieException;
@@ -77,6 +80,7 @@ public class BookieServer {
throws IOException, KeeperException, InterruptedException,
BookieException, UnavailableException, CompatibilityException,
SecurityException {
this.conf = conf;
+ validateUser(conf);
this.statsLogger = statsLogger;
this.nettyServer = new BookieNettyServer(this.conf, null);
try {
@@ -159,6 +163,28 @@ public class BookieServer {
running = false;
}
+ /**
+ * Ensure the current user can start-up the process if it's restricted.
+ */
+ private void validateUser(ServerConfiguration conf) throws
AccessControlException {
+ if (conf.containsKey(PERMITTED_STARTUP_USERS)) {
+ String currentUser = System.getProperty("user.name");
+ String[] propertyValue = conf.getPermittedStartupUsers();
+ for (String s : propertyValue) {
+ if (s.equals(currentUser)) {
+ return;
+ }
+ }
+ String errorMsg =
+ "System cannot start because current user isn't in
permittedStartupUsers."
+ + " Current user: " + currentUser + "
permittedStartupUsers: "
+ + Arrays.toString(propertyValue);
+ LOG.error(errorMsg);
+ throw new AccessControlException(errorMsg);
+ }
+ }
+
+
public boolean isRunning() {
return bookie.isRunning() && nettyServer.isRunning() && running;
}
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
index 412e20d..dbfb9ab 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
@@ -34,9 +34,11 @@ import java.io.IOException;
import java.io.OutputStreamWriter;
import java.net.BindException;
import java.net.InetAddress;
+import java.security.AccessControlException;
import java.util.ArrayList;
import java.util.List;
import java.util.Random;
+
import
org.apache.bookkeeper.bookie.BookieException.DiskPartitionDuplicationException;
import
org.apache.bookkeeper.bookie.LedgerDirsManager.NoWritableLedgerDirException;
import org.apache.bookkeeper.client.BookKeeper;
@@ -350,6 +352,105 @@ public class BookieInitializationTest extends
BookKeeperClusterTestCase {
}
/**
+ * Verify user cannot start if user is in permittedStartupUsers conf list
BKException BKUnauthorizedAccessException
+ * if cannot start.
+ */
+ @Test
+ public void testUserNotPermittedToStart() throws Exception {
+ File tmpDir = createTempDir("bookie", "test");
+
+ ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration();
+ int port = PortManager.nextFreePort();
+
conf.setZkServers(null).setBookiePort(port).setJournalDirName(tmpDir.getPath())
+ .setLedgerDirNames(new String[] { tmpDir.getPath() });
+ String userString = "larry, curly,moe,,";
+ conf.setPermittedStartupUsers(userString);
+ BookieServer bs1 = null;
+
+ boolean sawException = false;
+ try {
+ bs1 = new BookieServer(conf);
+ Assert.fail("Bookkeeper should not have started since current user
isn't in permittedStartupUsers");
+ } catch (AccessControlException buae) {
+ sawException = true;
+ } finally {
+ if (bs1 != null && bs1.isRunning()) {
+ bs1.shutdown();
+ }
+ }
+ assertTrue("Should have thrown exception", sawException);
+ }
+
+ /**
+ * Verify user cannot start if user is in permittedStartupUsers conf list
BKException BKUnauthorizedAccessException
+ * if cannot start.
+ */
+ @Test
+ public void testUserPermittedToStart() throws Exception {
+ File tmpDir = createTempDir("bookie", "test");
+
+ ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration();
+ int port = PortManager.nextFreePort();
+
conf.setZkServers(null).setBookiePort(port).setJournalDirName(tmpDir.getPath())
+ .setLedgerDirNames(new String[] { tmpDir.getPath() });
+
+ BookieServer bs1 = null;
+
+ // Multiple commas
+ String userString = "larry,,,curly ," +
System.getProperty("user.name") + " ,moe";
+ conf.setPermittedStartupUsers(userString);
+ try {
+ bs1 = new BookieServer(conf);
+ bs1.start();
+ } catch (AccessControlException buae) {
+ Assert.fail("Bookkeeper should have started since current user is
in permittedStartupUsers");
+ } finally {
+ if (bs1 != null && bs1.isRunning()) {
+ bs1.shutdown();
+ }
+ }
+
+ // Comma at end
+ userString = "larry ,curly, moe," + System.getProperty("user.name") +
",";
+ conf.setPermittedStartupUsers(userString);
+ try {
+ bs1 = new BookieServer(conf);
+ bs1.start();
+ } catch (AccessControlException buae) {
+ Assert.fail("Bookkeeper should have started since current user is
in permittedStartupUsers");
+ } finally {
+ if (bs1 != null && bs1.isRunning()) {
+ bs1.shutdown();
+ }
+ }
+ }
+
+ /**
+ * Verify user can start if user is not in permittedStartupUsers but it is
empty BKException
+ * BKUnauthorizedAccessException if cannot start.
+ */
+ @Test
+ public void testUserPermittedToStartWithMissingProperty() throws Exception
{
+ File tmpDir = createTempDir("bookie", "test");
+
+ ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration();
+ int port = PortManager.nextFreePort();
+
conf.setZkServers(null).setBookiePort(port).setJournalDirName(tmpDir.getPath())
+ .setLedgerDirNames(new String[] { tmpDir.getPath() });
+ BookieServer bs1 = null;
+ try {
+ bs1 = new BookieServer(conf);
+ bs1.start();
+ } catch (AccessControlException buae) {
+ Assert.fail("Bookkeeper should have started since
permittedStartupUser is not specified");
+ } finally {
+ if (bs1 != null && bs1.isRunning()) {
+ bs1.shutdown();
+ }
+ }
+ }
+
+ /**
* Verify duplicate bookie server startup. Should throw
* java.net.BindException if already BK server is running
*/
--
To stop receiving notification emails like this one, please contact
[email protected].
