| Message ID | 6e7024bb-71ec-66e6-252d-283c2a22666b@ipfire.org |
|---|---|
| State | Superseded |
| Headers |
Return-Path: <location-bounces@lists.ipfire.org> Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384 client-signature ECDSA (P-384) client-digest SHA384) (Client CN "mail01.haj.ipfire.org", Issuer "R3" (verified OK)) by web04.haj.ipfire.org (Postfix) with ESMTPS id 4HS6Vw4lBgz44S9 for <patchwork@web04.haj.ipfire.org>; Sun, 10 Oct 2021 16:16:24 +0000 (UTC) Received: from mail02.haj.ipfire.org (mail02.haj.ipfire.org [172.28.1.201]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384 client-signature ECDSA (P-384) client-digest SHA384) (Client CN "mail02.haj.ipfire.org", Issuer "R3" (verified OK)) by mail01.ipfire.org (Postfix) with ESMTPS id 4HS6Vw2wgvzj4; Sun, 10 Oct 2021 16:16:24 +0000 (UTC) Received: from mail02.haj.ipfire.org (localhost [127.0.0.1]) by mail02.haj.ipfire.org (Postfix) with ESMTP id 4HS6Vw1TPnz2xMX; Sun, 10 Oct 2021 16:16:24 +0000 (UTC) Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384 client-signature ECDSA (P-384) client-digest SHA384) (Client CN "mail01.haj.ipfire.org", Issuer "R3" (verified OK)) by mail02.haj.ipfire.org (Postfix) with ESMTPS id 4HS6Vv0k2Wz2xKc for <location@lists.ipfire.org>; Sun, 10 Oct 2021 16:16:23 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by mail01.ipfire.org (Postfix) with ESMTPSA id 4HS6Vs5q82zj4 for <location@lists.ipfire.org>; Sun, 10 Oct 2021 16:16:21 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1633882582; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=K3qFe+dUuQC6uTzqhUIN/JeP3WK8fgTd3UoCSLg9lIo=; b=4yEG6dw5A3fERFBjp+M9j1HbGhxI7Sa4LJCZwmyusCC9CnEquM7bCw/vjesKB/FYNv7ySV XItrh8Nmto/W2/Cg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1633882582; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=K3qFe+dUuQC6uTzqhUIN/JeP3WK8fgTd3UoCSLg9lIo=; b=L5RhZBt/eVhB8TN4jYQGy8LyQtcrbU0MGX32WhI2+Jtd2FQVRgXB8RTjs+wbDbEWzccoe7 QBcuvnZj0Fi/NiZtxrJNuDNrm2qj9MfGQFI7AB5+fl+62PX8WtXPkkcaGpdUtIjtgye9K2 CmpgeYjeeRknffZdRLfg55HINtCMbM55KQHXukD6Jw2IjQXLW8o9JHuWJNPnLtNGlg5te+ jYdwSIS4e4lbjYYvE5gQGR+lPnibfE2TEqDGzMzIK5x9P3l0SyneRD1xhOm2WYlOH6oeDW k/NVey3haryOHl1FwT8y9jLhG9klQDFX/AgfnAFnub9k4DLYa+hRfUef0DhqOQ== To: "IPFire: Location" <location@lists.ipfire.org> From: =?utf-8?q?Peter_M=C3=BCller?= <peter.mueller@ipfire.org> Subject: [PATCH 1/2] location-importer: Introduce auxiliary function to sanitise ASNs Message-ID: <6e7024bb-71ec-66e6-252d-283c2a22666b@ipfire.org> Date: Sun, 10 Oct 2021 18:16:19 +0200 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: location@lists.ipfire.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: <location.lists.ipfire.org> List-Unsubscribe: <https://lists.ipfire.org/mailman/options/location>, <mailto:location-request@lists.ipfire.org?subject=unsubscribe> List-Archive: <http://lists.ipfire.org/pipermail/location/> List-Post: <mailto:location@lists.ipfire.org> List-Help: <mailto:location-request@lists.ipfire.org?subject=help> List-Subscribe: <https://lists.ipfire.org/mailman/listinfo/location>, <mailto:location-request@lists.ipfire.org?subject=subscribe> Errors-To: location-bounces@lists.ipfire.org Sender: "Location" <location-bounces@lists.ipfire.org> |
| Series |
[1/2] location-importer: Introduce auxiliary function to sanitise ASNs
|
|
Commit Message
Peter Müller
Oct. 10, 2021, 4:16 p.m. UTC
Signed-off-by: Peter Müller <peter.mueller@ipfire.org>
---
src/python/location-importer.in | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
Comments
Hello, > On 10 Oct 2021, at 17:16, Peter Müller <peter.mueller@ipfire.org> wrote: > > Signed-off-by: Peter Müller <peter.mueller@ipfire.org> > --- > src/python/location-importer.in | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/src/python/location-importer.in b/src/python/location-importer.in > index da058d3..c2b3e41 100644 > --- a/src/python/location-importer.in > +++ b/src/python/location-importer.in > @@ -574,6 +574,22 @@ class CLI(object): > # be suitable for libloc consumption... > return True > > + def _check_parsed_asn(self, asn): > + """ > + Assistive function to filter Autonomous System Numbers not being suitable > + for adding to our database. Returns False in such cases, and True otherwise. > + """ > + > + if not asn or not isinstance(asn, int): > + return False Does this happen that a non-integer is being passed to this function? You also return False for zero without logging the message. I would suggest to drop the check above. > + > + if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)): > + log.debug("Skipping invalid ASN: %s" % asn) > + return False This works, but I do not consider this very Pythonic. I would have written a tuple which conatins one tuple for each range and then iterate over that until you find a match. > + > + # ASN is fine if we made it here... > + return True Ellipses in comments are sometimes weird... > + > def _parse_block(self, block, source_key, validcountries = None): > # Get first line to find out what type of block this is > line = block[0] > @@ -829,8 +845,8 @@ class CLI(object): > log.debug("Skipping ARIN AS names line not containing an integer for ASN") > continue > > - if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)): > - log.debug("Skipping ARIN AS names line not containing a valid ASN: %s" % asn) > + # Filter invalid ASNs... > + if not self._check_parsed_asn(asn): > continue > > # Skip any AS name that appears to be a placeholder for a different RIR or entity... > -- > 2.26.2
Hello Michael, thanks for your reply. > Hello, > >> On 10 Oct 2021, at 17:16, Peter Müller <peter.mueller@ipfire.org> wrote: >> >> Signed-off-by: Peter Müller <peter.mueller@ipfire.org> >> --- >> src/python/location-importer.in | 20 ++++++++++++++++++-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/src/python/location-importer.in b/src/python/location-importer.in >> index da058d3..c2b3e41 100644 >> --- a/src/python/location-importer.in >> +++ b/src/python/location-importer.in >> @@ -574,6 +574,22 @@ class CLI(object): >> # be suitable for libloc consumption... >> return True >> >> + def _check_parsed_asn(self, asn): >> + """ >> + Assistive function to filter Autonomous System Numbers not being suitable >> + for adding to our database. Returns False in such cases, and True otherwise. >> + """ >> + >> + if not asn or not isinstance(asn, int): >> + return False > > Does this happen that a non-integer is being passed to this function? What's wrong with input validation? I _like_ input validation. :-) Seriously: Anything else than an integer does not make sense for an ASN. Sure, this function is not intended to get anything else, but we will never know. Better to be safe than sorry. > You also return False for zero without logging the message. True. Since there will probably a second version of this patchset, I will ensure it logs anything useful in this case. > I would suggest to drop the check above. Frankly, I don't see why. >> + >> + if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)): >> + log.debug("Skipping invalid ASN: %s" % asn) >> + return False > > This works, but I do not consider this very Pythonic. > > I would have written a tuple which conatins one tuple for each range and then iterate over that until you find a match. Far from being a Python developer, this wouldn't have come to my mind. But if it's Pythonic, I'll do so. When in Rome... > >> + >> + # ASN is fine if we made it here... >> + return True > > Ellipses in comments are sometimes weird... ??? Thanks, and best regards, Peter Müller > >> + >> def _parse_block(self, block, source_key, validcountries = None): >> # Get first line to find out what type of block this is >> line = block[0] >> @@ -829,8 +845,8 @@ class CLI(object): >> log.debug("Skipping ARIN AS names line not containing an integer for ASN") >> continue >> >> - if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)): >> - log.debug("Skipping ARIN AS names line not containing a valid ASN: %s" % asn) >> + # Filter invalid ASNs... >> + if not self._check_parsed_asn(asn): >> continue >> >> # Skip any AS name that appears to be a placeholder for a different RIR or entity... >> -- >> 2.26.2 >
Hi, > On 13 Oct 2021, at 17:33, Peter Müller <peter.mueller@ipfire.org> wrote: > > Hello Michael, > > thanks for your reply. > >> Hello, >> >>> On 10 Oct 2021, at 17:16, Peter Müller <peter.mueller@ipfire.org> wrote: >>> >>> Signed-off-by: Peter Müller <peter.mueller@ipfire.org> >>> --- >>> src/python/location-importer.in | 20 ++++++++++++++++++-- >>> 1 file changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/python/location-importer.in b/src/python/location-importer.in >>> index da058d3..c2b3e41 100644 >>> --- a/src/python/location-importer.in >>> +++ b/src/python/location-importer.in >>> @@ -574,6 +574,22 @@ class CLI(object): >>> # be suitable for libloc consumption... >>> return True >>> >>> + def _check_parsed_asn(self, asn): >>> + """ >>> + Assistive function to filter Autonomous System Numbers not being suitable >>> + for adding to our database. Returns False in such cases, and True otherwise. >>> + """ >>> + >>> + if not asn or not isinstance(asn, int): >>> + return False >> >> Does this happen that a non-integer is being passed to this function? > > What's wrong with input validation? I _like_ input validation. :-) There is nothing wrong with that. You are just checking the developer here and I am not sure whether you want that or not. > Seriously: Anything else than an integer does not make sense for an ASN. Sure, this > function is not intended to get anything else, but we will never know. Better to be > safe than sorry. Not entirely. You want code to perform. If you want to be 100% use, Python isn’t the language this parser should be written in. Nothing else but an integer makes sense. The question is how do you want to treat zero? >> You also return False for zero without logging the message. > > True. Since there will probably a second version of this patchset, I will ensure it > logs anything useful in this case. > >> I would suggest to drop the check above. > > Frankly, I don't see why. > >>> + >>> + if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)): >>> + log.debug("Skipping invalid ASN: %s" % asn) >>> + return False >> >> This works, but I do not consider this very Pythonic. >> >> I would have written a tuple which conatins one tuple for each range and then iterate over that until you find a match. > > Far from being a Python developer, this wouldn't have come to my mind. But if it's > Pythonic, I'll do so. When in Rome... I don’t make the rules. That is just how I would do it: * Data in one place * A short algorithm that works on the data In C I would hope that the compiler makes it fast. > >> >>> + >>> + # ASN is fine if we made it here... >>> + return True >> >> Ellipses in comments are sometimes weird... > > ??? This one left the comment kind of open ended. Making it sound kind of unlikely. > > Thanks, and best regards, > Peter Müller > >> >>> + >>> def _parse_block(self, block, source_key, validcountries = None): >>> # Get first line to find out what type of block this is >>> line = block[0] >>> @@ -829,8 +845,8 @@ class CLI(object): >>> log.debug("Skipping ARIN AS names line not containing an integer for ASN") >>> continue >>> >>> - if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)): >>> - log.debug("Skipping ARIN AS names line not containing a valid ASN: %s" % asn) >>> + # Filter invalid ASNs... >>> + if not self._check_parsed_asn(asn): >>> continue >>> >>> # Skip any AS name that appears to be a placeholder for a different RIR or entity... >>> -- >>> 2.26.2
diff --git a/src/python/location-importer.in b/src/python/location-importer.in index da058d3..c2b3e41 100644 --- a/src/python/location-importer.in +++ b/src/python/location-importer.in @@ -574,6 +574,22 @@ class CLI(object): # be suitable for libloc consumption... return True + def _check_parsed_asn(self, asn): + """ + Assistive function to filter Autonomous System Numbers not being suitable + for adding to our database. Returns False in such cases, and True otherwise. + """ + + if not asn or not isinstance(asn, int): + return False + + if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)): + log.debug("Skipping invalid ASN: %s" % asn) + return False + + # ASN is fine if we made it here... + return True + def _parse_block(self, block, source_key, validcountries = None): # Get first line to find out what type of block this is line = block[0] @@ -829,8 +845,8 @@ class CLI(object): log.debug("Skipping ARIN AS names line not containing an integer for ASN") continue - if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)): - log.debug("Skipping ARIN AS names line not containing a valid ASN: %s" % asn) + # Filter invalid ASNs... + if not self._check_parsed_asn(asn): continue # Skip any AS name that appears to be a placeholder for a different RIR or entity...