-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update go libraries to OPEV-0014, OPEV-0015 and spec #45
Conversation
pkg/vex/product.go
Outdated
Component | ||
} | ||
|
||
type Algorithm string |
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.
to prevent mismatch in caps and convention, do we want to have this match IANA, and have const for each of them?
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.
I added constants for the sw identifiers and some hash algorithms. Do you mean this list of registered algorithms? (because it seems kind of outdated update I was looking at another one)
// Version is the document version. It must be incremented when any content | ||
// within the VEX document changes, including any VEX statements included within | ||
// the VEX document. | ||
Version string `json:"version"` | ||
Version int `json:"version"` |
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.
since we're making some breaking changes, should we introduce the VEX spec version as well? openvex/spec#20
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.
I had added the version but I think I lost the commit at some point. It's back! Documents now have the spec context versioned:
go run . create "pkg:apk/wolfi/[email protected]?arch=x86_64" CVE-2022-39260 fixed
{
"@context": "https://openvex.dev/ns/0.1.0",
"@id": "https://openvex.dev/docs/public/vex-d1d09738f646225c75aa64ef57b5c669947b6d5b1ad2ffa03816eba43e96e372",
"author": "Unknown Author",
"timestamp": "2023-07-13T18:36:48.978656084-06:00",
"version": 1,
"statements": [
{
"vulnerability": {
"name": "CVE-2022-39260"
@@ -16,25 +16,30 @@ import ( | |||
// [vul_id] for one or more [product_id]s. A VEX Statement exists within a VEX | |||
// Document. | |||
type Statement struct { | |||
// ID is an optional identifier for the statement. It takes an IRI and must | |||
// be unique for each statement in the document. | |||
ID string `json:"@id,omitempty"` |
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.
would that be a convention of the IRI for a statement within a document? Or would one have to have uniqueness through the tuple of the parent document and the statement ID?
It would be nice to have it be a postfix kind of like SPDX namespace + elements.
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.
When defined, the IRI must be unique. Not only for the document but universally. Relative IRIs give you that "namespace" effect:
For example, supposing a document is retrieved from the document node id IRI, both statements here would have the same identifier:
{
"@context": "https://openvex.dev/ns/0.1.0",
"@id": "https://openvex.dev/docs/public/mydoc/",
"author": "Unknown Author",
"timestamp": "2023-07-13T18:36:48.978656084-06:00",
"version": 1,
"statements": [
{ "@id": "https://openvex.dev/docs/public/mydoc/mystatement" } ,
{ "@id": "mystatement" }
]
}
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.
gotcha - sounds good, would it be helpful to add a requirement that the statment IDs should have the prefix of the document IDs?
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.
I don't think we should. Statements can use the short form IRI's but may also be an IRI of a statement living in another document, even potentially in another format. While we're not supporting these use cases in our tooling just now, I think we should not break the underlying json-ld fundamentals.
b62bc42
to
90d350d
Compare
This commit introduces to go-vex the required changes to implement OPEV-0014: It introduces a new Component type that is the base of the new expanded product and subomponent types. It also modifies the cannonical hash algorith to add the new fields in the component data to the cstring that is used to compute the hash. It also updates the tests where required. Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit implements the changes in OPEV-0015. The change expands the vulnerability field to a full object to allow having more aliases. Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo Garcia Veytia (puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit implements the changes to the spec done to update OpenVEX to the published version of the minimum elements for VEX document. These changes were proposed to the spec in: openvex/spec#25 Specifically, this commit introduces the following changes: Document - version is now an integer - Added last_updated to record changes to the doc - author is now recommended to be a machine-readable identifier - role is now optional Statement - @id takes an IRI to identify the statement and make it referenceable - version an integer field, accounts for changes in the statement. Defaults to zero, and may be omitted when so. - last_updated added to the statement - supplier an identifier of the supplier of the document Signed-off-by: Adolfo Garcia Veytia (puerco) <[email protected]>
This commit adds constants for commonly used software identifiers and hash algorithms. Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
@@ -16,25 +16,30 @@ import ( | |||
// [vul_id] for one or more [product_id]s. A VEX Statement exists within a VEX | |||
// Document. | |||
type Statement struct { | |||
// ID is an optional identifier for the statement. It takes an IRI and must | |||
// be unique for each statement in the document. | |||
ID string `json:"@id,omitempty"` |
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.
gotcha - sounds good, would it be helpful to add a requirement that the statment IDs should have the prefix of the document IDs?
pkg/vex/vex.go
Outdated
if bytes.Contains(data, []byte(Context+"/"+SpecVersion)) { | ||
logrus.Info("opening current vex") | ||
return Parse(data) | ||
} else if bytes.Contains(data, []byte(Context)) { |
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.
I think it's possible that this bytes.Contains
check isn't as precise as we'd want, and as lib consumers would want. I'd suggest we try parsing in a data-structure-aware way to grab the context, and do an exact match on its value.
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.
I'd like to avoid parsing the whole document and instead do it this way to detect the OpenVEX version early to only open against the spec once. I can strengthen the regex if that is better. Also, if we can follow up on this one I would really appreciate it (to unlock the other PRs) 🙏
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.
Yeah we can make this a follow-up — but this one's important to address. bytes.Contains
is logically problematic because it'll find the string anywhere in the data, including in other field values.
I'd be curious to learn more about your concern with parsing. In the existing code, it looks like you've already read the entire file into memory by the time you're doing the check.
On top of that, bytes.Contains
will search through the byte slice (the entire file, here) until it finds the search string. And since you have two calls to bytes.Contains
here, you'll end up searching the whole file twice if the string can't be found. If you go the parsing route, you can parse just once, and then check the context's field's value as many times as you want afterwards.
One note is that it's possible to do a "partial parse", too. So you could do a preliminary parse using a struct that only has the field you want to examine. Then you can check its value how you want, and come back and re-parse with the full struct once you know the exact spec to which the data must now conform.
Last thought: As I think about this, I'm concerned about the security implications of doing this parsing in an imprecise manner. It shouldn't be possible for someone to put a context string into the body of a VEX statement and completely recolor how the VEX document is interpreted by a security tool. (I don't see this as a likely attack, but to me it's a hint that data isn't being handled properly).
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.
@puerco, assuming this isn't too much work, this does sound like it could be worth addressing here in this PR. If not, another PR ASAP should probably handle this.
I too am curious about the performance concern you bring up with a parsing. Perhaps a timing test or two could help inform this discussion? More generally, I'm not sure it's worth focusing on performance speed at this point in go-vex
's lifecycle. That can come later :)
The partial parse option sounds promising.
And I do think that the security implication should be taken seriously, as least as a heuristic.
Thanks, @luhring, for the careful review!
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.
I merged this one as it is to unlock the rest of the incoming PRs, I'll follow up with this 👍
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.
@@ -266,3 +278,23 @@ func TestOpenCSAF(t *testing.T) { | |||
require.Len(t, doc.Statements, tc.len) | |||
} | |||
} | |||
|
|||
func TestOpen(t *testing.T) { |
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.
Should we have a test for the new, updated version of the spec? Including an instructive JSON example of the current spec would also be helpful for orienting to what's expected in the current spec, too
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.
Good call, I've added a document to the test fixtures and added the test case her and a new (simple while I write a conformance suite) to the Parse()
function.
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.
Okay, great! I think that'll be really nice to have. Over time, it'd be nice to expand the test data to cover more of the spec, too.
This commit adds a new SpecVersion constant and uses it to generate the context url to start versioning the specification used when creating the documents. Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit adds a compatibility parser to the vex.Open function to support opening older openvex document. This is needed to avoid breaking users working with older documents in vexctl and other clients. Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
e2ea383
to
28f9738
Compare
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
@luhring thanks for the review. I've gone through the comments on the PR, I've also noticed that this PR was referencing v0.1.0 of the spec while the rest of the PRs are targeting v0.2.0. I've changed this one to refer to the new version of the spec as v0.2.0 to match the rest of PRs implementing the spec changes. PTAL! |
Thanks everybody! |
This pull request updates the go libraries to implement the first revision to the OpenVEX spec: