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

Add SBOM protocol buffer definitions #212

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Add SBOM protocol buffer definitions #212

merged 1 commit into from
Jan 10, 2023

Conversation

L3n41c
Copy link
Member

@L3n41c L3n41c commented Dec 19, 2022

Reviewers: please see the review guidelines.

Define the SBOM message to be sent by the agent to the sbom EVP track.

This payload is further described in the “Container images vulnerabilities” RFC.

This PR is built on top of #213.

Generating the upstream CycloneDX .proto file with the current versions of protoc and gogo/protobuf doesn’t work.

  • The .proto file isn’t considered valid. protoc complains about the optional keyword for ex.
    proto/deps/github.com/CycloneDX/specification/schema/bom-1.4.proto:9:12: Explicit 'optional' labels are disallowed in the Proto3 syntax. To define 'optional' fields in Proto3, simply remove the 'optional' label, as fields are 'optional' by default.
    proto/deps/github.com/CycloneDX/specification/schema/bom-1.4.proto:11:12: Explicit 'optional' labels are disallowed in the Proto3 syntax. To define 'optional' fields in Proto3, simply remove the 'optional' label, as fields are 'optional' by default.
    …
    
  • Even if the .proto file is “fixed”, the generated code doesn’t work because the use of “well-know” types like google.protobuf.Timestamp generates use of external GO modules (github.com/golang/protobuf/ptypes/timestamp) that don’t implement the interface expected by the generated code.
    cyclonedx_v1_4/bom-1.4.pb.go:3522:55: m.Timestamp.Size undefined (type *timestamppb.Timestamp has no field or method Size)
    cyclonedx_v1_4/bom-1.4.pb.go:3523:26: m.Timestamp.MarshalTo undefined (type *timestamppb.Timestamp has no field or method MarshalTo)
    …
    

Upgrading the version of the ProtocolBuffer compiler for all the messages doesn’t work because it adds some state, sizeCache and unknownFields google.golang.org/protobuf/runtime/protoimpl unexported fields which are making the generated struct unsuitable for:

  • comparison inside unit tests;
  • use as keys in map[…]…;
    pkg/process/net/resolver/resolver.go:51:18: cannot use *networkAddr (variable of type process.ContainerAddr) as type process.ContainerAddr in map index
    
  • being copied (go vet complains each time a generated struct is copied (either passed by value as a function argument or as a function return value) because one of the protoimpl unexported field contains a sync.Mutex which cannot be copied.
    pkg/network/encoding/encoding.go:97:9: copylocks: range var v copies lock: github.com/DataDog/datadog-agent/pkg/network/encoding.RouteIdx contains github.com/DataDog/agent-payload/v5/process.Route contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)
    	for _, v := range routeIndex {
    	       ^
    pkg/network/encoding/format.go:233:16: copylocks: assignment copies lock value to idx: (github.com/DataDog/datadog-agent/pkg/network/encoding.RouteIdx, bool) contains github.com/DataDog/datadog-agent/pkg/network/encoding.RouteIdx contains github.com/DataDog/agent-payload/v5/process.Route contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)
    	if idx, ok := routes[k]; ok {
    	              ^
    pkg/network/encoding/format_test.go:75:11: copylocks: range var v copies lock: github.com/DataDog/datadog-agent/pkg/network/encoding.RouteIdx contains github.com/DataDog/agent-payload/v5/process.Route contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)
    			for k, v := range te.routesOut {
    			       ^
    pkg/network/encoding/format_test.go:76:19: copylocks: assignment copies lock value to otherv: (github.com/DataDog/datadog-agent/pkg/network/encoding.RouteIdx, bool) contains github.com/DataDog/datadog-agent/pkg/network/encoding.RouteIdx contains github.com/DataDog/agent-payload/v5/process.Route contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)
    				otherv, ok := te.routesIn[k]
    				              ^
    pkg/network/encoding/format_test.go:78:22: copylocks: call of require.Equal copies lock value: github.com/DataDog/datadog-agent/pkg/network/encoding.RouteIdx contains github.com/DataDog/agent-payload/v5/process.Route contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)
    				require.Equal(t, v, otherv, "routes in and out are not equal, expected: %v, actual: %v", te.routesOut, te.routesIn)
    				                 ^
    

That’s why I had to use different version of protoc depending on the message.

proto/sbom/sbom.proto Outdated Show resolved Hide resolved
@L3n41c L3n41c force-pushed the lenaic/sbom branch 4 times, most recently from c037cd0 to f083a7c Compare December 21, 2022 12:43
}

enum SBOMSourceType {
UNSPECIFIED = 0;

Choose a reason for hiding this comment

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

In what cases can it be UNSPECIFIED. Can this be useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s just that 0 is the default value when a field isn’t explicitly set or present in the encoded buffer.
Having a dedicated value for 0 allows to make the distinction between a bug where we forgot to set the field and a functionally relevant value.
I basically only mimicked the samples of the enum documentation.

@L3n41c
Copy link
Member Author

L3n41c commented Dec 22, 2022

I’ve extracted the protoc upgrade in a dedicated PR: #213.

@L3n41c L3n41c force-pushed the lenaic/sbom branch 2 times, most recently from bdadc83 to 51a8dc1 Compare December 23, 2022 14:49
@L3n41c
Copy link
Member Author

L3n41c commented Dec 23, 2022

I’ve rebased this PR on top of #213, which allowed me to undo the hack I had to apply on top of the upstream CycloneDX .proto file to make it accepted by the deprecated gogo/protobuf compiler.
In order to review only the SBOM related changes, you should have a look at ba62413.

@L3n41c L3n41c force-pushed the lenaic/sbom branch 2 times, most recently from 33e814c to ba62413 Compare December 23, 2022 15:07
@L3n41c L3n41c force-pushed the lenaic/sbom branch 3 times, most recently from 14bf007 to 4224ec8 Compare January 2, 2023 15:15
@L3n41c L3n41c marked this pull request as ready for review January 3, 2023 10:07
@L3n41c L3n41c requested a review from a team as a code owner January 3, 2023 10:07
Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

There are a number of things wrong with the repository but none of them are caused by this PR; I know you are just trying to work around the pertinent issues. I have to admit I'm not thrilled about you having to implement all those workarounds/hacks to be able to introduce the sbom definitions, but I understand we really have to tackle addressing all those issues in a separate effort. 😞

@L3n41c L3n41c merged commit 672e97c into master Jan 10, 2023
@L3n41c L3n41c deleted the lenaic/sbom branch January 10, 2023 13:46
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.

3 participants