Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support strings and bytes for credential store keys and values (#242) #243

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Conversation

michael-o
Copy link
Contributor

This closes #242

Disclaimer: I am not a daily Python hacker, I hope I correctly understood how Cython and type conversion/assignment from Python to C works here.
I wasn't also able to run tests because of:

$ python3 setup.py test
Building from Cython files...
Detected: krb5-config
running test
WARNING: Testing via this command is deprecated and will be removed in a future version. Users looking for a generic test entry point independent of test runner are encouraged to use tox.
running egg_info
writing gssapi.egg-info/PKG-INFO
writing dependency_links to gssapi.egg-info/dependency_links.txt
writing requirements to gssapi.egg-info/requires.txt
Compiling gssapi/raw/ext_cred_store.pyx because it changed.
[1/1] Cythonizing gssapi/raw/ext_cred_store.pyx
writing top-level names to gssapi.egg-info/top_level.txt
reading manifest template 'MANIFEST.in'
warning: no files found matching '*.txt' under directory 'docs'
writing manifest file 'gssapi.egg-info/SOURCES.txt'
running build_ext
building 'gssapi.raw.ext_cred_store' extension
cc -pthread -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -O2 -pipe -fstack-protector-strong -fno-strict-aliasing -fPIC -Igssapi/raw -I./gssapi/raw -I/usr/local/include/python3.7m -c gssapi/raw/ext_cred_store.c -o build/temp.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_cred_store.o -I/usr/local/include -DHAS_GSSAPI_EXT_H
gssapi/raw/ext_cred_store.c:2510:21: warning: assigning to 'gss_key_value_set_desc *' (aka 'struct
      gss_key_value_set_struct *') from 'gss_const_key_value_set_t' (aka 'const struct
      gss_key_value_set_struct *') discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
    __pyx_v_c_store = GSS_C_NO_CRED_STORE;
                    ^ ~~~~~~~~~~~~~~~~~~~
gssapi/raw/ext_cred_store.c:3281:21: warning: assigning to 'gss_key_value_set_desc *' (aka 'struct
      gss_key_value_set_struct *') from 'gss_const_key_value_set_t' (aka 'const struct
      gss_key_value_set_struct *') discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
    __pyx_v_c_store = GSS_C_NO_CRED_STORE;
                    ^ ~~~~~~~~~~~~~~~~~~~
gssapi/raw/ext_cred_store.c:3987:21: warning: assigning to 'gss_key_value_set_desc *' (aka 'struct
      gss_key_value_set_struct *') from 'gss_const_key_value_set_t' (aka 'const struct
      gss_key_value_set_struct *') discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
    __pyx_v_c_store = GSS_C_NO_CRED_STORE;
                    ^ ~~~~~~~~~~~~~~~~~~~
3 warnings generated.
cc -pthread -shared -L/usr/local/lib -fstack-protector-strong build/temp.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_cred_store.o -L/usr/local/lib -L/usr/lib -L/usr/local/lib -L/usr/local/lib -lgssapi_krb5 -lkrb5 -lk5crypto -lcom_err -lpython3.7m -o build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_cred_store.so -Wl,-rpath,/usr/local/lib:/usr/lib -fstack-protector-strong
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/misc.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/exceptions.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/creds.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/names.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/sec_contexts.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/types.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/message.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/oids.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/cython_converters.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/chan_bindings.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_s4u.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_cred_store.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_rfc4178.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_rfc5587.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_rfc5588.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_rfc5801.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_cred_imp_exp.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_dce.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_iov_mic.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_ggf.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_set_cred_opt.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_rfc6680.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_rfc6680_comp_oid.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_password.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/ext_password_add.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/mech_krb5.so -> gssapi/raw
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/_enum_extensions/ext_dce.so -> gssapi/raw/_enum_extensions
copying build/lib.freebsd-12.2-STABLE-amd64-3.7/gssapi/raw/_enum_extensions/ext_iov_mic.so -> gssapi/raw/_enum_extensions
Traceback (most recent call last):
  File "setup.py", line 389, in <module>
    install_requires=install_requires
  File "/usr/local/lib/python3.7/site-packages/setuptools/__init__.py", line 145, in setup
    return distutils.core.setup(**attrs)
  File "/usr/local/lib/python3.7/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/local/lib/python3.7/distutils/dist.py", line 966, in run_commands
    self.run_command(cmd)
  File "setup.py", line 230, in run_command
    Distribution.run_command(self, command)
  File "/usr/local/lib/python3.7/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "/usr/local/lib/python3.7/site-packages/setuptools/command/test.py", line 237, in run
    self.run_tests()
  File "/usr/local/lib/python3.7/site-packages/setuptools/command/test.py", line 259, in run_tests
    exit=False,
  File "/usr/local/lib/python3.7/unittest/main.py", line 100, in __init__
    self.parseArgs(argv)
  File "/usr/local/lib/python3.7/unittest/main.py", line 124, in parseArgs
    self._do_discovery(argv[2:])
  File "/usr/local/lib/python3.7/unittest/main.py", line 244, in _do_discovery
    self.createTests(from_discovery=True, Loader=Loader)
  File "/usr/local/lib/python3.7/unittest/main.py", line 154, in createTests
    self.test = loader.discover(self.start, self.pattern, self.top)
  File "/usr/local/lib/python3.7/unittest/loader.py", line 349, in discover
    tests = list(self._find_tests(start_dir, pattern))
  File "/usr/local/lib/python3.7/unittest/loader.py", line 406, in _find_tests
    full_path, pattern, namespace)
  File "/usr/local/lib/python3.7/unittest/loader.py", line 483, in _find_test_path
    tests = self.loadTestsFromModule(package, pattern=pattern)
  File "/usr/local/lib/python3.7/site-packages/setuptools/command/test.py", line 55, in loadTestsFromModule
    tests.append(self.loadTestsFromName(submodule))
  File "/usr/local/lib/python3.7/unittest/loader.py", line 191, in loadTestsFromName
    return self.loadTestsFromModule(obj)
  File "/usr/local/lib/python3.7/site-packages/setuptools/command/test.py", line 55, in loadTestsFromModule
    tests.append(self.loadTestsFromName(submodule))
  File "/usr/local/lib/python3.7/unittest/loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
  File "/var/osipovmi/Projekte/python-gssapi/gssapi/tests/test_high_level.py", line 16, in <module>
    import k5test.unit as ktu
  File "/net/home/osipovmi/.local/lib/python3.7/site-packages/k5test/__init__.py", line 1, in <module>
    from k5test.realm import K5Realm  # noqa
  File "/net/home/osipovmi/.local/lib/python3.7/site-packages/k5test/realm.py", line 92, in <module>
    'kdb'),
  File "/usr/local/lib/python3.7/posixpath.py", line 80, in join
    a = os.fspath(a)
TypeError: expected str, bytes or os.PathLike object, not NoneType

Design decision for sys.getdefaultencoding(): I do understand that the entire API uses UTF-8 to convert Python strings to char * because Kerberos requires names to be encoded UTF-8 by RFC. Here, we use filesystem paths where Kerberos operates on char * which will be passed to the FS and most Unix FS will store paths as raw bytes of the locale instead of canonicalized form in UTF-8 or UTF-16. I hope that most have abandoned non-UTF-8 locales, but who knows.

My tries:

$ python3
Python 3.7.10 (default, Apr  3 2021, 22:19:39)
[Clang 10.0.1 (git@github.com:llvm/llvm-project.git llvmorg-10.0.1-0-gef32c611a on freebsd12
Type "help", "copyright", "credits" or "license" for more information.
>>> import gssapi.raw
>>> gssapi.raw.acquire_cred_from(store={"ccache":"/tmp/krb5cc_sawow2", "client_keytab":"/net/home/osipovmi/keytab"}, usage="initiate")
AcquireCredResult(creds=<gssapi.raw.creds.Creds object at 0x801450850>, mechs={<OID 1.3.6.1.5.5.2>, <OID 1.2.840.113554.1.2.2>}, lifetime=35826)
>>> exit()

Please comment.

Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally still think that utf-8 should be used by default and if the caller requires a different encoding scheme they pass in a byte string that is already encoded as needed. I'll leave that up to @frozencemetery to decide though.

gssapi/raw/ext_cred_store.pyx Outdated Show resolved Hide resolved
@michael-o michael-o marked this pull request as ready for review April 8, 2021 15:33
Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally still think that utf-8 should be used by default and if the caller requires a different encoding scheme they pass in a byte string that is already encoded as needed. I'll leave that up to @frozencemetery to decide though.

I agree with this. Our default encoding is utf-8; callers can switch that with gssapi.set_encoding().

@michael-o
Copy link
Contributor Author

I personally still think that utf-8 should be used by default and if the caller requires a different encoding scheme they pass in a byte string that is already encoded as needed. I'll leave that up to @frozencemetery to decide though.

I agree with this. Our default encoding is utf-8; callers can switch that with gssapi.set_encoding().

But you do understand the implications of the file system encoding with not being not UTF-8?

@michael-o
Copy link
Contributor Author

@frozencemetery Done as requested.

@jborean93
Copy link
Contributor

jborean93 commented Apr 13, 2021

But you do understand the implications of the file system encoding with not being not UTF-8?

On Linux the filesystem isn't even enforced with an encoding. I can easily create a file with invalid UTF-8 sequences as paths are just a sequence of bytes

import os

os.mkdir(b'/tmp/test-folder')

path = b'/tmp/test-folder/test-\xef.txt'
with open(path, mode='wb') as fd:
    fd.write(b'test')

print(os.listdir('/tmp/test-folder')
# ['test-\udcef.txt']

print(os.listdir(b'/tmp/test-folder')
# [b'test-\xef.txt']

When showing the path with bash you get 'test-'$'\357''.txt'. So while these scenarios are rare you can't even rely on a "default" file system encoding. There's no winner in these scenarios unfortunately but the world seems to have settled on UTF-8 being the defacto standard which I think is a sane choice.

@michael-o
Copy link
Contributor Author

@jborean93 I didn't say anything about enforcement, I said that invalid file names, as you have seen in the Bash are possible.

@simo5
Copy link
Contributor

simo5 commented Apr 13, 2021

@michael-o we consider this to be user error. It would be nice if the FS would enforce UTF-8, but it doesn't.
Note that many, many tools by now just expect utf-8 and treats non-utf-8 filenames as a user error, we just do the same.

@michael-o
Copy link
Contributor Author

@michael-o we consider this to be user error. It would be nice if the FS would enforce UTF-8, but it doesn't.
Note that many, many tools by now just expect utf-8 and treats non-utf-8 filenames as a user error, we just do the same.

So would I.

Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As others have pointed out, it's possible to use pretty much arbitrary bytestrings for filenames. If users need to do that, they're welcome to pass bytes objects in instead. So I'm fine with this as it currently stands.

Closes: #242
Signed-off-by: Michael Osipov <michael.osipov@siemens.com>
[rharwood@redhat.com: commit message]
@frozencemetery frozencemetery merged commit a85972e into pythongssapi:main Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Common Values for Credentials Store Extensions documentation is wrong
4 participants