Opened 9 years ago

Closed 7 years ago

#1044 closed defect (complete)

SSL/TLS certificate for b10-cmdctl is expired

Reported by: cas Owned by: jelte
Priority: medium Milestone: Sprint-20121204
Component: ~cmd-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: High
Sub-Project: Core Feature Depending on Ticket: alpha2
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Version: bind10-devel-20110519:

the SSL/TLS certificate for b10-cmdctl in bind10-devel-20110519 is expired.

would be good to create a new certificate using OpenSSL during 'make install'.

Or provide a tool for that (similar to 'unbound-control-setup')

Subtickets

Change History (23)

comment:1 Changed 9 years ago by shane

  • Defect Severity changed from N/A to Medium
  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:2 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 3

comment:3 follow-up: Changed 8 years ago by shane

  • Sub-Project changed from DNS to Core

I think the best way is to provide a utility as proposed, since certificates will expire over time and need to be refreshed by the administrator.

We also should check the certificate expiration when we use it!

comment:4 Changed 7 years ago by shane

  • Feature Depending on Ticket set to alpha2
  • Milestone set to Next-Sprint-Proposed

comment:6 Changed 7 years ago by jelte

  • Defect Severity changed from Medium to High
  • Milestone changed from Next-Sprint-Proposed to Sprint-20121120

comment:7 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:8 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

and done!

src/bin/cmdctl/ is now the proud owner of a new small tool b10-certgen to create and update the self-signed certificate. Please see the manpage for an explanation.

(in short; by default it will check the cmdctl-certfile.pem in the current working directory, and print whether it is valid. with -w one can update it)

I removed the certificate and key we provided, and it is now generated when 'make' is done. The tool is installed so the administrator can update the installed one at his or her leisure.

Possible future enhancements: allow user to set properties like organization and common name, and/or print the content the certificate (currently the properties are hardcoded and it will only tell whether the cert is valid or not).

comment:9 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks

Picking.

comment:10 follow-ups: Changed 7 years ago by muks

  • Owner changed from muks to jelte

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.

comment:11 in reply to: ↑ 3 Changed 7 years ago by muks

Replying to shane:

We also should check the certificate expiration when we use it!

Is this already implemented?

comment:12 in reply to: ↑ 10 Changed 7 years ago by muks

Replying to muks:

Also a weird file mode like 0200 will allow a write without -w.

That should be: .. allow a write without -f.

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.

s/print a different method/print a different message/.

:)

comment:13 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to muks

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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jelte

Hi Jelte

Apologies for the delay in responding as I had the day off on Friday.

Replying to jelte:

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

If Botan adds a single additional enumeration constant = 100 into that enum, it will not break Botan's API or ABI, but it will break our assumption upon which this code is written. I merely want to point this out in the review, that's all. :)

The changes look good, but they are a set of changes grouped together in a single commit with a log message "Fix review comments". :) It seems that one unrelated change in perfdhcp was included by mistake as well. I suggest breaking these into individual commits for each thing they address.

What about Shane's comment about checking certificate expiry before using it (comment 11)? Does the code already check the expiry time in the certificate?

comment:15 in reply to: ↑ 14 Changed 7 years ago by jelte

  • Owner changed from jelte to muks

If Botan adds a single additional enumeration constant = 100 into that enum, it will not break Botan's API or ABI, but it will break our assumption upon which this code is written. I merely want to point this out in the review, that's all. :)

Yes, I know, but I do not expect that to be much of a problem (or rather, I think not redefining them all to 1 exit code is more useful than this potential problem)

The changes look good, but they are a set of changes grouped together in a single commit with a log message "Fix review comments". :) It seems that one unrelated change in perfdhcp was included by mistake as well. I suggest breaking these into individual commits for each thing they address.

I've been trying to get the number of commits down after the squash discussion. Often, review changes are a number of small changes that IMO don't really need a separate commit each. I may have gone too far with this one (like the additional file permission checks), but hey I did point to the comment here :)

The perfdhcp change should not have been committed, which is my bad. Reverted that (I'll push that as a separate commit straight to master). But rather than splitting up an existing commit I propose I leave it as is for now and try to be better in grouping review changes next time ;)

What about Shane's comment about checking certificate expiry before using it (comment 11)? Does the code already check the expiry time in the certificate?

No. And unfortunately, there is no easy way to actually do this AFAICT. The python libraries don't expose this level of use, and I can't really find any builtin way to do it. So we'd either need to

  • add scary threading stuff and make cmdctl connect to itself to validate
  • call b10-certgen from the cmdctl code
  • move the checking code from b10-certgen to our cryptowrappers, and make python wrappers for them, then call those from cmdctl
  • make a hard dependency on an external tool (like openssl req)

I have no idea which would be the right approach, and if I did I think we should probably defer that to a new ticket.

comment:16 Changed 7 years ago by shane

The Python SSL library should let us check the valid times for a certificate:

http://docs.python.org/3/library/ssl.html#ssl.cert_time_to_seconds

It is possibly out of scope for this ticket, but possibly not. If we want to do this on a separate ticket I'll create that!

comment:17 Changed 7 years ago by muks

Go ahead and merge. :) I think we should have a ChangeLog entry for it.

comment:18 Changed 7 years ago by muks

  • Owner changed from muks to jelte

comment:19 Changed 7 years ago by jinmei

Quick question: is it okay to directly depend on Botan?

comment:20 Changed 7 years ago by jinmei

I'm also afraid there was portability issues with getopt_long().
I forgot details, but IIRC that's why we don't use it anywhere else.

comment:21 Changed 7 years ago by jelte

oh i hadn't seen those last comments, and I just merged it.

If it turns out to be a problem I guess we'll have to revert.

not having longopt would be a shame btw

comment:22 Changed 7 years ago by muks

I checked BSDs during the review and they seemed to have the long variant (though I remember it being just a GNU thing at one time in the past). I just checked support in Solaris and Mac OS (just web search) and they also seem to have it. So unless our builders are failing (and they're not too old), we may be able to keep the code.

comment:23 Changed 7 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

Yeah, solaris supports it 'to be compatible', and I did need to fix a few consts due to the structure being different there, but it appears to work, so I'm closing the ticket.

We can consider moving the specific Botan calls to libcryptolink to remove the direct dependencies, but that'll have to be in a new ticket then :)

Note: See TracTickets for help on using tickets.