Browse Source

[1044] Address review comments

See http://bind10.isc.org/ticket/1044#comment:13
Jelte Jansen 12 years ago
parent
commit
ebe0d8a602

+ 0 - 8
configure.ac

@@ -663,12 +663,6 @@ if test "x${BOTAN_CONFIG}" != "x"
 then
     BOTAN_LIBS=`${BOTAN_CONFIG} --libs`
     BOTAN_INCLUDES=`${BOTAN_CONFIG} --cflags`
-    echo "XXX"
-    echo "XXX ${BOTAN_CONFIG}"
-    echo "XXX libs: ${BOTAN_LIBS}"
-    echo "XXX includes: ${BOTAN_INCLUDES}"
-    echo "XXX"
-    echo "XXX"
 
     # We expect botan-config --libs to contain -L<path_to_libbotan>, but
     # this is not always the case.  As a heuristics workaround we add
@@ -711,13 +705,11 @@ fi
 AC_SUBST(BOTAN_LDFLAGS)
 AC_SUBST(BOTAN_LIBS)
 AC_SUBST(BOTAN_INCLUDES)
-echo "XXX substed: ${BOTAN_INCLUDES}"
 # Even though chances are high we already performed a real compilation check
 # in the search for the right (pkg)config data, we try again here, to
 # be sure.
 CPPFLAGS_SAVED=$CPPFLAGS
 CPPFLAGS="$BOTAN_INCLUDES $CPPFLAGS"
-echo "XXX CPPFLAGS: ${CPPFLAGS}"
 LIBS_SAVED="$LIBS"
 LIBS="$LIBS $BOTAN_LIBS"
 AC_CHECK_HEADERS([botan/botan.h],,AC_MSG_ERROR([Missing required header files.]))

+ 5 - 4
src/bin/cmdctl/Makefile.am

@@ -4,6 +4,8 @@ pkglibexecdir = $(libexecdir)/@PACKAGE@
 
 pkglibexec_SCRIPTS = b10-cmdctl
 
+bin_PROGRAMS = b10-certgen
+
 nodist_pylogmessage_PYTHON = $(PYTHON_LOGMSGPKG_DIR)/work/cmdctl_messages.py
 pylogmessagedir = $(pyexecdir)/isc/log_messages/
 
@@ -26,7 +28,7 @@ CLEANFILES += $(PYTHON_LOGMSGPKG_DIR)/work/cmdctl_messages.py
 CLEANFILES += $(PYTHON_LOGMSGPKG_DIR)/work/cmdctl_messages.pyc
 
 man_MANS = b10-cmdctl.8 b10-certgen.1
-DISTCLEANFILES = $(man_MANS)
+DISTCLEANFILES = $(man_MANS) cmdctl-certfile.pem cmdctl-keyfile.pem
 EXTRA_DIST += $(man_MANS) b10-certgen.xml b10-cmdctl.xml cmdctl_messages.mes
 
 if GENERATE_DOCS
@@ -57,17 +59,16 @@ b10-cmdctl: cmdctl.py $(PYTHON_LOGMSGPKG_DIR)/work/cmdctl_messages.py
 	$(SED) "s|@@PYTHONPATH@@|@pyexecdir@|" cmdctl.py >$@
 	chmod a+x $@
 
-bin_PROGRAMS = b10-certgen
 b10_certgen_SOURCES = b10-certgen.cc
 b10_certgen_CXXFLAGS = $(BOTAN_INCLUDES)
 b10_certgen_LDFLAGS = $(BOTAN_LIBS)
 
 # Generate the initial certificates immediately
 cmdctl-certfile.pem: b10-certgen
-	./b10-certgen -w
+	./b10-certgen -q -w
 
 cmdctl-keyfile.pem: b10-certgen
-	./b10-certgen -w
+	./b10-certgen -q -w
 
 if INSTALL_CONFIGURATIONS
 

+ 77 - 11
src/bin/cmdctl/b10-certgen.cc

@@ -51,6 +51,7 @@ const int READ_ERROR = 102;
 const int WRITE_ERROR = 103;
 const int UNKNOWN_ERROR = 104;
 const int NO_SUCH_FILE = 105;
+const int FILE_PERMISSION_ERROR = 106;
 
 void
 usage() {
@@ -66,21 +67,40 @@ usage() {
     std::cout << "-h, --help\t\t\tshow this help" << std::endl;
     std::cout << "-k, --keyfile=FILE\t\tfile to store the generated private key"
               << std::endl;
-    std::cout << "-w, --write\t\t\tcreate a new certificate if the given file "
-                 "does not exist, or if is is not valid" << std::endl;
+    std::cout << "-w, --write\t\t\tcreate a new certificate if the given file"
+              << std::endl << "\t\t\t\tdoes not exist, or if is is not valid"
+              << std::endl;
     std::cout << "-q, --quiet\t\t\tprint no output when creating or validating"
               << std::endl;
 }
 
+/// \brief Returns true if the given file exists
+///
+/// \param filename The file to check
+/// \return true if file exists
+bool
+fileExists(const std::string& filename) {
+    return (access(filename.c_str(), F_OK) == 0);
+}
+
 /// \brief Returns true if the given file exists and is readable
 ///
 /// \param filename The file to check
 /// \return true if file exists and is readable
 bool
-fileExists(const std::string& filename) {
+fileIsReadable(const std::string& filename) {
     return (access(filename.c_str(), R_OK) == 0);
 }
 
+/// \brief Returns true if the given file exists and is writable
+///
+/// \param filename The file to check
+/// \return true if file exists and is writable
+bool
+fileIsWritable(const std::string& filename) {
+    return (access(filename.c_str(), W_OK) == 0);
+}
+
 /// \brief Helper function for readable error output;
 ///
 /// Returns string representation of X509 result code
@@ -146,15 +166,19 @@ public:
             AutoSeeded_RNG rng;
 
             // Create and store a private key
-            RSA_PrivateKey key(rng, 2048);
-
             print("Creating key file " + key_file_name);
+            RSA_PrivateKey key(rng, 2048);
             std::ofstream key_file(key_file_name.c_str());
+            if (!key_file.good()) {
+                print(std::string("Error writing to ") + key_file_name +
+                      ": " + strerror(errno));
+                return (WRITE_ERROR);
+            }
             key_file << PKCS8::PEM_encode(key, rng, "");
             if (!key_file.good()) {
                 print(std::string("Error writing to ") + key_file_name +
                       ": " + strerror(errno));
-                return WRITE_ERROR;
+                return (WRITE_ERROR);
             }
             key_file.close();
 
@@ -163,8 +187,8 @@ public:
             // settable.
             X509_Cert_Options opts;
             opts.common_name = "localhost";
-            opts.organization = "BIND10";
-            opts.country = "US";
+            opts.organization = "UNKNOWN";
+            opts.country = "XX";
 
             opts.CA_key();
 
@@ -187,6 +211,11 @@ public:
                 return (WRITE_ERROR);
             }
             cert_file << cert.PEM_encode();
+            if (!cert_file.good()) {
+                print(std::string("Error writing to ") + cert_file_name +
+                      ": " + strerror(errno));
+                return (WRITE_ERROR);
+            }
             cert_file.close();
         } catch(std::exception& e) {
             std::cout << "Error creating key or certificate: " << e.what()
@@ -241,6 +270,40 @@ public:
         if (create_cert) {
             // Unless force is given, only create it if the current
             // one is not OK
+
+            // First do some basic permission checks; both files
+            // should either not exist, or be both readable
+            // and writable
+            // The checks are done one by one so all errors can
+            // be enumerated in one go
+            if (fileExists(certfile)) {
+                if (!fileIsReadable(certfile)) {
+                    print(certfile + " not readable: " + strerror(errno));
+                    create_cert = false;
+                }
+                if (!fileIsWritable(certfile)) {
+                    print(certfile + " not writable: " + strerror(errno));
+                    create_cert = false;
+                }
+            }
+            // The key file really only needs write permissions (for
+            // b10-certgen that is)
+            if (fileExists(keyfile)) {
+                if (!fileIsWritable(keyfile)) {
+                    print(keyfile + " not writable: " + strerror(errno));
+                    create_cert = false;
+                }
+            }
+            if (!create_cert) {
+                print("Not creating new certificate, "
+                      "check file permissions");
+                return (FILE_PERMISSION_ERROR);
+            }
+
+            // If we reach this, we know that if they exist, we can both
+            // read and write them, so now it's up to content checking
+            // and/or force_create
+
             if (force_create || !fileExists(certfile) ||
                 validateCertificate(certfile) != VERIFIED) {
                 return (createKeyAndCertificate(keyfile, certfile));
@@ -252,8 +315,12 @@ public:
                 print(certfile + ": " + strerror(errno));
                 return (NO_SUCH_FILE);
             }
+            if (!fileIsReadable(certfile)) {
+                print(certfile + " not readable: " + strerror(errno));
+                return (FILE_PERMISSION_ERROR);
+            }
             int result = validateCertificate(certfile);
-            if (result != 0 && !quiet_) {
+            if (result != 0) {
                 print("Running with -w would overwrite the certificate");
             }
             return (result);
@@ -341,8 +408,7 @@ main(int argc, char* argv[])
     }
 
     // Some sanity checks on option combinations
-    if ((create_cert && certfile_default && !keyfile_default) ||
-        (create_cert && !certfile_default && keyfile_default)) {
+    if (create_cert && (certfile_default ^ keyfile_default)) {
         std::cout << "Error: keyfile and certfile must both be specified "
                      "if one of them is when calling b10-certgen in write "
                      "mode." << std::endl;

+ 3 - 3
src/bin/cmdctl/b10-certgen.xml

@@ -143,8 +143,8 @@
         <listitem>
           <para>
             Check the given certificate file. If it does not exist, a new
-            private key and certificate are created. If it is does exist,
-            the certificate is validated. If it is not valid (for instance
+            private key and certificate are created. If it does exist, the
+            certificate is validated. If it is not valid (for instance
             because it has expired), it is overwritten with a newly created
             certificate. If it is valid, nothing happens (use
             <option>-f</option> to force an update in that case).
@@ -179,7 +179,7 @@
   <refsect1>
     <title>HISTORY</title>
     <para>
-      The <command>b10-certgen</command> daemon was first implemented
+      The <command>b10-certgen</command> tool was first implemented
       in November 2012 for the ISC BIND 10 project.
     </para>
   </refsect1>

+ 3 - 0
src/bin/cmdctl/tests/Makefile.am

@@ -1,6 +1,9 @@
 PYCOVERAGE_RUN=@PYCOVERAGE_RUN@
 PYTESTS = cmdctl_test.py b10-certgen_test.py
 EXTRA_DIST = $(PYTESTS)
+EXTRA_DIST += testdata/expired-certfile.pem
+EXTRA_DIST += testdata/mangled-certfile.pem
+EXTRA_DIST += testdata/noca-certfile.pem
 
 # If necessary (rare cases), explicitly specify paths to dynamic libraries
 # required by loadable python modules.

+ 87 - 3
src/bin/cmdctl/tests/b10-certgen_test.py

@@ -22,6 +22,7 @@ import os
 from subprocess import call
 import subprocess
 import ssl
+import stat
 
 def run(command):
     """
@@ -49,6 +50,39 @@ class FileDeleterContext:
             if os.path.exists(f):
                 os.unlink(f)
 
+class FilePermissionContext:
+    """
+    Simple Context Manager that temporarily modifies file permissions for
+    a given file
+    """
+    def __init__(self, f, unset_flags = [], set_flags = []):
+        """
+        Initialize file permission context.
+        See the stat module for possible flags to set or unset.
+        The flags are changed when the context is entered (i.e.
+        you can create the context first without any change)
+        The flags are changed back when the context is left.
+
+        Parameters:
+        f: string, file to change permissions for
+        unset_flags: list of flags to unset
+        set_flags: list of flags to set
+        """
+        self.file = f
+        self.orig_mode = os.stat(f).st_mode
+        new_mode = self.orig_mode
+        for flag in unset_flags:
+            new_mode = new_mode & ~flag
+        for flag in set_flags:
+            new_mode = new_mode | flag
+        self.new_mode = new_mode
+
+    def __enter__(self):
+        os.chmod(self.file, self.new_mode)
+
+    def __exit__(self, type, value, traceback):
+        os.chmod(self.file, self.orig_mode)
+
 def read_file_data(filename):
     """
     Simple text file reader that returns its contents as an array
@@ -138,9 +172,13 @@ class TestCertGenTool(unittest.TestCase):
         """
         Tests a few pre-created certificates with the -c option
         """
-        self.validate_certificate(10, 'testdata/expired-certfile.pem')
-        self.validate_certificate(100, 'testdata/mangled-certfile.pem')
-        self.validate_certificate(17, 'testdata/noca-certfile.pem')
+        if ('CMDCTL_SRC_PATH' in os.environ):
+            path = os.environ['CMDCTL_SRC_PATH'] + "/tests/testdata/"
+        else:
+            path = "testdata/"
+        self.validate_certificate(10, path + 'expired-certfile.pem')
+        self.validate_certificate(100, path + 'mangled-certfile.pem')
+        self.validate_certificate(17, path + 'noca-certfile.pem')
 
     def test_bad_options(self):
         """
@@ -165,6 +203,52 @@ class TestCertGenTool(unittest.TestCase):
         # No such file
         self.run_check(105, None, None, [self.TOOL, '-c', 'foo'])
 
+    def test_permissions(self):
+        """
+        Test some combinations of correct and bad permissions.
+        """
+        keyfile = 'mod-keyfile.pem'
+        certfile = 'mod-certfile.pem'
+        command = [ self.TOOL, '-q', '-w', '-c', certfile, '-k', keyfile ]
+        # Delete them at the end
+        with FileDeleterContext([keyfile, certfile]):
+            # Create the two files first
+            self.run_check(0, '', '', command)
+            self.validate_certificate(0, certfile)
+
+            # Make the key file unwritable
+            with FilePermissionContext(keyfile, unset_flags = [stat.S_IWUSR]):
+                self.run_check(106, '', '', command)
+                # Should have no effect on validation
+                self.validate_certificate(0, certfile)
+
+            # Make the cert file unwritable
+            with FilePermissionContext(certfile, unset_flags = [stat.S_IWUSR]):
+                self.run_check(106, '', '', command)
+                # Should have no effect on validation
+                self.validate_certificate(0, certfile)
+
+            # Make the key file unreadable (this should not matter)
+            with FilePermissionContext(keyfile, unset_flags = [stat.S_IRUSR]):
+                self.run_check(0, '', '', command)
+
+                # unreadable key file should also not have any effect on
+                # validation
+                self.validate_certificate(0, certfile)
+
+            # Make the cert file unreadable (this should matter)
+            with FilePermissionContext(certfile, unset_flags = [stat.S_IRUSR]):
+                self.run_check(106, '', '', command)
+
+                # Unreadable cert file should also fail validation
+                self.validate_certificate(106, certfile)
+
+        # Not directly a permission problem, but trying to check or create
+        # in a nonexistent directory returns different error codes
+        self.validate_certificate(105, 'fakedir/cert')
+        self.run_check(103, '', '', [ self.TOOL, '-q', '-w', '-c',
+                                      'fakedir/cert', '-k', 'fakedir/key' ])
+
 if __name__== '__main__':
     unittest.main()
 

+ 2 - 2
tests/tools/perfdhcp/tests/test_control_unittest.cc

@@ -896,7 +896,7 @@ TEST_F(TestControlTest, Packet6) {
     }
 }
 
-TEST_F(TestControlTest, Packet4Exchange) {
+TEST_F(TestControlTest, DISABLED_Packet4Exchange) {
     // Get the local loopback interface to open socket on
     // it and test packets exchanges. We don't want to fail
     // the test if interface is not available.
@@ -939,7 +939,7 @@ TEST_F(TestControlTest, Packet4Exchange) {
     EXPECT_EQ(12, iterations_performed);
 }
 
-TEST_F(TestControlTest, Packet6Exchange) {
+TEST_F(TestControlTest, DISABLED_Packet6Exchange) {
     // Get the local loopback interface to open socket on
     // it and test packets exchanges. We don't want to fail
     // the test if interface is not available.