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

Migrate postgresql.py #985

Merged
merged 12 commits into from
Nov 21, 2022
Merged

Migrate postgresql.py #985

merged 12 commits into from
Nov 21, 2022

Conversation

johnmhoran
Copy link
Member

Reference: #969

@johnmhoran
Copy link
Member Author

@TG1999 I've just committed and pushed my latest code (still a w-i-p) with updates to postgresql.py and test_postgresql.py. I'm also uploading 2 related .txt files:

  • 2022-10-28-postgresql.py-to_advisories()-print-statements.txt -- copy of postgresql.py to_advisories() print statements triggered by the skeletal test_to_advisories_simple test
  • 2022-10-28-postgresql.py-to_advisories()-print-statements-advisories-object.txt -- the advisories object from the print statements, plus a manual effort to partially extract/display the structure of the advisories object

@johnmhoran
Copy link
Member Author

@TG1999 Note that I am separating the individual packages (AffectedPackage objects) in each table row by using the count of versions listed in the Affected column, pairing each version with the corresponding version in the Fixed column, and assuming that this pairing is correct and reliable. It does seem a bit "fragile" to me -- perhaps there's a better approach?

@johnmhoran
Copy link
Member Author

@TG1999 I have been unable to find any licensing information on the PostgreSQL security pages. The primary license reference is at https://www.postgresql.org/about/licence/, which identifies the applicable license as "the PostgreSQL License", with a link to https://www.opensource.org/licenses/postgresql.

By its terms, this license covers "this software and its documentation", which could be interpreted to include security-related information within the definition of "documentation". That is the approach I am taking, subject to your input and input from other colleagues. Accordingly, I have updated the license-related info near the top of postgresql.py to read as follows:

    root_url = "https://www.postgresql.org/support/security/"
    license_url = "https://www.postgresql.org/about/licence/"
    spdx_license_expression = "PostgreSQL"

Copy link
Member

@TG1999 TG1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@TG1999
Copy link
Member

TG1999 commented Nov 10, 2022

@johnmhoran the changes look good to me, please add a CHANGELOG entry for same.

@johnmhoran
Copy link
Member Author

@TG1999 On my local branch 969-migrate-postgresql-importer the top of the changelog looks like this:

Version v30.2.2
----------------

- We enabled API throttling for a basic user and for a staff user 
  they can have unlimited access on API.

- We added throttle rate for each API endpoint and it can be 
  configured from the settings #991 https://github.com/nexB/vulnerablecode/issues/991.

Version v30.2.1
----------------

- We refactored and fixed the LaunchPad API code.
- We now ignore qualifiers and subpath from PURL search lookups.
- We fixed severity table column spillover.

In the main branch the top of the changelog looks like this:

Version v30.3.1
This is a minor bug fix release.

We enabled proper CSRF configuration for deployments

Do I add the changelog entry (e.g., We re-enabled support for PostgreSQL securities advisories importer.) under Version v30.2.2, or do I update main, rebase main on my branch, and then add the changelog entry under a new Version v30.3.2 heading (and then force push the commit)? Or something different?

@TG1999
Copy link
Member

TG1999 commented Nov 10, 2022

rebase main on my branch, and then add the changelog entry under a new Version v30.3.2 heading (and then force push the commit)

@johnmhoran please do this.

@johnmhoran johnmhoran force-pushed the 969-migrate-postgresql-importer branch from f14e5c5 to 20e61f3 Compare November 10, 2022 18:22
@johnmhoran
Copy link
Member Author

@TG1999 Updated/pushed as discussed -- all GH checks have passed.

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. See a comment inlined.

# when there are 2 packages one with qualifiers and one without
# qualifiers, having all other fields same, this raises MultipleObjectsReturned
# so we are filling out the fields with empty value to avoid this
for field in PackageURL._fields:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand what you are trying to do with this change. Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when there are 2 packages one with qualifiers and one without qualifiers. For say pkg:generic/postgres?foo=bar and one package pkg:generic/postgres. So when we try to get second package using (type="generic", name="postgres"), it returns 2 packages which raises MultipleObjectsReturned. By declaring qualifiers as an empty dict, we only get the package that doesn't have qualifiers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TG1999 this should not be part of this PR. And I am not sure that problem and the fix are correct

johnmhoran and others added 10 commits November 21, 2022 20:58
Reference: #969

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
Reference: #969

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
Reference: #969

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
Reference: #969

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
Reference: #969

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
Fixed get_or_create_from_purl

Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Reference: #969

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
Reference: #969

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@TG1999 TG1999 force-pushed the 969-migrate-postgresql-importer branch from e000171 to bf048be Compare November 21, 2022 15:51
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you ++ ... I am merging now.

@pombredanne
Copy link
Member

This likely needs a small adjustment for the new CVSS processing

Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@TG1999
Copy link
Member

TG1999 commented Nov 21, 2022

Thanks @johnmhoran for your PR, I have regen the tests and all tests are passing now.

@TG1999 TG1999 merged commit 469d20e into main Nov 21, 2022
@pombredanne pombredanne deleted the 969-migrate-postgresql-importer branch November 28, 2022 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants