Consume changes go-mod-core-contracts v0.1.56

[jdharms]

Updates to edgex-go to consume changes to go-mod-core-contracts
from the following PRs:

edgexfoundry/go-mod-core-contracts#233

edgexfoundry/go-mod-core-contracts#231

This is being clubbed into one PR on the edgex-go side because both
PRs on go-mod-core-contracts have already been merged.

Fixes #2439 and Fixes #2480 in edgex-go.

Replaces ugorji/go with fxamacker/cbor for security reasons.

Reflects the system management client functionality being
moved from the "general" client in go-mod-core-contracts
to an "agent" client.

Signed-off-by: Daniel Harms [email protected]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://wiki.edgexfoundry.org/display/FA/Contributor%27s+Guide

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

Issue Number:

#2439 #2480

Does this PR introduce a breaking change?

• Yes
• No

[bnevis-i]

LGTM. Note: github.com/fxamacker/cbor has only two maintainers.

[x448]

@jdharms in the old code you didn't modify, the call to ioutil.ReadAll(reader) can lead to denial of service attacks using large payloads (if another program is providing the data, etc.)

edgex-go/internal/core/data/io.go

Lines 72 to 76 in 5214d25

	// Read reads and converts the request's CBOR event data into an Event struct
	func (cr cborReader) Read(reader io.Reader, ctx *context.Context) (models.Event, error) {
	c := context.WithValue(*ctx, clients.ContentType, clients.ContentTypeCBOR)
	event := models.Event{}
	bytes, err := ioutil.ReadAll(reader)

It would be safer to use io.LimitReader with a reasonable size limit appropriate for edgex-go and then pass the limited reader directly to the CBOR decoder.

Even without io.LimitReader, it's still safer to pass io.Reader directly to the decoder.

// optionally use io.LimitReader r with size limit appropriate to edgex-go
... 
decoder := cbor.NewDecoder(reader)  // decoder will read in up to 512 byte chunks
err := decoder.Decode(&event)

I only looked at one function and didn't review other parts of io.go.

[x448]

Note: github.com/fxamacker/cbor has only two maintainers.

@bnevis-i by comparison, the CBOR library used by edgex-go has zero contributors based on commit activity since August 15, 2019. For context, security problems were found in Sep 2019.

If small team size (without context) is an indicator of risk, take a look at a 27 contributor CBOR library written in Rust looking for maintainers in part because the maintainer says,

Within the past year it became increasingly clear to me that I don't understand all the details and interactions of different features. This is in part because I accepted pull requests implementing new features but did not review the code thoroughly enough.

To avoid such problems, fxamacker/cbor won't accept pull requests if it harms maintainability. In fact, oasislabs/oasis-core cited maintainability as a factor in choosing fxamacker/cbor.

(click to expand) Comparison Charts

Number of contributors

Contributor counts (based on commits) from Aug 15, 2019 - Apr 8, 2020 are:

• 2 contributors -- fxamacker/cbor (proposed new CBOR library)
• 0 contributors -- ugorji/go (current CBOR library used by edgex-go)

Graphs showing activity from Aug 11, 2019 - Apr 8, 2020:

For fxamacker/cbor:

For ugorji/go:

Source:

• https://github.com/fxamacker/cbor/graphs/contributors?from=2019-08-11&to=2020-04-08&type=c
• https://github.com/ugorji/go/graphs/contributors?from=2019-08-11&to=2020-04-08&type=c

[mkbhanda]

I agree with both comments @x448 has made. Please use a limited reader. Also consider making the limit a constant that we could possibly configure, defaulting to something we think sensible today. If you like, could make this a separate issue. For the future, do keep patches separate for different issues, easier on the reviewers.

[mkbhanda]

Does it make sense to:
s/general.GeneralClient/agent.AgentClient

[jdharms]

Yes, thank you.

[jdharms]

I will make the change to use a limit reader and re-request.

I clubbed the PRs because currently both changes on the go-mod-core-contracts side have been merged, and they include interface changes (the sma client functionality). This means any other go-mod-core-contracts changes won't be able to be used by EdgeX unless it incorporates these changes, so I wanted to unblock development there as quickly as possible.

In the future I'll keep them separate.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
2.7% Duplication

Sorry, we're working on something.

[tsconn23]

I found a bug.

1.) Bring up core-data
2.) Bring up sys-mgmt-agent
3.) Make the following call GET http://localhost:48090/api/v1/metrics/edgex-core-data

That should return metrics for the core-data service but it returns a 404.

[
  {
    "operation": "metrics",
    "service": "edgex-core-data",
    "executor": "direct-service",
    "Success": false,
    "errorMessage": "404 - 404 page not found\n"
  }
]

The root cause actually lies in go-mod-core-contracts here
https://github.com/edgexfoundry/go-mod-core-contracts/blob/9132f554263fd61cb848f5b965b4d1aa9442ed97/clients/agent/client.go#L73

rc.UrlClient points to the host/port of core-data and the second parameter of clients.GetRequest should be the route. The value being constructed in this case is /api/v1/metrics/edgex-core-data which doesn't exist. I'm unclear why suffix is needed at all but in this case, it's clearly obfuscating the route resulting in the 404.

Recommend checking the other AgentClient methods as well since they use the same pattern.

When I remove use of suffix and change the line to
body, err := clients.GetRequest(ctx, clients.ApiMetricsRoute, rc.urlClient)
the correct result is returned

[
  {
    "operation": "metrics",
    "service": "edgex-core-data",
    "executor": "direct-service",
    "Success": true,
    "result": {
      "cpuUsedPercent": -1,
      "memoryUsed": 72155384,
      "raw": {
        "Memory": {
          "Alloc": 1028584,
          "TotalAlloc": 1028584,
          "Sys": 72155384,
          "Mallocs": 12029,
          "Frees": 1796,
          "LiveObjects": 10233
        },
        "CpuBusyAvg": -1
      }
    }
  }
]

[tsconn23]

DIscussed the above bug with @jdharms and @michaelestrin. The requirements were clarified and Daniel will rework his changes in go-mod-core-contracts before creating another integration PR.