BIND 10 #1044: SSL/TLS certificate for b10-cmdctl is expired

[BIND 10 Development]

#1044: SSL/TLS certificate for b10-cmdctl is expired
-------------------------------------+-------------------------------------
                   Reporter:  cas    |                 Owner:  jelte
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20121204
  medium                             |            Resolution:
                  Component:  cmd-   |             Sensitive:  0
  ctl                                |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  3.0
            Defect Severity:  High   |           Total Hours:  0
Feature Depending on Ticket:         |
  alpha2                             |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jelte


Comment:

 Hi Jelte

 Some of these are just nitpicking, so ignore them at your will. :)

 * Are the echo statements in `configure.ac` left in on purpose? If you
   put it in so that the buildbot returns it in our configure output,
   it's perhaps better to get `config.log` uploaded instead as that has
   much more information.

 * In `src/bin/cmdctl/Makefile.am`, do you think it's better to move just
   `bin_PROGRAMS` to the top of the file next to `pkglibexec_SCRIPTS` (so
   that we know immediately what are the main targets in this
   Makefile.am)?

 * Is a `CLEANFILES` necessary for `cmdctl-certfile.pem` and
   `cmdctl-keyfile.pem` (or does `make distcheck` pass even though they
   exist?)

 * Line printed by `usage()` goes over 80 columns and doesn't indent when
   wrapping.

 * Shall we set "O=Unknown (BIND10 sample)" instead of "O=BIND10" in the
   created cert, so that we are not assumed as its owner by mistake? We
   provide the software, but any certs belong to the site. For country
   code too, I think the `openssl` tool uses "C=XX" by default.

 * If this tool does validation too, would `b10-certtool` or
   `b10-certutil` be a better name than `b10-certgen`?

 * `fileExists()` should probably be split into two
   methods. `fileExists()` that uses `F_OK` and `fileIsReadable()` that
   uses `R_OK`, and we use `fileIsReadable()` in the validation
   case. With the current code, `fileExists()` will return false if the
   file exists and is not readable, and the write case will attempt
   creation. [The creation fails, but the function name doesn't match
   what it does, and `NO_SUCH_FILE` would not match the error too.]
   Also a weird file mode like 0200 will allow a write without `-w`.

 * There indeed does not seem to be any way to convert a `X509_Code` into
   a string in Botan. Perhaps you can make a patch for Botan with these
   strings and send it upstream. :)

 * In `createKeyAndCertificate()`, can we print the "Creating key file"
   message before `RSA_PrivateKey key` is defined? The key creation may
   take time and the user gets a message first this way.

 * Just like the key_file, it may be better to test the certificate_file
   `std::ofstream` for goodness after writing to it, or in both cases,
   test before and after.

 * Is "SHA-256" well-supported by all browsers including slightly older
   versions of IE? If not, maybe we should use SHA-1. Many certificate
   authorities do not allow issue with hashalg other than SHA-1. On my
   machine I have botan-1.8.14, which is defaulting to SHA-1.

 * The way we overload Botan's `X509_Code` with our own extra values can
   be problematic. The two should be separate I think. In case Botan adds
   more values in future versions, it may clash with our values. In the
   case of unittests, maybe we can regex-match the output in case we want
   to check for Botan errors.

 * The `!quiet` is probably not needed here:
 {{{
             if (result != 0 && !quiet_) {
                     print("Running with -w would overwrite the
 certificate");
             }
 }}}

 * The xor operator can be used for this:

 {{{
     if ((create_cert && certfile_default && !keyfile_default) ||
     (create_cert && !certfile_default && keyfile_default)) {
 }}}

 replaced by:

 {{{
     if (create_cert && (certfile_default ^ keyfile_default)) {
 }}}

 * There's a minor typo in the manpage:
 {{{
 -            private key and certificate are created. If it is does exist,
 +            private key and certificate are created. If it does exist,
 }}}

 * This is also incorrect (`b10-certgen` is not a daemon):
 {{{
 +      The <command>b10-certgen</command> daemon was first implemented
 +      in November 2012 for the ISC BIND 10 project.
 }}}

 Two other nitpicks:

 * We currently overwrite any existing keyfile if -w is specified, -f is
   not specified and the key file exists. Maybe this is OK.

 * We should distinguish between CA-signed certificates and self-signed
   certificates when the tool prints "foo.crt is valid". A way to do it
   is first try to verify the certfile without adding the certfile as a
   trusted cert. If it fails, we `store.add_trusted_certs()` then, but
   print a different method saying "foo.crt is valid, but
   self-signed". This is a point about extending the tool further I
   suppose, so maybe you can ignore it. It's not needed for a basic tool
   that just generates a certificate during installation.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1044#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development