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

TypeError: __str__ returned non-string (type bytes) #853

Closed
franciscouzo opened this Issue Nov 26, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@franciscouzo
Copy link
Contributor

franciscouzo commented Nov 26, 2016

When using Python 3, str(key) throws a TypeError exception:

>>> key = paramiko.RSAKey(filename="id_rsa_paramiko")
>>> str(key)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __str__ returned non-string (type bytes)

I only tried with RSAKey, but looking at the source code it looks like all PKey subclasses have the same problem.

@arpit1997

This comment has been minimized.

Copy link

arpit1997 commented Nov 29, 2016

It seems like a small mistake in the code to me:

    def asbytes(self):
        m = Message()
        m.add_string('ssh-rsa')
        m.add_mpint(self.public_numbers.e)
        m.add_mpint(self.public_numbers.n)
        return m.asbytes()

    def __str__(self):
        return self.asbytes()

here it returns bytes instead of strings even in __str__ function. It seems odd to me
🛩 😸

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Nov 29, 2016

Yea that sounds like something which was only tested under Python 2, wonder if our test suite covers that particular method. Thanks for the report! Figure at the very least it can be fixed with a quick if PY2 test, but I'd also want to doublecheck exactly what the semantics of __str__ are under Python 3 vs 2. Not that I expect it to be anything besides "return unicode plz"...

@bitprophet bitprophet added this to the 1.16.4 et al milestone Nov 29, 2016

@bitprophet bitprophet modified the milestones: 1.17.3 et al, 1.17.4 etc Dec 9, 2016

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 21, 2017

Side note, I've also always wondered why asbytes is even considered a useful __str__ for a key at all. Why not its public key text, or even better, a standard colon-separated hex-y fingerprint? But that's for another ticket because changing it that much is backwards incompatible...

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 21, 2017

Also, looking, this is actually a dupe of #484. This ticket has more in it now, so I'll roll it in here instead of vice versa.

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 21, 2017

I have a test that proves the issue, so there's that.

Of course, now we're back in the usual problem space of "...so how should we string these bytes?". I squinted at this for way too long before realizing that if we want to be backwards compat we just...return str(self.asbytes()). I.e. the issue was the 'protocol violation' of a __str__ returning bytes - not the actual, literal attempt to turn the bytes object into a (Unicode) string. As long as nobody explicitly tries .decode() on the bytes (which won't work) things appear happy.

Also, #726 is the ticket for "let's make this return something more useful in 3.0". I'll note that the behavior of this method is unchanged since 2003.

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 21, 2017

Hm nope, I'm still dumb. That's getting us the string of the repr for whatever reason (i.e. "b'\x00\x00...'" instead of '\x00\x00...'). Definitely something obvious I'm missing, will keep poking and feeling this impostor syndrome.

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 21, 2017

Think we have only two reasonable options:

  • Go for literal equality between Python 2 and Python 3, meaning a string mixing backslash-escape hex and ascii or utf8 string data. This is what I was trying to get (mostly because I wanted to compare to a constant string value in the test file) but it seems unrealistic without doing dumb shit.
  • Go for logical equivalence, meaning effectively, repr(key.asbytes()). These do not compare equally between the interpreters because the Python 3 edition has a leading b. But as it stands the return value of this method isn't intended to be any sort of comparable value, and anyways, we're talking two different interpreters here. The only thing that would unify them is our test suite, and I can just plug in two different values instead, I guess.

It's tempting to just say "fuckit, this type change is allowable in a feature release", bump this to e.g 1.9/2.2, and do #726 early...sigh. (OTOH I'm trying to get away from pre-2.0 feature releases now, so...eh.)

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 21, 2017

Hrm that still feels dumb because it's still quite different from the old Python 2 behavior :(

Poked some more, and interestingly, self.asbytes().decode('utf8', errors='ignore') results in an identical output (to the pre-patch Python 2 behavior) when printed even though the repr()s of the resulting strings differ in spots. I don't entirely grok why that would be but I'm kinda past caring at this point, again given how pointless this all is for anything serious.

So I may just go for that for now.

bitprophet added a commit that referenced this issue Feb 21, 2017

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Feb 21, 2017

Test happy, manual fuckery happy, Travis-CI happy, we are now exiting the rabbit hole.

@bitprophet bitprophet modified the milestone: 1.18.3 etc Feb 21, 2017

@franciscouzo

This comment has been minimized.

Copy link
Contributor Author

franciscouzo commented Mar 22, 2017

Just a minor nitpick, but it looks like the fix was applied only to RSAKey, I think it would be better to move the __str__ implementation to PKey, so all Pkey subclasses can benefit from it.

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Mar 22, 2017

Yea the structure of the key classes is definitely not how I would have done it and there's at least some cleanup that wants doing. We'll get there sometime. Other things are currently higher priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.