Bugfix: get correct principal name when keytab is given #97
Conversation
belldandyxtq
force-pushed
the
bugfix_krb_keytab
branch
from
d56e4f5
to
c77dffe
Compare
belldandyxtq
changed the title
belldandyxtq
force-pushed
the
bugfix_krb_keytab
branch
from
c77dffe
to
4a7f9cb
Compare
There was a problem hiding this comment.
Currently username is only taken from keytab. But I think it can be improved more: my suggestion of taking username from krb5 info is like following::
- try to take username from just
klist, regardingKRB5CCNAME - If 1 fails, try to take it from
klist -k, regardingKRB5_KTNAME - If 2 fails, fall back to Unix user name.
Limitation on procedure 2 that it only takes first KVNO in the keytab must be documented precisely.
| else: | ||
| os.environ['KRB5_KTNAME'] = original_krb5_ktname | ||
|
|
||
| chainerio.remove(keytab_path) |
There was a problem hiding this comment.
Use tempfile.TemporaryDirectory() with it's cleanup capability on with statement in case of unexpected test failure.
|
|
||
| # save the original KRB5_KTNAME | ||
| if "KRB5_KTNAME" in os.environ: | ||
| original_krb5_ktname = os.environ['KRB5_KTNAME'] |
There was a problem hiding this comment.
Use os.getenv() or os.environ.get() for default value.
|
|
||
| # put KRB5_KTNAME back | ||
| if original_krb5_ktname is None: | ||
| del os.environ['KRB5_KTNAME'] |
There was a problem hiding this comment.
Also this must be done in finally clause in case of unexpected exceptions.
| out, err = pipe.communicate() | ||
| return keytab_path | ||
|
|
||
| def test_get_principel_name_from_keytab(self): |
There was a problem hiding this comment.
This method name indicates it being intended for unittest of def _parse_klist_keytab_output(output): somehow. Also practical unittest on parsing the output of klist had better be separated from integrated test of create_handler().
|
The username is not only taken from keytab. If the keytab is given then we get username from keytab, otherwise from |
Yes. But the priority behaviour is different than mine. Current behaviour is 1) if KRB5_KTNAME is defined then run
|
| keytab_path = self.create_dummy_keytab(tmpd, dummy_username) | ||
|
|
||
| os.environ['KRB5_KTNAME'] = keytab_path | ||
| with HdfsFileSystem() as handler: |
There was a problem hiding this comment.
My intention was not to change from create_handler to direct object construction, but to clarify what unit feature to be tested with the test method name matching test target. Hereby there're several unit functionalities here, 1) calling the klist command, 2) parsing klist command output, 3) identifying the user name. Strictly speaking, all these three must be tested one by one in unit tests. I don't request strict tests, but at least it must be clarified what is being tested here.
In other word, _parse_klist_output() is tested at (new) line 93 below, but why not _parse_klist_keytab_output(out) ? Also integrated test of those would be nice if it's done here.
This commit fixes a bug where the principal name is not obtained correctly when a keytab is given though KRB5_KTNAME
belldandyxtq
force-pushed
the
bugfix_krb_keytab
branch
from
744df8d
to
155449b
Compare
| @@ -44,21 +49,97 @@ def test_read_non_exist(self): | |||
| with chainerio.create_handler(self.fs) as handler: | |||
| self.assertRaises(IOError, handler.open, non_exist_file) | |||
|
|
|||
| def test_klist_not_exist(self): | |||
| path = os.environ['PATH'] | |||
| def create_dummy_keytab(self, tmpd, dummy_username): | |||
There was a problem hiding this comment.
As self isn't used in this method, it should be moved out of the class.
chainerio/filesystems/hdfs.py
Outdated
| return _parse_principal_name_from_klist(output.decode('utf-8')) | ||
|
|
||
|
|
||
| def _run_klist(keytab_path=None): |
There was a problem hiding this comment.
I think passing boolean indicating -k rather than string would remove potential bug that may stem from string variations.
chainerio/filesystems/hdfs.py
Outdated
|
|
||
|
|
||
| def _get_principal_name_from_keytab(): | ||
| keytab_path = os.getenv("KRB5_KTNAME") |
There was a problem hiding this comment.
I don't this line is needed, but just running klist -k is enough. Because not only it automatically refers to KRB5_KTNAMEbut follows default fallback to /etc/krb5.keytab .
chainerio/filesystems/hdfs.py
Outdated
|
|
||
| if principal_name is not None: | ||
| return principal_name | ||
| else: |
There was a problem hiding this comment.
This else indent is not needed, I would prefer it removed for readability.
This commit fixes a bug where the principal name is not obtained
correctly when a keytab is given though KRB5_KTNAME
This closes #93
This should be tested after #96