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:  muks
                       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 jelte):

 * owner:  jelte => muks


Comment:

 thanks for the review :)

 >
 > * 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.
 >

 oops, no those should've been kept. Removed them.

 > * 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)?
 >

 sure, done

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

 there were more problems :)

 I'd say they should be DISTCLEANFILES, but yeah.

 another problems were that the test data files weren't added and were
 addressed wrongly :) All three fixed now

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

 fixed

 > * 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.
 >

 sure, done

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

 i'd agree if it did more, but the validation really is mostly to decide
 whether to generate, so IMO certgen is fine.

 > * `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`.
 >

 Ack. I've also added an isWritable(). The checking code then got a bit
 hairy (lots of different scenarios), so I've changed it a bit, in the case
 of creation permission checks are done before even seeing if it needs
 validation.

 Also added tests for permissions

 > * 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. :)
 >

 yeah, will consider it if i find time :)

 > * 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.
 >

 ack, done

 > * 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.
 >

 added

 > * 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.
 >

 Personally, I think SHA-1 should be phased out as soon as possible
 (oh hey another future addition, provide which algorithm to use :) of
 course it depends on the lib, and I don't think 1.8 even supports sha2 for
 self-signed generation)

 For browsers it shouldn't really matter, since bindctl is the client here.

 > * 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.
 >

 I don't think I follow. Do you mean the exit codes we add? I don't think
 the addition would be a problem (i don't really see botan adding more than
 99 error codes); depending on the exit code in external scripts might,
 since we can't be sure botan doesn't *change* them. But in that case the
 best solution would probably be to exit with one fixed number for every
 non-zero botan code.

 Or are you suggesting to pick a subset of botan codes and redefine values
 for those as our exit codes?

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

 ack :) removed

 > * 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)) {
 > }}}
 >

 ah doh, of course :)

 > * 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,
 > }}}
 >

 ack, fixed

 > * 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.
 > }}}
 >

 damn you copy-paste gods! fixed :)

 > 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.
 >

 yeah there was a point where i stopped caring about different potential
 scenarios :) IMO this one shouldn't be much of a problem (especially with
 the added permisson checks, so at least it won't start overwriting until
 it is reasonably sure it can also write the new cert)

 > * 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.

 I kept the focus on self-signed certificates. If and when we do, we can
 extend this. One problem is that we'll need to manage real CA certificates
 in that case (or provide another option). This would be a good approach
 then though :)

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