-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add Apache HTTPD advisory importer #261
Conversation
Fixes #99 |
Signed-off-by: Shivam Sandbhor <[email protected]>
bc94350
to
4b89ef1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See a few nits inline.
Looking great otherwise!
|
||
if info.tag == "fixed": | ||
resolved_packages.append( | ||
PackageURL(name="httpd", version=info.attrib["version"], type="generic") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a nit but I prefer putting args in their original order, e.g. type
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we may want to have something more distinctive:
- what about a namespace or
apache
orapache.org
? - or/and a qualifier for
download_url
? - or if there are more than just httpd, we could create an
apache
type for all Apache Foundation projects? after all they (all Apache projects) share many characteristics that would make them Package URL-type worthy
See https://svn.apache.org/repos/asf/comdev/projects.apache.org/trunk/data/projects.xml for instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the namespaces, and qualifiers seems fine.
About creating apache
type, that I don't think is a good idea because that would end up creating ambiguity in case an apache package is packaged, eg apache-httpd.rpm
. What would then be package type of apache-httpd.rpm
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would then be package type of apache-httpd.rpm ?
That's always an rpm
type then.
The rationale is the format of packaging and metadata determines the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using type=apache name=httpd, version=x
# Visit https://github.com/nexB/vulnerablecode/ for support and download. | ||
|
||
from dataclasses import dataclass | ||
import xml.etree.ElementTree as ET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of a shorter ET import?
IMHO for readability and clarity I would use from import xml.etree import ElementTree
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of a shorter ET import?
Nothing.
IMHO for readability and clarity I would use from import xml.etree import ElementTree instead
That works.
FWIW using the alias ET
is a common practice. The docs used it https://docs.python.org/3/library/xml.etree.elementtree.html .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ElementTree
return [] | ||
|
||
def create_etag(self, url): | ||
etag = requests.head(url).headers.get("ETag") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI, at some point of time, we should review together the etag story and why this is used in parallel of https://github.com/nexB/vulnerablecode/pull/261/files#diff-941e9f048d96b3af4998962020f96430R105 and not with it?
I would like to make sure that I grok the caching approach alright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
The gist of it is: create_tag
uses a HEADER
request to just fetch the etag. If this etag matches with old etag we stop the process because the data hasn't changed. Otherwise use https://github.com/nexB/vulnerablecode/pull/261/files#diff-941e9f048d96b3af4998962020f96430R105 which is just normal download.
|
||
if info.tag == "affects" or info.tag == "maybeaffects": | ||
impacted_packages.append( | ||
PackageURL(name="httpd", version=info.attrib["version"], type="generic") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit as above wrt. args order and Apache.
Signed-off-by: Shivam Sandbhor <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for your patience!
Signed-off-by: Shivam Sandbhor [email protected]