Opened 10 years ago

Closed 9 years ago

#127 closed enhancement (complete)

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

Reported by: zhanglikun Owned by: zhanglikun
Priority: low Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

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.

Subtickets

Change History (20)

comment:1 Changed 10 years ago by shane

  • Component changed from Unclassified to bind-ctl
  • Milestone changed from 02. Running, functional authoritative-only server to 04. 2nd Incremental Release
  • Priority changed from major to minor
  • Status changed from new to assigned
  • Type changed from task to enhancement

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.

comment:2 Changed 10 years ago by 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.

comment:3 Changed 10 years ago by jreed

Any update on this?

comment:4 Changed 10 years ago by 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.

comment:5 Changed 10 years ago by zhanglikun

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

comment:6 Changed 10 years ago by shane

  • Milestone changed from 04. 2nd Incremental Release: Early Adopters to 05. 3rd Incremental Release

comment:7 Changed 9 years ago by zhanglikun

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

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

comment:8 follow-up: Changed 9 years ago by zhanglikun

Add a new revision r2254.

comment:9 in reply to: ↑ 8 Changed 9 years ago by zhanglikun

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

comment:10 Changed 9 years ago by 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.

comment:11 Changed 9 years ago by 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

comment:12 Changed 9 years ago by zhanglikun

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

comment:13 follow-up: Changed 9 years ago by 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

comment:14 in reply to: ↑ 13 Changed 9 years ago by 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.

comment:15 Changed 9 years ago by jelte

  • Owner changed from jelte to zhanglikun

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

comment:16 Changed 9 years ago by zhanglikun

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

comment:17 Changed 9 years ago by shane

  • Owner changed from zhanglikun to jelte

comment:18 Changed 9 years ago by jelte

  • Owner changed from jelte to zhanglikun

Yes, make distcheck now works. Ship it :)

comment:19 Changed 9 years ago by zhanglikun

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

comment:20 Changed 9 years ago by shane

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

Closing, thanks Likun.

Note: See TracTickets for help on using tickets.