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

Pull in protojson vuln fixes #5267

Merged
merged 3 commits into from Jan 8, 2024
Merged

Conversation

tdeebswihart
Copy link
Contributor

@tdeebswihart tdeebswihart commented Jan 8, 2024

What was changed

I bumped the version of api-go and sdk-go and slightly altered our nettest RPC factory interface to deal with changes in v1.60.0 of go-grpc

Why?

To fix the protojson DOS vulns recently patched in the upstream golang/protobuf. See temporalio/api-go#143 for details

How did you test it?

I pulled the tests added to the protojson repo into our fork

Potential risks

N/A

Is hotfix candidate?

No as it requires all our other proto changes which aren't released

@tdeebswihart tdeebswihart requested a review from a team as a code owner January 8, 2024 18:43
@@ -41,20 +41,20 @@ require (
go.opentelemetry.io/otel/metric v1.19.0
go.opentelemetry.io/otel/sdk v1.19.0
go.opentelemetry.io/otel/sdk/metric v1.19.0
go.temporal.io/api v1.26.2-0.20231129165614-630d88440548
go.temporal.io/sdk v1.25.2-0.20231129171107-288a04f72145
go.temporal.io/api v1.26.1-0.20240103185939-608bdd111e4b
Copy link
Contributor

@stephanos stephanos Jan 8, 2024

Choose a reason for hiding this comment

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

It looks like it's downgraded; or am I reading this wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did not; we retracted the v1.26.1 version. It's... weird. I don't think Go handles it gracefully

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see!

The DialContext function was altered in such a way that a non-blocking
dial wouldn't see the context had been cancelled. This looks like what
was intended all along, so we force the dial to block until the
connection is established in order to force a test failure.
@tdeebswihart tdeebswihart enabled auto-merge (squash) January 8, 2024 22:20
@tdeebswihart tdeebswihart merged commit 6a4450f into main Jan 8, 2024
10 checks passed
@tdeebswihart tdeebswihart deleted the tds/remediate-protojson-vuln branch January 8, 2024 22:41
tdeebswihart added a commit that referenced this pull request Jan 9, 2024
## What was changed

I bumped the version of api-go and sdk-go and _slightly_ altered our
nettest RPC factory interface to deal with changes in v1.60.0 of go-grpc

## Why?

To fix the protojson DOS vulns recently patched in the upstream
golang/protobuf. See temporalio/api-go#143 for
details

## How did you test it?

I pulled the tests added to the protojson repo into our fork

## Potential risks

N/A

## Is hotfix candidate?

No as it requires all our other proto changes which aren't released
tdeebswihart added a commit that referenced this pull request Jan 9, 2024
## What was changed

I bumped the version of api-go and sdk-go and _slightly_ altered our
nettest RPC factory interface to deal with changes in v1.60.0 of go-grpc

## Why?

To fix the protojson DOS vulns recently patched in the upstream
golang/protobuf. See temporalio/api-go#143 for
details

## How did you test it?

I pulled the tests added to the protojson repo into our fork

## Potential risks

N/A

## Is hotfix candidate?

No as it requires all our other proto changes which aren't released
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

4 participants