Opened 7 years ago

Closed 7 years ago

#2641 closed enhancement (fixed)

Disable default account, require authentication setup during initialization

Reported by: shane Owned by: muks
Priority: very high Milestone: Sprint-20130319
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 7.25 Internal?: no

Description

It's bad security practice to have default accounts.

This ticket is to disable the default account.

We should also have bindctl report something if this has not yet been done:

$ bindctl
Please configure a user account using the b10-cmdctl-usermgr program

This would require a change in the startup of bindctl to connect to the server before asking for user/password - but that's probably a good idea anyway. It would also require the cmdctl recognize that there are no users and report that via our RESTful API, but these seem simple changes.

Subtickets

Change History (39)

comment:1 Changed 7 years ago by shane

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:2 Changed 7 years ago by shane

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:3 Changed 7 years ago by shane

I should have made sure this one got on the sprint before the release candidate. :(

comment:4 Changed 7 years ago by jreed

Also see #2678 about documentation.

comment:5 Changed 7 years ago by jelte

note that this will probably break all lettuce and system tests. For lettuce, I suspect we can get around this through a fix in one or two of the terrain files. For systests I don't know.

comment:6 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130219

comment:7 Changed 7 years ago by shane

  • Priority changed from medium to very high

Bumping priority since it might be nice to get this in the RC. :)

comment:8 Changed 7 years ago by muks

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

Picking

comment:9 Changed 7 years ago by muks

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

Up for review.

This needs a ChangeLog entry:

XYZ.    [func]          muks
	There is no longer a default user account. The old default account
        with username 'root' has been removed. In a fresh installation of
	BIND 10, the administrator has to configure a user account using
	the b10-cmdctl-usermgr program.
        (Trac #2641, git ...)

Note that while some direct methods were tested, there are tests missing
for cmdctl HTTP API calls [both in cmdctl and also in bindctl (which
calls cmdctl)]. Adding such a framework for tests to cover the various
HTTP API calls would delay this bug far beyond its estimate. I suggest
we create another bug (similar to #2353) to comprehensively test bindctl
and cmdctl.

Please check that the lettuce tests also work properly when reviewing
this branch.

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

I think the guide also needs some updating (see also #2678). I'll do that when the bug comes back to me from review.

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

Replying to muks:

I think the guide also needs some updating (see also #2678). I'll do that when the bug comes back to me from review.

This is now fixed in the branch.

comment:12 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:13 follow-up: Changed 7 years ago by jinmei

general

Most of bindcmd and cmdctl changes are not tested. I'm not sure
if "tests missing for cmdctl HTTP API calls" meant this, but in any
case I don't think we can allow us to use it as an excuse of skipping
tests that "the amount of work would exceed the estimation". If it
would be beyond the estimation, we should defer the whole development
to a later task with re-evaluation, not allowing to merge the
implementation only. Besides, I don't necessarily think testing these
requires an excessively large amount of work; from a quick look many
things can be tested without requiring actual network I/O or SSL
related things (if that's the reason why you thought it was big).

If we really think adding tests is huge addition, my suggestion is:

  • if removing the default account is deemed to be important, introduce only that change, and defer all other implementation changes to a later task.
  • if we can also defer removing the default account, simply defer the entire task to post-release, re-estimating it if necessary.

bindcmd.py

  • _have_users repeats many things of _try_login. Besides, if I understand it correctly, the introduction of _have_users would make the checks in _try_login unnecessary. These should be clarified, and the code needs to be cleaned up accordingly.
  • _have_users lacks documentation.
  • send_POST: I'd revise it to:
            if post_param is not None and len(post_param) != 0:
                param = json.dumps(post_param)
    
    to clarify it's about comparison with None, not just whether it's evaluated to be true/false as boolean. I'd also remove the unnecessary parentheses.

cmdctl

  • why was cmdctl-accounts.csv moved to tests/testdata? Also, is the now-empty csv file under cmdctl/ used? And, in any event, the changes seem to introduce several non trivial implications. First, how should this work if we run bind10 from source using run_bind10.sh? Using the one in testdata or the empty one? If it's the latter, we can override the file for local experiments/tests and can accidentally commit it to the repository, which is not good. Also, I personally would like to avoid having hardcoded "tests/testdata" in the production version of script; even though things like B10_FROM_BUILD or B10_FROM_SOURCE is already a hack, embedded string of "test" makes it look even uglier.
  • get_num_users: it's not clear why such a very short method only used by one other method is defined as a separate public method. If the intent is to make it testable separately, that should be documented (but it seems we can easily cover that part of test by testing _handle_users_exist). If the intent is something else, I don't see the reason. And, in any case, this is essentially a private method, so unless it's needed for tests, it should be prefixed with __; if it's needed to be tested directly, I'd make it "protected" with a single underscore with documenting the intent.
  • test_get_num_users: although not necessarily harmful, I don't see the point of testing the result of get_user_info in this case:
            self.assertIn('6f0c73bd33101a5ec0294b3ca39fec90ef4717fe',
                          self.server.get_user_info('root'))
    

comment:14 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:15 Changed 7 years ago by jreed

Note that part of this was committed to master.

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

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

general

Most of bindcmd and cmdctl changes are not tested. I'm not sure
if "tests missing for cmdctl HTTP API calls" meant this, but in any
case I don't think we can allow us to use it as an excuse of skipping
tests that "the amount of work would exceed the estimation". If it
would be beyond the estimation, we should defer the whole development
to a later task with re-evaluation, not allowing to merge the
implementation only. Besides, I don't necessarily think testing these
requires an excessively large amount of work; from a quick look many
things can be tested without requiring actual network I/O or SSL
related things (if that's the reason why you thought it was big).

If we really think adding tests is huge addition, my suggestion is:

  • if removing the default account is deemed to be important, introduce only that change, and defer all other implementation changes to a later task.
  • if we can also defer removing the default account, simply defer the entire task to post-release, re-estimating it if necessary.

You have merged a partial implementation to master branch to disable
the default accounts. The rest of the work is now in the trac2641_3
branch, which is revised on top of that work.

bindcmd.py

  • _have_users repeats many things of _try_login. Besides, if I understand it correctly, the introduction of _have_users would make the checks in _try_login unnecessary. These should be clarified, and the code needs to be cleaned up accordingly.

I don't understand how the checks are unnecessary. If these failures
occur, they should still cause FailToLogin to be raised. But maybe I
don't follow properly.

  • _have_users lacks documentation.

This has been added.

  • send_POST: I'd revise it to:
            if post_param is not None and len(post_param) != 0:
                param = json.dumps(post_param)
    
    to clarify it's about comparison with None, not just whether it's evaluated to be true/false as boolean. I'd also remove the unnecessary parentheses.

Nod. I had this version first, and shortened it. :)

cmdctl

  • why was cmdctl-accounts.csv moved to tests/testdata? Also, is the now-empty csv file under cmdctl/ used? And, in any event, the changes seem to introduce several non trivial implications. First, how should this work if we run bind10 from source using run_bind10.sh? Using the one in testdata or the empty one? If it's the latter, we can override the file for local experiments/tests and can accidentally commit it to the repository, which is not good. Also, I personally would like to avoid having hardcoded "tests/testdata" in the production version of script; even though things like B10_FROM_BUILD or B10_FROM_SOURCE is already a hack, embedded string of "test" makes it look even uglier.

B10_FROM_SOURCE and B10_FROM_BUILD hotwire stuff for the tests to work.
I don't think making use of tests/testdata/ in this context is too
bad, considering the contents of the .csv file *are* test data and it
belongs in the tests/testdata/ directory.

I think you have made it not install the empty one in your branch, so
there is no longer any empty file in the tree.

  • get_num_users: it's not clear why such a very short method only used by one other method is defined as a separate public method. If the intent is to make it testable separately, that should be documented (but it seems we can easily cover that part of test by testing _handle_users_exist). If the intent is something else, I don't see the reason. And, in any case, this is essentially a private method, so unless it's needed for tests, it should be prefixed with __; if it's needed to be tested directly, I'd make it "protected" with a single underscore with documenting the intent.

get_num_users() belongs to a different class (SecureHTTPServer) and
access member data inside that class. It is called from
SecureHTTPRequestHandler class. So for this reason, it is public. It
is also easy to test it this way.

  • test_get_num_users: although not necessarily harmful, I don't see the point of testing the result of get_user_info in this case:
            self.assertIn('6f0c73bd33101a5ec0294b3ca39fec90ef4717fe',
                          self.server.get_user_info('root'))
    

I have moved this to a unittest for get_user_info(). I hope this is a
more appropriate place for it.

In response to your other comments about missing tests, I have covered
the API with tests now. I have also added tests for some related code in
cmdctl and bindctl which were missing tests. Please check these. I think
these should (reasonably) be enough tests for the API.

I should have assigned this ticket back earlier today, but I found a regression triggered during lettuce testing that had to be fixed first.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

bindcmd.py

  • _have_users repeats many things of _try_login. Besides, if I understand it correctly, the introduction of _have_users would make the checks in _try_login unnecessary. These should be clarified, and the code needs to be cleaned up accordingly.

I don't understand how the checks are unnecessary. If these failures
occur, they should still cause FailToLogin to be raised. But maybe I
don't follow properly.

The checks in _try_login was mainly for catching the issue that
would be reported by __print_check_ssl_msg. But it's very unlikely
to happen in _try_login any more because that case should have been
eliminated on the successful completion of _have_users. Am I clear
now?

But, on thinking about this more closely, a more fundamental point
occurred to me. I now think it's probably not really a good idea to
introduce things like _have_users in the first place. Not being a
security expert, my general sense of security is that it's generally a
bad practice to give a possibly untrusted remote user unnecessary
state at the server side. For a login session, the should simply say
the login succeeds or not for the given user name and password; it's
not wise to tell the user if the password file exists or the list is
empty. In that sense, the existing code would also be not really
good; I guess we shouldn't terminate the connection immediately after
cmdctl finds the file isn't accessible, but it should reject the login
request due to that just like the case where the user/password doesn't
match.

If we need to give the user a hint for possible diagnosis (like "you
should probably check the cmdctl log"), that should be a local
extension to bindctl, not based on a response from cmdctl.

cmdctl

  • why was cmdctl-accounts.csv moved to tests/testdata? Also, is the now-empty csv file under cmdctl/ used? And, in any event, the changes seem to introduce several non trivial implications. First, how should this work if we run bind10 from source using run_bind10.sh? Using the one in testdata or the empty one? If it's the latter, we can override the file for local experiments/tests and can accidentally commit it to the repository, which is not good. Also, I personally would like to avoid having hardcoded "tests/testdata" in the production version of script; even though things like B10_FROM_BUILD or B10_FROM_SOURCE is already a hack, embedded string of "test" makes it look even uglier.

B10_FROM_SOURCE and B10_FROM_BUILD hotwire stuff for the tests to work.
I don't think making use of tests/testdata/ in this context is too
bad, considering the contents of the .csv file *are* test data and it
belongs in the tests/testdata/ directory.

I don't necessarily object to placing the test .csv files under
tests. My point is that something like
/usr/local/libexec/bind10/b10-cmdctl should have this line:

            accountsfile  = sysconf_path + "tests/testdata/cmdctl-accounts.csv"

admittedly, the line before this is also not clean:

            sysconf_path = os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/"

but I can live it if there's no simpler alternative. But I don't
think that can be used as an excuse to make it even uglier.

The "fundamental point" above may make most of other part of the
branch moot, so I'm giving the ticket back to you at this point.
But I'm dumping specific code comments I had so far just for reference
(note, again, that they may become moot).

bindcmd.py

  • _have_users: what if json.loads() (or maybe even read() or decode()) fails and raises an exception?
                if response.status == http.client.OK:
                    return json.loads(response.read().decode())
    
  • _have_users: shouldn't we display what's specifically wrong here?
                # if not OK, fall through to raise error
                self._print("Failure in cmdctl when checking if users already exist")
    
    (and this line is too long)

bindctl_test.py

  • not actually about the test but rather about the main code (and, actually it's rather derived from _try_login), this message looks awkward:
                expected_printed_messages.append(
                    'Socket error checking if users exist:  test error')
    
    due to the redundant white space. Shouldn't the main code be actually like this?
                self._print("SSL error checking if users exist:", err)
    
    Same for other similar cases.
  • test_have_users has a lot of duplicates with test_try_login.
  • test_have_users: cases where json.loads/response.read/decode raises an exception aren't tested. See also comments on the code.

comment:18 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:19 in reply to: ↑ 17 Changed 7 years ago by shane

Replying to jinmei:

But, on thinking about this more closely, a more fundamental point
occurred to me. I now think it's probably not really a good idea to
introduce things like _have_users in the first place. Not being a
security expert, my general sense of security is that it's generally a
bad practice to give a possibly untrusted remote user unnecessary
state at the server side. For a login session, the should simply say
the login succeeds or not for the given user name and password; it's
not wise to tell the user if the password file exists or the list is
empty. In that sense, the existing code would also be not really
good; I guess we shouldn't terminate the connection immediately after
cmdctl finds the file isn't accessible, but it should reject the login
request due to that just like the case where the user/password doesn't
match.

If we need to give the user a hint for possible diagnosis (like "you
should probably check the cmdctl log"), that should be a local
extension to bindctl, not based on a response from cmdctl.

It's still annoying to have to type in a user name & password to be told "hey, maybe you should, you know, set up the system?"... but it does make sense from general security principles.

So, the approach would be:

  • On failed login, check to see if the user has logged in successfully ever (this can be stored in the ~/.bind10/ directory, perhaps even in the ~/.bind10/default_user.csv). If not, then output a message saying something like Jinmei describes, although possibly more explicit:

Login failed: either the user name or password is invalid.
When the system is first set up you need to create at least one user account. For
information on how to set up a BIND 10 system, please check see the BIND 10 Guide:

http://bind10.isc.org/docs/bind10-guide.html#quick-start-auth-dns

If a user account has been set up, please check the b10-cmdctl log for other
information.

  • On a successful login, set the flag to indicate that the user has at one point logged in. This is to prevent the helpful message above after the system has actually ben setup - when it is no longer helpful.

comment:20 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

I have removed the newly introduced cmdctl API endpoint and also the call from bindctl to it (to check whether users exist or not). So the major thing that this branch does is add missing tests for previously existing code.

From reading the code and also running bindctl -> cmdctl, there doesn't seem to be any difference between how it treats a lack of users vs. a password mismatch during authentication. For both cases, it returns exactly the same error back to bindctl.

I have updated the bindctl messages that bindctl prints when login fails as suggested. If I understand it correctly, there's nothing more to do for this ticket.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

I have removed the newly introduced cmdctl API endpoint and also the call from bindctl to it (to check whether users exist or not). So the major thing that this branch does is add missing tests for previously existing code.

From reading the code and also running bindctl -> cmdctl, there doesn't seem to be any difference between how it treats a lack of users vs. a password mismatch during authentication. For both cases, it returns exactly the same error back to bindctl.

Looks like so, but I now wonder why the socket or SSL error doesn't
happen when, e.g., the account file exists but lacks permission.
Related, I wonder whether we still need these messages:

        except ssl.SSLError as err:
            self._print("SSL error while sending login information: ", err)
            if err.errno == ssl.SSL_ERROR_EOF:
                self.__print_check_ssl_msg()
        except socket.error as err:
            self._print("Socket error while sending login information: ", err)
            # An SSL setup error can also bubble up as a plain CONNRESET...
            # (on some systems it usually does)
            if err.errno == errno.ECONNRESET:
                self.__print_check_ssl_msg()
            pass

I'd also note that this message isn't shown once the local password
cache (~/.bind10) is created. I'm not sure that was your intent, but
in any case I think the behavior makes sense.

There's one remaining open points: in cmdctl.py.in, I'd still like to
avoid hardconding 'tests/testdata':

            sysconf_path = os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/"
            accountsfile  = sysconf_path + "tests/testdata/cmdctl-accounts.csv"

one way is to keep it under
os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/" as before,
although you may not like it as this file is now also used in unit
tests. In that case, an alternative would be to copy
tests/testdata/cmdctl-accounts.csv to {top_builddir}/src/bin/cmdctl
(or even directly on top_builddir) and refer to it from cmdctl.py.in
when B10_FROM_BUILD is defined.

comment:22 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

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

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

Related, I wonder whether we still need these messages:

        except ssl.SSLError as err:
            self._print("SSL error while sending login information: ", err)
            if err.errno == ssl.SSL_ERROR_EOF:
                self.__print_check_ssl_msg()
        except socket.error as err:
            self._print("Socket error while sending login information: ", err)
            # An SSL setup error can also bubble up as a plain CONNRESET...
            # (on some systems it usually does)
            if err.errno == errno.ECONNRESET:
                self.__print_check_ssl_msg()
            pass

If such exceptions are raised (due to any environmental reasons),
they'll go unhandled. So maybe there's no harm in leaving them there.

I'd also note that this message isn't shown once the local password
cache (~/.bind10) is created. I'm not sure that was your intent, but
in any case I think the behavior makes sense.

A different message is shown on purpose when default_user.csv exists
(which indicates the user has logged in successfully at some point in
the past).

There's one remaining open points: in cmdctl.py.in, I'd still like to
avoid hardconding 'tests/testdata':

            sysconf_path = os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/"
            accountsfile  = sysconf_path + "tests/testdata/cmdctl-accounts.csv"

one way is to keep it under
os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/" as before,
although you may not like it as this file is now also used in unit
tests. In that case, an alternative would be to copy
tests/testdata/cmdctl-accounts.csv to {top_builddir}/src/bin/cmdctl
(or even directly on top_builddir) and refer to it from cmdctl.py.in
when B10_FROM_BUILD is defined.

I've moved it back to os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/" as before.

comment:24 in reply to: ↑ 23 ; follow-ups: Changed 7 years ago by jinmei

Replying to muks:

Hi Jinmei

        except ssl.SSLError as err:
            self._print("SSL error while sending login information: ", err)
            if err.errno == ssl.SSL_ERROR_EOF:
                self.__print_check_ssl_msg()
        except socket.error as err:
            self._print("Socket error while sending login information: ", err)
            # An SSL setup error can also bubble up as a plain CONNRESET...
            # (on some systems it usually does)
            if err.errno == errno.ECONNRESET:
                self.__print_check_ssl_msg()
            pass

If such exceptions are raised (due to any environmental reasons),
they'll go unhandled. So maybe there's no harm in leaving them there.

Could you also explain this?

Looks like so, but I now wonder why the socket or SSL error doesn't
happen when, e.g., the account file exists but lacks permission.

And, depending on the answer for it, the message printed by
__print_check_ssl_msg shouldn't make sense any more.

There's one remaining open points: in cmdctl.py.in, I'd still like to
avoid hardconding 'tests/testdata':

            sysconf_path = os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/"
            accountsfile  = sysconf_path + "tests/testdata/cmdctl-accounts.csv"

one way is to keep it under
os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/" as before,
although you may not like it as this file is now also used in unit
tests. In that case, an alternative would be to copy
tests/testdata/cmdctl-accounts.csv to {top_builddir}/src/bin/cmdctl
(or even directly on top_builddir) and refer to it from cmdctl.py.in
when B10_FROM_BUILD is defined.

I've moved it back to os.environ["B10_FROM_SOURCE"] + "/src/bin/cmdctl/" as before.

Okay, but then cmdctl-accounts.csv should be re-created under
bin/cmdctl. In fact, unit test doesn't pass any more; if this means
you actually didn't test it on the revised branch, please make sure
everything including system/lettuce tests (and distcheck) pass.

comment:25 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

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

Hi Jinmei

Replying to jinmei:

Okay, but then cmdctl-accounts.csv should be re-created under
bin/cmdctl. In fact, unit test doesn't pass any more; if this means
you actually didn't test it on the revised branch, please make sure
everything including system/lettuce tests (and distcheck) pass.

It was tested on the revised branch. I always run make check and lettuce before putting anything to review. What had happened was that I missed using git in the move command:

[muks@totoro ~]$ history | grep cmdctl-accounts
  858  mv tests/testdata/cmdctl-accounts.csv .

I've added it back to the branch now.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

It was tested on the revised branch. I always run make check and lettuce before putting anything to review. What had happened was that I missed using git in the move command:

[muks@totoro ~]$ history | grep cmdctl-accounts
  858  mv tests/testdata/cmdctl-accounts.csv .

I've added it back to the branch now.

Okay, but distcheck still failed. Is that only me?

======================================================================
FAIL: test_check_bad_certificates (__main__.TestCertGenTool)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jinmei/src/isc/git/bind10-2641/bind10-20130213/_build/../src/bin/cmdctl/tests/b10-certgen_test.py", line 179, in test_check_bad_certificates
    self.validate_certificate(10, path + 'expired-certfile.pem')
  File "/Users/jinmei/src/isc/git/bind10-2641/bind10-20130213/_build/../src/bin/cmdctl/tests/b10-certgen_test.py", line 121, in validate_certificate
    [self.TOOL, '-q', '-c', certfile])
  File "/Users/jinmei/src/isc/git/bind10-2641/bind10-20130213/_build/../src/bin/cmdctl/tests/b10-certgen_test.py", line 107, in run_check
    self.assertEqual(expected_returncode, returncode, " ".join(command))
AssertionError: 10 != 105 : ../b10-certgen -q -c testdata/expired-certfile.pem

comment:28 in reply to: ↑ 27 Changed 7 years ago by muks

Replying to jinmei:

Okay, but distcheck still failed. Is that only me?

======================================================================
FAIL: test_check_bad_certificates (__main__.TestCertGenTool)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jinmei/src/isc/git/bind10-2641/bind10-20130213/_build/../src/bin/cmdctl/tests/b10-certgen_test.py", line 179, in test_check_bad_certificates
    self.validate_certificate(10, path + 'expired-certfile.pem')
  File "/Users/jinmei/src/isc/git/bind10-2641/bind10-20130213/_build/../src/bin/cmdctl/tests/b10-certgen_test.py", line 121, in validate_certificate
    [self.TOOL, '-q', '-c', certfile])
  File "/Users/jinmei/src/isc/git/bind10-2641/bind10-20130213/_build/../src/bin/cmdctl/tests/b10-certgen_test.py", line 107, in run_check
    self.assertEqual(expected_returncode, returncode, " ".join(command))
AssertionError: 10 != 105 : ../b10-certgen -q -c testdata/expired-certfile.pem

I did not run distcheck. It should be fixed now.

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

  • Owner changed from muks to jinmei

Replying to jinmei:

Replying to muks:

Hi Jinmei

        except ssl.SSLError as err:
            self._print("SSL error while sending login information: ", err)
            if err.errno == ssl.SSL_ERROR_EOF:
                self.__print_check_ssl_msg()
        except socket.error as err:
            self._print("Socket error while sending login information: ", err)
            # An SSL setup error can also bubble up as a plain CONNRESET...
            # (on some systems it usually does)
            if err.errno == errno.ECONNRESET:
                self.__print_check_ssl_msg()
            pass

If such exceptions are raised (due to any environmental reasons),
they'll go unhandled. So maybe there's no harm in leaving them there.

Could you also explain this?

The method's description has been updated for it.

Looks like so, but I now wonder why the socket or SSL error doesn't
happen when, e.g., the account file exists but lacks permission.

And, depending on the answer for it, the message printed by
__print_check_ssl_msg shouldn't make sense any more.

The message here has also been updated. It still asks to check b10-cmdctl logs (as something may be logged in this case), but doesn't say anything about permissions anymore.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

If such exceptions are raised (due to any environmental reasons),
they'll go unhandled. So maybe there's no harm in leaving them there.

Could you also explain this?

The method's description has been updated for it.

Maybe I wasn't clear enough...first off, I didn't understand whether
or not SSL or socket error can still happen in the scenario described
in #2595. If it can still happen, we should still also prevent that
in the sense we discussed in this (#2641) ticket. If it can't, at
least it doesn't make sense any more to be this specific in bindcmd.py:

        except ssl.SSLError as err:
            self._print("SSL error while sending login information: ", err)
            if err.errno == ssl.SSL_ERROR_EOF:
                self.__print_check_ssl_msg()
        except socket.error as err:
            self._print("Socket error while sending login information: ", err)
            # An SSL setup error can also bubble up as a plain CONNRESET...
            # (on some systems it usually does)
            if err.errno == errno.ECONNRESET:
                self.__print_check_ssl_msg()
            pass

At least it doesn't make sense to catch specific errno's like
SSL_ERROR_EOF or ECONNRESET. __print_check_ssl_msg would also not
be needed any more either. And, since ssl.SSLError is derived from
socket.error (but maybe Python 3.3 has changed that; don't remember
it), it also does not even make sense to catch SSLError separately.
And, further, if socket.error is sufficiently unusual (again, it's
just an 'if'; I didn't check how often it can happen in the latest
code), explicitly catching socket.error may also not even make sense.

What I wanted is to clarify these things and make the code as simple
(while still providing necessary logs/outputs) as possible based on
that clarification.

BTW, bindctl unit tests now fail. I suspect it was because of the
latest changes.

(and I couldn't confirm whether distcheck now passes).

As for the change to b10-certgen_test, (although maybe not directly
related to this ticket) I wonder why we have this 'if-else':

        if ('CMDCTL_TESTDATA_PATH' in os.environ):
            path = os.environ['CMDCTL_TESTDATA_PATH'] + "/tests/testdata/"
        else:
            path = "testdata/"

Also, setting CMDCTL_TESTDATA_PATH' to the srcdir seems to be awkward
(why it's named "TESTDATA_PATH" then?). Some clarification and
cleanup seem to be needed here.

comment:31 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

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

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

Maybe I wasn't clear enough...first off, I didn't understand whether
or not SSL or socket error can still happen in the scenario described
in #2595. If it can still happen, we should still also prevent that
in the sense we discussed in this (#2641) ticket. If it can't, at
least it doesn't make sense any more to be this specific in bindcmd.py:

        except ssl.SSLError as err:
            self._print("SSL error while sending login information: ", err)
            if err.errno == ssl.SSL_ERROR_EOF:
                self.__print_check_ssl_msg()
        except socket.error as err:
            self._print("Socket error while sending login information: ", err)
            # An SSL setup error can also bubble up as a plain CONNRESET...
            # (on some systems it usually does)
            if err.errno == errno.ECONNRESET:
                self.__print_check_ssl_msg()
            pass

At least it doesn't make sense to catch specific errno's like
SSL_ERROR_EOF or ECONNRESET. __print_check_ssl_msg would also not
be needed any more either. And, since ssl.SSLError is derived from
socket.error (but maybe Python 3.3 has changed that; don't remember
it), it also does not even make sense to catch SSLError separately.
And, further, if socket.error is sufficiently unusual (again, it's
just an 'if'; I didn't check how often it can happen in the latest
code), explicitly catching socket.error may also not even make sense.

What I wanted is to clarify these things and make the code as simple
(while still providing necessary logs/outputs) as possible based on
that clarification.

I have unified the exceptions to check them in a single except clause,
and have also updated the messages. Is this a reasonable solution?
I ask this because it's still possible for a socket error to occur due
to the environment (not just some condition at b10-cmdctl) and we may
want to handle such an exception.

BTW, bindctl unit tests now fail. I suspect it was because of the
latest changes.

(and I couldn't confirm whether distcheck now passes).

If it fails again, please can you include how this failed for you? I do
run make check (and in this bug, make distcheck after your recent
comment) before putting a bug to review. It did not fail for me, so the
failure message would be helpful.

From code updates done now, it could be because messages printed after
an exception in bindctl changed, but distcheck did not fail when the
bug was put to review last time.

As for the change to b10-certgen_test, (although maybe not directly
related to this ticket) I wonder why we have this 'if-else':

        if ('CMDCTL_TESTDATA_PATH' in os.environ):
            path = os.environ['CMDCTL_TESTDATA_PATH'] + "/tests/testdata/"
        else:
            path = "testdata/"

Also, setting CMDCTL_TESTDATA_PATH' to the srcdir seems to be awkward
(why it's named "TESTDATA_PATH" then?). Some clarification and
cleanup seem to be needed here.

This environment variable has been renamed to CMDCTL_SRC_PATH and the
code has been adjusted for it.

Before putting this bug to review, I ran make distcheck and it passes
on my workstation.

comment:33 in reply to: ↑ 32 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

I have unified the exceptions to check them in a single except clause,
and have also updated the messages. Is this a reasonable solution?
I ask this because it's still possible for a socket error to occur due
to the environment (not just some condition at b10-cmdctl) and we may
want to handle such an exception.

Specifically how can that happen? If it's not related to the user
account stuff (missing file, lack of permission, missing entry, etc)
but is purely a network related problem, then it basically looks okay.
Whether this one makes sense would also depend on how it can happen:

            self._print("Please check the logs of b10-cmdctl, there may "
                        "have been a problem accepting SSL connections.")

When this happens would something be actually logged in b10-cmdctl?

Also, I thought we discussed this before, but possibly repeating
myself, this double-space after colon is awkward.

+                'Error while sending login information:  test error')

BTW, bindctl unit tests now fail. I suspect it was because of the
latest changes.

(and I couldn't confirm whether distcheck now passes).

If it fails again, please can you include how this failed for you? I do
run make check (and in this bug, make distcheck after your recent
comment) before putting a bug to review. It did not fail for me, so the
failure message would be helpful.

Hmm, after a complete clean and rebuild, unit tests now passed. Maybe
it was due to some intermediate leftover.

comment:34 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

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

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

Specifically how can that happen? If it's not related to the user
account stuff (missing file, lack of permission, missing entry, etc)
but is purely a network related problem, then it basically looks okay.
Whether this one makes sense would also depend on how it can happen:

            self._print("Please check the logs of b10-cmdctl, there may "
                        "have been a problem accepting SSL connections.")

When this happens would something be actually logged in b10-cmdctl?

I had network related problem in mind. This message about checking
b10-cmdctl logs has been removed now.

Also, I thought we discussed this before, but possibly repeating
myself, this double-space after colon is awkward.

+                'Error while sending login information:  test error')

Fixed now.

comment:36 in reply to: ↑ 35 Changed 7 years ago by jinmei

Okay, it now looks ready for merge.

comment:37 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:38 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 7.25

comment:39 Changed 7 years ago by muks

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

Merged to master branch in commit 6067533e5c4ebded8260ef4b97f2709868bb2799:

* 85a61f1 [2641] Remove duplicate space from messages
* 0f0c175 [2641] Removed message asking users to check b10-cmdctl logs
* 99fbbc3 [2641] Rename and fix CMDCTL_SRC_PATH usage
* ee109e1 [2641] Unify exception handling code
* e2e93cd [2641] Update message that is printed when an SSL or socket error occurs
* 60aa5f5 [2641] Add a note to the description about the errors that we expect
* 7c62339 [2641] Fix test env variable (fix distcheck)
* 90251f7 [2641] Add back missing cmdctl-accounts.csv
* 4ea6a9c [2641] Move cmdctl-accounts.csv file back to where it was
* cfc683c [2641] editorial: folded a long line
* 27dc1e7 [2641] Revise prompts displayed by bindctl when login fails
* f75fcf0 [2641] Remove the /users-exist cmdctl API call, and client code from bindctl
* 80b1a68 [2641] Fix __init__() args
* 77af081 [2641] Test /login API call
* e86811d [2641] Rename test
* 1e653cb [2641] Test /users-exist API call
* 2b9c6a4 [2641] Add test for _is_session_valid()
* 77520e9 [2641] Test that _try_login() makes the right API call to cmdctl
* 0abfd1c [2641] Test that login_to_cmdctl() calls _have_users()
* 7886858 [2641] Test that _have_users() makes the right API call to cmdctl
* 221bb9d [2641] Add tests for _have_users()
* ada6840 [2641] Update doc for _try_login()
* e363285 [2641] Add doc for _have_users()
* e1725e2 [2641] Add doc for get_num_users()
* 2133c6a [2641] Update if statement
* f597ce3 [2641] Move get_user_info() call to its own unittest
* b6d5135 [2641] a minor piggy-back style cleanup: removed space in default param spec.
* 544b01d [2641] From bindctl, call /users-exist in cmdctl to check if users exist
* 10c4a13 [2641] Handle /users-exist API call in cmdctl
* 97d4cce [2641] Add method to get the number of users
* 21e0ea8 [2641] Move populated cmdctl-accounts.csv to testdata directory

make distcheck passed after merge to master. ChangeLog has been updated already for this bug.

Resolving as fixed. Thank you for the reviews Jinmei.

Note: See TracTickets for help on using tickets.