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.
- Digest authentication will be used for the username/password sent to cmdctl, just like you suggested.
- Remove client-side PEM certificate (bindctl.pem), when bindctl starts up, it will ask certificate from cmdctl.
- 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
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: ↓ 9 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: ↓ 14 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.
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.