Bindctl: remove client-side certificate && using digest authentication

[zhanglikun]

Create this new ticket for the following feature.

1. Digest authentication will be used for the username/password sent to cmdctl, just like you suggested.
2. Remove client-side PEM certificate (bindctl.pem), when bindctl starts up, it will ask certificate from cmdctl.
3. cmdctl.spec will be used by cmdctl, cmdctl will load certifcate/account file from the path specified by cmdctl.spec.

[shane]

Had a quick chat with Likun, and he says that this code is done but not committed. We think it can get done for the next release, so I'm moving it to that.

[jreed]

Also there was the idea of having the client use a specific certificate to authenticate to the cmdctl server to identify itself for known privileges. Any thoughts on this? Will we still consider that?

As for your current code, I'd like to see it in a branch since I am also working on make target and/or documentation and/or shell script to generate key/certificate per task 105.

[jreed]

Any update on this?

[zhanglikun]

Sorry for delay response.
Currently I don't think we need think on it(client certificate validation), since it works with the same way with server certificate validation. Once we have fixed the latter one, it's easy to support client certificate validation.(I think), any suggestion?
I rechecked the code sent to me by somebody yesterday(Which made me think it has been done), I found it didn't implement as what I expected. the code did like this: let bindctl first connect with cmdctl over msgq, to get the path of server certificate file. What I expected is: let bindctl to get server certificate over normal tcp connection.

[zhanglikun]

so I wish the code can be committed in first several days after coming bind10 release.

[zhanglikun]

The code for this ticket is finished now, it's ready for reviw. There are three revisions: r2167, r2168, and r2253. Left the review task to Jelte, :).

[zhanglikun]

Add a new revision r2254.

[zhanglikun]

Replying to zhanglikun:
Add a new revision r2254 and r2262.

[jreed]

Last Changed Rev: 2263

make check fails with:

  File "/backup/isc-work-svn/svn/branches/trac127/src/lib/python/isc/config/module_spec.py", line 45, in module_spec_from_file
    file = open(spec_file)
IOError: [Errno 2] No such file or directory: '/home/reed/opt/bind10-testing/share/bind10-devel/cmdctl.spec'

(multiple failures like this.)

Problem is unrelated to this ticket I think. But this shouldn't rely on uninstalled file for testing.

[jreed]

Also a make check failure and an error:

Making check in cmdctl
Making check in tests
make  check-local
for pytest in cmdctl_test.py ; do  echo Running test: $pytest ;  env PYTHONPATH=/backup/isc-work-svn/svn/branches/trac127/src/lib/python:/backup/isc-work-svn/svn/branches/trac127/src/lib/python:/backup/isc-work-svn/svn/branches/trac127/src/bin/cmdctl  /usr/pkg/bin/python3.1 /backup/isc-work-svn/svn/branches/trac127/src/bin/cmdctl/tests/$pytest ;  done
Running test: cmdctl_test.py
.F........................E
======================================================================
ERROR: test_wrap_sock_in_ssl_context (__main__.TestSecureHTTPServer)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/backup/isc-work-svn/svn/branches/trac127/src/bin/cmdctl/tests/cmdctl_test.py", line 394, in test_wrap_sock_in_ssl_context
    '../cmdctl-certfile')
  File "/backup/isc-work-svn/svn/branches/trac127/src/bin/cmdctl/cmdctl.py", line 492, in _wrap_socket_in_ssl_context
    self._check_key_and_cert(key, cert)
  File "/backup/isc-work-svn/svn/branches/trac127/src/bin/cmdctl/cmdctl.py", line 485, in _check_key_and_cert
    raise socket.error
socket.error

======================================================================
FAIL: test_command_handler (__main__.TestCommandControl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/backup/isc-work-svn/svn/branches/trac127/src/bin/cmdctl/tests/cmdctl_test.py", line 332, in test_command_handler
    self._check_answer(answer, 0, None)
  File "/backup/isc-work-svn/svn/branches/trac127/src/bin/cmdctl/tests/cmdctl_test.py", line 325, in _check_answer
    self.assertEqual(msg, msg_)
AssertionError: {'accounts_file': '/home/reed/opt/bind10-testing/etc/bind10-devel/cmdctl-accounts.csv', 'key_file': '/home/reed/opt/bind10-testing/etc/bind10-devel/cmdctl-keyfile.pem', 'cert_file': '/home/reed/opt/bind10-testing/etc/bind10-devel/cmdctl-certfile.pem'} != None

----------------------------------------------------------------------
Ran 27 tests in 0.059s

FAILED (failures=1, errors=1)
*** Error code 1

Stop.
make: stopped in /backup/isc-work-svn/svn/branches/trac127/src/bin/cmdctl/tests
*** Error code 1

[zhanglikun]

Thanks for your finding, I have committed the code in r2278, please test again.

[jelte]

make distcheck fails for me too:

======================================================================
ERROR: test_check_config_handler (__main__.TestCommandControl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jelte/repos/bind10/branches/trac127/bind10-devel-20100602/_build/../src/bin/cmdctl/tests/cmdctl_test.py", line 294, in setUp
    self.cmdctl = MyCommandControl(None, True)
  File "/home/jelte/repos/bind10/branches/trac127/bind10-devel-20100602/_build/src/bin/cmdctl/cmdctl.py", line 214, in __init__
    self._setup_session()
  File "/home/jelte/repos/bind10/branches/trac127/bind10-devel-20100602/_build/../src/bin/cmdctl/tests/cmdctl_test.py", line 282, in _setup_session
    module_spec = isc.config.module_spec_from_file("../cmdctl.spec.pre.in")
  File "/home/jelte/repos/bind10/branches/trac127/bind10-devel-20100602/src/lib/python/isc/config/module_spec.py", line 45, in module_spec_from_file
    file = open(spec_file)
IOError: [Errno 2] No such file or directory: '../cmdctl.spec.pre.in'

Changing that hardcoded cmdctl.spec.pre.in to cmdctl.spec fixes it
(after that i get the same errors as Jeremy)

bindcmd.py
111: Parse commands inputted from user and send them to cmdctl.

inputted sounds weird, make it simply "parse commands from user and"

cmdctl.py.in:

54: you could get that module name from the specfile (or better, through the api, i think its moduleccsession.get_module_spec().get_module_name() or something similar). That way, should we change it we only have to update the spec file :) (its also hardcoded in cfgmgr, we need to find a more general solution on that side)

209: that docstring seems wrong, talks about shutdown in the init function?

_setup_session(self): hmz. I think we should consider making msgq/cc so that we don't need two sessions, possible 'future work'? :)

482: that todo seems useful to, eh, do :) you haven't found a method for this yet?

485: i think that socket.error is the wrong exception to raise here (also at 489)

518: shouldn't we use constants for these? (another one on line 530)

Tests:

i see a lot of lines like this one:

self.assertTrue(ret == False)

You can also simply use self.assertFalse(ret) :)

Rest of the changes look good

[zhanglikun]

Replying to jelte:

make distcheck fails for me too:

Changing that hardcoded cmdctl.spec.pre.in to cmdctl.spec fixes it

(after that i get the same errors as Jeremy)>
bindcmd.py
111: Parse commands inputted from user and send them to cmdctl.>
inputted sounds weird, make it simply "parse commands from user and"

cmdctl.py.in:>
54: you could get that module name from the specfile (or better, through the api, i think its moduleccsession.get_module_spec().get_module_name() or something similar). That way, should we change it we only have to update the spec file :) (its also hardcoded in cfgmgr, we need to find a more general solution on that side)

209: that docstring seems wrong, talks about shutdown in the init function?

485: i think that socket.error is the wrong exception to raise here (also at 489)

518: shouldn't we use constants for these? (another one on line 530)

Tests:

i see a lot of lines like this one:

self.assertTrue(ret == False)

You can also simply use self.assertFalse(ret) :)

All above items have been fixed in r2305, please check.

_setup_session(self): hmz. I think we should consider making msgq/cc so that we >don't need two sessions, possible 'future work'? :)

I prefer to two sessions or left it as 'future work', since I want to receive the commands from other modules in block mode.(have added it to TODO list)

482: that todo seems useful to, eh, do :) you haven't found a method for this yet?

sorry, still haven't, I have added it to TODO list.

[jelte]

Almost, make distcheck still fails, i think CMDCTL_SPEC_PATH in src/bin/cmdctl/tests/Makefile.am should use builddir instead of srcdir (cmdctl.spec is generated and does not appear n the source dir but the build dir).

However, just that doesn't work, since some of the file paths it's used in do live in the source dir. Perhaps we need two variables for this (though a quick attempt to split them here failed).

Rest of the changes are good. Nearly there :)

[zhanglikun]

yes, please check r2321, "abs_top_builddir" is used to get file cmdctl.spec, thanks for your hint.

[jelte]

Yes, make distcheck now works. Ship it :)

[zhanglikun]

Code has been committed to trunk in r2357, so close this ticket.

[shane]

Closing, thanks Likun.