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

t.c.checkers.UNIXAuthorizedKeysFiles passes bytes to getpwnam #10286

Closed
twisted-trac opened this issue Dec 27, 2021 · 9 comments
Closed

t.c.checkers.UNIXAuthorizedKeysFiles passes bytes to getpwnam #10286

twisted-trac opened this issue Dec 27, 2021 · 9 comments

Comments

@twisted-trac
Copy link

twm's avatar @twm reported
Trac ID trac#10286
Type defect
Created 2021-12-27 20:26:57Z
Branch https://github.com/twisted/twisted/tree/10286-pwd-checkers-types

Per chat twisted.conch.checkers.UNIXAuthorizedKeysFiles passes bytes to getpwnam, which only accepts str. Appears to be a Python 3 porting/mocking error.

passwd = self._userdb.getpwnam(username)

Similar to #9130

Searchable metadata
trac-id__10286 10286
type__defect defect
reporter__twm twm
priority__normal normal
milestone__None None
branch__10286_pwd_checkers_types 10286-pwd-checkers-types
branch_author__ 
status__closed closed
resolution__fixed fixed
component__conch conch
keywords__None None
time__1640636817513431 1640636817513431
changetime__1653284950572131 1653284950572131
version__None None
owner__twm twm

@twisted-trac
Copy link
Author

twm's avatar @twm set owner to @twm
@twm set status to assigned

@twisted-trac
Copy link
Author

twm's avatar @twm commented

WIP PR here: #1678

@twisted-trac
Copy link
Author

glyph's avatar @glyph commented

ticket:10335 was a duplicate of this, and I made a branch there to fix it as well

@twisted-trac
Copy link
Author

twm's avatar @twm set status to closed

Merged in: 14f36c2

@glyph
Copy link
Member

glyph commented Aug 29, 2022

Just saw this on 22.8.0.rc1, with the equivalent of twist conch --auth=sshkey .

2022-08-29T07:31:45+0000 [twisted.conch.ssh.userauth.SSHUserAuthServer#critical] Error checking auth for user b'user'
	Traceback (most recent call last):
	  File "/code/env/lib/pypy3.8/site-packages/twisted/conch/ssh/userauth.py", line 285, in auth_publickey
	    return self.portal.login(c, None, interfaces.IConchUser)
	  File "/code/env/lib/pypy3.8/site-packages/twisted/cred/portal.py", line 120, in login
	    ).addCallback(self.realm.requestAvatar, mind, *interfaces)
	  File "/code/env/lib/pypy3.8/site-packages/twisted/internet/defer.py", line 531, in addCallback
	    return self.addCallbacks(callback, callbackArgs=args, callbackKeywords=kwargs)
	  File "/code/env/lib/pypy3.8/site-packages/twisted/internet/defer.py", line 511, in addCallbacks
	    self._runCallbacks()
	--- <exception caught here> ---
	  File "/code/env/lib/pypy3.8/site-packages/twisted/internet/defer.py", line 892, in _runCallbacks
	    current.result, *args, **kwargs
	  File "/code/env/lib/pypy3.8/site-packages/twisted/conch/ssh/userauth.py", line 208, in _ebMaybeBadAuth
	    reason.trap(error.NotEnoughAuthentication)
	  File "/code/env/lib/pypy3.8/site-packages/twisted/python/failure.py", line 480, in trap
	    self.raiseException()
	  File "/code/env/lib/pypy3.8/site-packages/twisted/python/failure.py", line 504, in raiseException
	    raise self.value.with_traceback(self.tb)
	  File "/code/env/lib/pypy3.8/site-packages/twisted/internet/defer.py", line 892, in _runCallbacks
	    current.result, *args, **kwargs
	  File "/code/env/lib/pypy3.8/site-packages/twisted/conch/unix.py", line 49, in requestAvatar
	    user = UnixConchUser(username)
	  File "/code/env/lib/pypy3.8/site-packages/twisted/conch/unix.py", line 57, in __init__
	    self.pwdData = pwd.getpwnam(self.username)
	builtins.TypeError: expected str, got bytes object

@glyph glyph reopened this Aug 29, 2022
@glyph glyph removed the fixed label Aug 29, 2022
@adiroiban
Copy link
Member

I guess that since this was reopened, we need to auto revert the merge for the associated PR

That is revert commit 14f36c2

And do the same revert in the release branch.

Is that correct?

@twm
Copy link
Contributor

twm commented Aug 30, 2022

I guess that since this was reopened, we need to auto revert the merge for the associated PR

Are the changes in the patch incorrect, or merely incomplete?

@twm
Copy link
Contributor

twm commented Aug 30, 2022

@glyph This doesn’t look like what I was trying to fix here:

	  File "/code/env/lib/pypy3.8/site-packages/twisted/conch/unix.py", line 57, in __init__
	    self.pwdData = pwd.getpwnam(self.username)
	builtins.TypeError: expected str, got bytes object

Here is the offending line:

self.pwdData = pwd.getpwnam(self.username)

I didn't touch this file. I only fixed UNIXAuthorizedKeysFile.

Incidentally this may have worked before in PyPy (only PyPy) but as part of this project I reported the behavioral discrepancy upstream: https://foss.heptapod.net/pypy/pypy/-/issues/3624 So... you're welcome and please forgive me?

As it doesn't appear that there's evidence that UNIXAuthorizedKeysFile has regressed I'm reclosing this ticket and reopening #10335. I do not see any reason to revert here — fixing the Conch CLI was never the goal.

@twm
Copy link
Contributor

twm commented Aug 30, 2022

See #11626 which I just filed based on @glyph's comment. I don't think this is a release blocker as it has been broken for many releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants