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

feat(python): add pypa support #96

Closed

Conversation

AndreyLevchenko
Copy link

No description provided.

pypa/pypa.go Outdated
}
type option func(*options)

type Pypa struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type Pypa struct {
type PyPA struct {

According to their website, PyPA is correct.
https://www.pypa.io/en/latest/

Copy link
Author

Choose a reason for hiding this comment

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

fixed

pypa/pypa.go Outdated
type options struct {
url string
dir string
retry int
Copy link
Collaborator

Choose a reason for hiding this comment

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

retry seems to be unused.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

pypa/pypa.go Outdated
bar := pb.StartNew(len(yamlFiles))

for _, file := range yamlFiles {
data, err := ioutil.ReadFile(file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ioutil is deprecated

Suggested change
data, err := ioutil.ReadFile(file)
data, err := os.ReadFile(file)

Copy link
Author

Choose a reason for hiding this comment

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

fixed

}

dir := t.TempDir()
c := NewPypa(WithURL(ts.URL+"/"+tt.inputArchive), WithDir(filepath.Join(dir)), WithRetry(0))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have to pass afero.NewMemMapFs() and need to use the filesystem below instead of os.ReadDir and os.ReadFile.

Copy link
Author

Choose a reason for hiding this comment

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

based on our chat we decided not to do it, because of go-getterincompatibility

@@ -0,0 +1,78 @@
package pypa
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
package pypa
package pypa_test

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 55 to 57
} else {
require.NoError(t, err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} else {
require.NoError(t, err)
}
}
require.NoError(t, err)

Copy link
Author

Choose a reason for hiding this comment

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

fixed

main.go Outdated
case "pypa":
p := pypa.NewPypa()
if err := p.Update(); err != nil {
return xerrors.Errorf("error in Pypa update: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return xerrors.Errorf("error in Pypa update: %w", err)
return xerrors.Errorf("error in PyPA update: %w", err)

Copy link
Author

Choose a reason for hiding this comment

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

fixed

main.go Outdated
if err := p.Update(); err != nil {
return xerrors.Errorf("error in Pypa update: %w", err)
}
commitMsg = "Pypa Security Advisories"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
commitMsg = "Pypa Security Advisories"
commitMsg = "PyPA Security Advisories"

Copy link
Author

Choose a reason for hiding this comment

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

fixed

pypa/pypa.go Outdated
return xerrors.Errorf("failed to download %s: %w", pypa.opts.url, err)
}

vulnDir := filepath.Join(dir, "advisory-db-main/vulns")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
vulnDir := filepath.Join(dir, "advisory-db-main/vulns")
vulnDir := filepath.Join(dir, "advisory-db-main", "vulns")

Copy link
Author

Choose a reason for hiding this comment

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

fixed

pypa/pypa.go Outdated
return xerrors.Errorf("unable to parse yaml %s: %w", file, err)
}

if err := utils.WriteJSON(pypa.AppFs, pypa.opts.dir, fmt.Sprintf("%s.json", osv.Id), osv); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep the directory structure as much as possible. It means we will have package dirs.
https://github.com/pypa/advisory-db/tree/main/vulns

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@AndreyLevchenko AndreyLevchenko marked this pull request as ready for review August 26, 2021 10:31

const (
pypaDir = "pypa"
securityTrackerURL = "https://github.com/pypa/advisory-db/archive/refs/heads/main.zip"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AndreyLevchenko Could you check if we can use the above URL?

Copy link
Author

Choose a reason for hiding this comment

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

@knqyf263 I think I can use the url above, but I need to understand benefits better. For now it looks pretty same to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The benefit is we can have a single implementation for some languages (Python, Go and Rust).

@patryk4815
Copy link
Contributor

I think we should change this PR and #83
Into one implementation for handling osv as datasource (like ghsa target):
https://github.com/google/osv/blob/master/README.md#current-data-sources
They support multiple source (python/go/etc) with same schema.

@knqyf263
Copy link
Collaborator

knqyf263 commented Sep 4, 2021

@patryk4815 It sounds like a nice idea. Go data source in OSV used to have an issue (golang/vulndb#1) and we needed to parse the repository ourselves. But after having a quick look, I found they might change API. Let me look into OSV carefully. If we can implement osv, it would be better. Thanks.

@patryk4815
Copy link
Contributor

@knqyf263 Maybe we need wait for "purl" support - google/osv.dev#64

@CLAassistant
Copy link

CLAassistant commented Sep 14, 2021

CLA assistant check
All committers have signed the CLA.

@westonsteimel
Copy link

@knqyf263, the OSV schema specification should at least be stable now per https://ossf.github.io/osv-schema/#status---2021-09-08. Let me know if there are any issues you're experiencing from the OSV API side and I can try to take a look. I've put quite a significant effort into triage for the PyPA Advisory DB, so definitely keen to help where I can to see it used.

@knqyf263
Copy link
Collaborator

@westonsteimel Thanks! It looks like PyPA Advisory DB works well for us. We have a problem with Go.

@knqyf263
Copy link
Collaborator

In favor of #113

@knqyf263 knqyf263 closed this Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants