PR#1498: Pass bytes to md5_constructor - koji

koji

#1498 Pass bytes to md5_constructor

Merged 4 years ago by mikem. Opened 4 years ago by mjshi.

mjshi/koji issue/1482 into master

encode to bytes only under py3

Tomas Kopecek • 4 years ago

b1865d2

Pass bytes to md5_constructor

Mengjing Shi • 4 years ago

b0a170c

www/kojiweb/index.py

file modified

+8 -4

		`@@ -55,8 +55,10 @@`
		`value = user + ':' + str(int(time.time()))`
		`if not options['Secret'].value:`
		`raise koji.AuthError('Unable to authenticate, server secret not configured')`
		`- shasum = sha1_constructor(value)`
		`- shasum.update(options['Secret'].value)`
		`+ digest_string = value + options['Secret'].value`
		`+ if six.PY3:`
		`+ digest_string = digest_string.encode('utf-8')`
		`+ shasum = sha1_constructor(digest_string)`
		`value = "%s:%s" % (shasum.hexdigest(), value)`
		`cookies = six.moves.http_cookies.SimpleCookie()`
		`cookies['user'] = value`
		`@@ -92,8 +94,10 @@`
		`sig, value = parts`
		`if not options['Secret'].value:`
		`raise koji.AuthError('Unable to authenticate, server secret not configured')`
		`- shasum = sha1_constructor(value)`
		`- shasum.update(options['Secret'].value)`
		`+ digest_string = value + options['Secret'].value`
		`+ if six.PY3:`
		`+ digest_string = digest_string.encode('utf-8')`
		`+ shasum = sha1_constructor(digest_string)`
		`if shasum.hexdigest() != sig:`
		`authlogger.warn('invalid user cookie: %s:%s', sig, value)`
		`return None`

www/lib/kojiweb/util.py

file modified

+4 -1

		`@@ -167,7 +167,10 @@`
		`return ''`
		`if tstamp == None:`
		`tstamp = _truncTime()`
		`- return md5_constructor(user + str(tstamp) + environ['koji.options']['Secret'].value).hexdigest()[-8:]`
		`+ value = user + str(tstamp) + environ['koji.options']['Secret'].value`
		`+ if six.PY3:`
		`+ value = value.encode('utf-8')`
		`+ return md5_constructor(value).hexdigest()[-8:]`

		`def _getValidTokens(environ):`
		`tokens = []`

mjshi commented 4 years ago

Fixes login TypeError on koji-web

mjshi commented 4 years ago

I'm not sure if we should hardcode 'utf-8' in, but PR #1489 doesn't fully fix #1486/#1482 because there's still an instance of md5_constructor running around without a utf-8 encode. @tkopecek ?

julian8628 commented 4 years ago

:thumbsup:

tkopecek commented 4 years ago

Yes, you're right, I've missed it. utf-8 seems to me ok. Theoretically 'ascii' should be enough, but utf-8 seems to be ok for me.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

4 years ago

mikem commented 4 years ago

This looks like it could fail under python2.

user comes from environ. Looks like this will be a plain str under py2
we're converting tstamp to a plain str, though that should always be ascii
it looks like environ['koji.options']['Secret'].value will also be a plain str under py2

Either the user value or the secret value could contain non-ascii characters. Calling .encode() on such a string in python2 will raise an error.

>>> s = 'həllo'
>>> s.encode('utf8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc9 in position 1: ordinal not in range(128)

Edited 4 years ago by mikem

mikem commented 4 years ago

The above also likely applies to #1489.

tkopecek commented 4 years ago

If it is passed this way, it means, that it is already converted to utf-8.

s1 = 'həllo'
s2 = u'həllo'
s1 == s2.encode('utf-8')
True

I think it should never happen, but anyway, I've added a commit to encode just under PY3 - it doesn't hurt: https://pagure.io/fork/tkopecek/koji/commits/pr1498-fix

Edited 4 years ago by tkopecek

mjshi commented 4 years ago

Thanks tkopecek! I'll try to cherry-pick your commit over. Having some issues cloning over SSH right now (even though I added my ssh-keys in, and I can't push when I clone via HTTPS). Worst-case I'll add you as a contributor to my fork, or you can just submit a PR with your branch instead.

Edited 4 years ago by mjshi

1 new commit added