From 2f84ab96a56d703cd8e3e69901c0e2b3397bade2 Mon Sep 17 00:00:00 2001 From: "Luke T. Shumaker" Date: Thu, 4 Apr 2024 14:05:40 -0600 Subject: [PATCH 1/2] Properly implement the libravatar federation API I have no strong opinions about whether WebFinger should be supported instead of Libravatar (as suggested in https://github.com/liberapay/liberapay.com/pull/1043), but as long as there's a thing in Liberapay that says "libravatar", it should do what the libravatar API docs say. It is surprising and confusing that Liberapay will tell users "We were unable to get an avatar for you from libravatar" while https://www.libravatar.org/tools/check/ is telling them that their avatar is A-OK. Unlike the old https://github.com/liberapay/liberapay.com/pull/1043, this implements the libravatar API in Liberapay, instead of using the unmaintained (since 2016) pyLibravatar library (which in turn uses the py3dns library, instead of the Dnspython library that Liberapay prefers). --- liberapay/models/participant.py | 65 ++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 2 deletions(-) diff --git a/liberapay/models/participant.py b/liberapay/models/participant.py index fc645de72..4d42b01db 100644 --- a/liberapay/models/participant.py +++ b/liberapay/models/participant.py @@ -6,6 +6,7 @@ from hashlib import pbkdf2_hmac, md5, sha1 from operator import attrgetter, itemgetter from os import urandom +from random import randint from threading import Lock from time import sleep from types import SimpleNamespace @@ -16,6 +17,7 @@ import aspen_jinja2_renderer from cached_property import cached_property from dateutil.parser import parse as parse_date +from dns.resolver import Cache as DNSCache, Resolver as DNSResolver from html2text import html2text from markupsafe import escape as htmlescape from pando import json, Response @@ -98,6 +100,10 @@ email_lock = Lock() +DNS = DNSResolver() +DNS.lifetime = 1.0 # 1 second timeout, per https://github.com/liberapay/liberapay.com/pull/1043#issuecomment-377891723 +DNS.cache = DNSCache() + class Participant(Model, MixinTeam): @@ -2176,8 +2182,63 @@ def update_avatar(self, src=None, cursor=None, avatar_email=None, check=True): if platform == 'libravatar' or platform is None and email: if not email: return - avatar_id = md5(email.strip().lower().encode('utf8')).hexdigest() - avatar_url = 'https://seccdn.libravatar.org/avatar/'+avatar_id + # https://wiki.libravatar.org/api/ + # + # We only use the first SRV record that we choose; if there is an + # error talking to that server, we give up, instead of retrying with + # another record. pyLibravatar does the same. + normalized_email = email.strip().lower() + try: + # Look up the SRV record to use. + _, email_domain = normalized_email.rsplit('@', 1) + try: + srv_records = DNS.query('_avatars-sec._tcp.'+email_domain, 'SRV') + scheme = 'https' + except Exception: + srv_records = DNS.query('_avatars._tcp.'+email_domain, 'SRV') + scheme = 'http' + # Filter down to just the records with the "highest" `.priority` + # (lower number = higher priority); for the libravatar API tells us: + # + # > Libravatar clients MUST only consider servers listed in the + # > highest SRV priority. + top_priority = min(rec.priority for rec in srv_records) + srv_records = [rec for rec in srv_records if rec.priority == top_priority] + # Of those, choose randomly based on their relative `.weight`s; + # for the libravatar API tells us: + # + # > They MUST honour relative weights. + # + # RFC 2782 (at the top of page 4) gives us this algorithm for + # randomly selecting a record based on the weights: + srv_records.sort(key=lambda rec: rec.weight) # ensure that .weight=0 recs are first in the list + weight_choice = randint(0, sum(rec.weight for rec in srv_records)) + weight_sum = 0 + for rec in srv_records: + weight_sum += rec.weight + if weight_sum >= weight_choice: + choice_record = rec + break + + # Validate. + # The Dnspython library has already validated that `.target` is + # a valid DNS name and that `.port` is a uint16. + host = choice_record.target.canonicalize().to_text(omit_final_dot=True) + port = choice_record.port + if port == 0: + raise ValueError("invalid port number") + + # Build `avatar_origin` from that. + # Only include an explicit port number if it's not the default + # port for the scheme. + if (scheme == 'http' and port != 80) or (scheme == 'https' and port != 443): + avatar_origin = '%s://%s:%d' % (scheme, host, port) + else: + avatar_origin = '%s://%s' % (scheme, host) + except Exception: + avatar_origin = 'https://seccdn.libravatar.org' + avatar_id = md5(normalized_email.encode('utf8')).hexdigest() + avatar_url = avatar_origin + '/avatar/' + avatar_id avatar_url += AVATAR_QUERY elif platform is None: From 202b69dcbd82ae2f1482ae2d7fc8341a8914edd0 Mon Sep 17 00:00:00 2001 From: Charly C Date: Tue, 9 Apr 2024 11:23:40 +0200 Subject: [PATCH 2/2] improve error handling + explain port zero + various tweaks --- liberapay/models/participant.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/liberapay/models/participant.py b/liberapay/models/participant.py index 4d42b01db..ac37fa914 100644 --- a/liberapay/models/participant.py +++ b/liberapay/models/participant.py @@ -17,6 +17,7 @@ import aspen_jinja2_renderer from cached_property import cached_property from dateutil.parser import parse as parse_date +from dns.exception import DNSException from dns.resolver import Cache as DNSCache, Resolver as DNSResolver from html2text import html2text from markupsafe import escape as htmlescape @@ -2188,9 +2189,10 @@ def update_avatar(self, src=None, cursor=None, avatar_email=None, check=True): # error talking to that server, we give up, instead of retrying with # another record. pyLibravatar does the same. normalized_email = email.strip().lower() + avatar_origin = 'https://seccdn.libravatar.org' try: # Look up the SRV record to use. - _, email_domain = normalized_email.rsplit('@', 1) + email_domain = normalized_email.rsplit('@', 1)[1] try: srv_records = DNS.query('_avatars-sec._tcp.'+email_domain, 'SRV') scheme = 'https' @@ -2211,7 +2213,7 @@ def update_avatar(self, src=None, cursor=None, avatar_email=None, check=True): # # RFC 2782 (at the top of page 4) gives us this algorithm for # randomly selecting a record based on the weights: - srv_records.sort(key=lambda rec: rec.weight) # ensure that .weight=0 recs are first in the list + srv_records.sort(key=attrgetter('weight')) # ensure that .weight=0 recs are first in the list weight_choice = randint(0, sum(rec.weight for rec in srv_records)) weight_sum = 0 for rec in srv_records: @@ -2220,23 +2222,25 @@ def update_avatar(self, src=None, cursor=None, avatar_email=None, check=True): choice_record = rec break - # Validate. + # Build the `avatar_origin` URL. # The Dnspython library has already validated that `.target` is # a valid DNS name and that `.port` is a uint16. host = choice_record.target.canonicalize().to_text(omit_final_dot=True) port = choice_record.port if port == 0: - raise ValueError("invalid port number") - - # Build `avatar_origin` from that. - # Only include an explicit port number if it's not the default - # port for the scheme. - if (scheme == 'http' and port != 80) or (scheme == 'https' and port != 443): + # Port zero isn't supposed to be used and typically can't be. The + # Libravatar wiki doesn't clearly specify what to do in this case. + pass + elif (scheme == 'http' and port != 80) or (scheme == 'https' and port != 443): + # Only include an explicit port number if it's not the default + # port for the scheme. avatar_origin = '%s://%s:%d' % (scheme, host, port) else: avatar_origin = '%s://%s' % (scheme, host) - except Exception: - avatar_origin = 'https://seccdn.libravatar.org' + except DNSException: + pass + except Exception as e: + website.tell_sentry(e) avatar_id = md5(normalized_email.encode('utf8')).hexdigest() avatar_url = avatar_origin + '/avatar/' + avatar_id avatar_url += AVATAR_QUERY