Skip to content

vendor: google.golang.org/grpc v1.56.3#4341

Merged
tonistiigi merged 2 commits intomoby:masterfrom
thaJeztah:bump_grpc
Oct 26, 2023
Merged

vendor: google.golang.org/grpc v1.56.3#4341
tonistiigi merged 2 commits intomoby:masterfrom
thaJeztah:bump_grpc

Conversation

@thaJeztah
Copy link
Copy Markdown
Member


vendor: google.golang.org/grpc v1.56.2

full diff:

vendor: google.golang.org/grpc v1.56.3

server: prohibit more than MaxConcurrentStreams handlers from running at once
(CVE-2023-44487).

In addition to this change, applications should ensure they do not leave running
tasks behind related to the RPC before returning from method handlers, or should
enforce appropriate limits on any such work.

@crazy-max
Copy link
Copy Markdown
Member

Why do wee need both commits 1.56.2 and 1.56.3 and not just 1.56.3?

@thaJeztah
Copy link
Copy Markdown
Member Author

I had the 1.56.2 commit because that's the version used by moby/moby currently (had it in a branch I was working on), then recalled that the latest Go CVE also affected grpc; I thought having the CVE-fix in a separate commit made it more apparent, and allowed for reviewing the security fix in isolation.

@tonistiigi
Copy link
Copy Markdown
Member

I think these need to land in containerd first. Otherwise, we don't know what this is breaking. The CVE does not apply to buildkit afaics because buildkit can only be accessed from authenticated API.

@thaJeztah
Copy link
Copy Markdown
Member Author

server: prohibit more than MaxConcurrentStreams handlers from running at once
(CVE-2023-44487).

In addition to this change, applications should ensure they do not leave running
tasks behind related to the RPC before returning from method handlers, or should
enforce appropriate limits on any such work.

- grpc/grpc-go@v1.56.2...v1.56.3

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Copy Markdown
Member Author

Update in containerd was merged; this good to go, or do we need to wait for containerd to release it? (did a quick rebase to get a fresh run of CI)

@jedevc
Copy link
Copy Markdown
Member

jedevc commented Oct 26, 2023

Update in containerd was merged; this good to go, or do we need to wait for containerd to release it?

Do we not need to vendor containerd to the version with the fix.

@thaJeztah
Copy link
Copy Markdown
Member Author

I guess the important questions here were;

  • if we update this dependency, is CI still happy connecting to the current version of containerd (without it having updated GRPC); and it looks it does
  • Is the containerd module happy with the updated GRPC version (and not causing havoc in the dependency tree); and the update worked in containerd's repository, and was accepted.

It's always the question who "owns" the dependency, but with go modules I don't think there's no other choice than "It's SemVer, so nobody own, and SemVer compatible means "we shouldn't care" (..... in an ideal world).

I opened this PR because I was in the process of updating the docker / moby dependency in BuildKit; for Moby we're already on this version, because the Google logging driver forced us to move to a newer version, so while Moby not yet has a go.mod, I do treat it as such, so update dependencies to versions Go modules would enforce. As there's a security fix in this version, I didn't want to "stuff" that update together with other updates; in case we must consider backporting this update (I know people already started to ask questions about BuildKit and Buildx being vulnerable because scanners started flagging them).

@thaJeztah
Copy link
Copy Markdown
Member Author

@tonistiigi @crazy-max PTAL; any concerns remaining on this one?

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.

4 participants