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.

Sign up for GitHub

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

Jump to bottom

gitlab: Use ListMergeRequestDiff instead of GetMergeRequestChanges #1470

Open
chmouel opened this issue Oct 16, 2023 · 0 comments
Open

gitlab: Use ListMergeRequestDiff instead of GetMergeRequestChanges #1470

chmouel opened this issue Oct 16, 2023 · 0 comments
Labels
refactoring

Comments

@chmouel
Copy link
Member

chmouel commented Oct 16, 2023

since gitlab deprecated it as documented here:

https://docs.gitlab.com/ee/api/merge_requests.html#get-single-merge-request-changes

pkg/provider/gitlab/gitlab.go:315:24: SA1019: v.Client.MergeRequests.GetMergeRequestChanges is deprecated: This endpoint has been replaced by MergeRequestsService.ListMergeRequestDiffs() (staticcheck)
		mrchanges, _, err := v.Client.MergeRequests.GetMergeRequestChanges(v.sourceProjectID, runevent.PullRequestNumber, &gitlab.GetMergeRequestChangesOptions{}
chmouel added a commit to chmouel/pipelines-as-code that referenced this issue Oct 16, 2023
@chmouel
Update dependencies
7f73dd1 
This critically fix these security bugs in the dependencies:

swift-nio-http2 vulnerable to HTTP/2 Stream Cancellation Attack
```
swift-nio-http2 is vulnerable to a denial-of-service vulnerability in which a malicious client can create and then reset a large number of HTTP/2 streams in a short period of time. This causes swift-nio-http2 to commit to a large amount of expensive work which it then throws away, including creating entirely new Channels to serve the traffic. This can easily overwhelm an EventLoop and prevent it from making forward progress.

swift-nio-http2 1.28 contains a remediation for this issue that applies reset counter using a sliding window. This constrains the number of stream resets that may occur in a given window of time. Clients violating this limit will have their connections torn down. This allows clients to continue to cancel streams for legitimate reasons, while constraining malicious actors.
```

HTTP/2 rapid reset can cause excessive work in net/http
```
A malicious HTTP/2 client which rapidly creates requests and immediately resets them can cause excessive server resource consumption. While the total number of requests is bounded by the http2.Server.MaxConcurrentStreams setting, resetting an in-progress request allows the attacker to create a new request while the existing one is still executing.

With the fix applied, HTTP/2 servers now bound the number of simultaneously executing handler goroutines to the stream concurrency limit (MaxConcurrentStreams). New requests arriving when at the limit (which can only happen after the client has reset an existing, in-flight request) will be queued until a handler exits. If the request queue grows too large, the server will terminate the connection.

This issue is also fixed in golang.org/x/net/http2 for users manually configuring HTTP/2.

The default stream concurrency limit is 250 streams (requests) per HTTP/2 connection. This value may be adjusted using the golang.org/x/net/http2 package; see the Server.MaxConcurrentStreams setting and the ConfigureServer function.
```

Pipelines do not validate child UID
```
Pipelines do not validate child UIDs, which means that a user that has access to create TaskRuns can create their own Tasks that the Pipelines controller will accept as the child Task.

We should add UID to PipelineRun status and validate that child Run status/results only come from Runs matching the same UID.
Details

While we store and validate the PipelineRun's (api version, kind, name, uid) in the child Run's OwnerReference, we only store (api version, kind, name) in the ChildStatusReference .

This means that if a client had access to create TaskRuns on a cluster, they could create a child TaskRun for a pipeline with the same name + owner reference, and the Pipeline controller picks it up as if it was the original TaskRun. This is problematic since it can let users modify the config of Pipelines at runtime, which violates SLSA L2 Service Generated / Non-falsifiable requirements.

I believe this is also true for TaskRuns -> Pods since it looks like we only lookup by name, though I haven't tested this.

If you have update permissions on tekton resources, you could also perform a similar bypass like this (because it's difficult to distinguish this from a Task retry). For now, I think relying on RBAC is fine and treat update as a privileged role (though we should perhaps update docs to stress this). Create is the most problematic for now. SPIFFE/SPIRE might be able to help with ensuring that only the controller can modify state long term (e.g. sign the expected UIDs?)
```

bitbucket has a change in their calls:

GetContent_11(v.projectKey, runevent.Repository, path, localVarOptionals)

become

GetRawContent(v.projectKey, runevent.Repository, path, localVarOptionals)

gitlab changed some of their struct to a proper type and deprecated
GetMergeRequestChanges to ListMergeRequestDiff (filled as TODO in openshift-pipelines#1470)
@chmouel chmouel mentioned this issue Oct 16, 2023
Merged
7 tasks
chmouel added a commit to chmouel/pipelines-as-code that referenced this issue Oct 16, 2023
@chmouel
Update dependencies
a3951c7 
bitbucket has a change in their calls:

GetContent_11(v.projectKey, runevent.Repository, path, localVarOptionals)

become

GetRawContent(v.projectKey, runevent.Repository, path, localVarOptionals)

gitlab changed some of their struct to a proper type and deprecated
GetMergeRequestChanges to ListMergeRequestDiff (filled as TODO in openshift-pipelines#1470)

Signed-off-by: Chmouel Boudjnah <[email protected]>
chmouel added a commit to chmouel/pipelines-as-code that referenced this issue Oct 16, 2023
@chmouel
Update dependencies
3094543 
bitbucket has a change in their calls:

GetContent_11(v.projectKey, runevent.Repository, path, localVarOptionals)
becomes:
GetRawContent(v.projectKey, runevent.Repository, path, localVarOptionals)

bitbucket has a change as well on how to pass files to create..

gitlab changed some of their struct to a proper type and deprecated
GetMergeRequestChanges to ListMergeRequestDiff (filled as TODO in openshift-pipelines#1470)

Signed-off-by: Chmouel Boudjnah <[email protected]>
piyush-garg pushed a commit that referenced this issue Oct 17, 2023
@chmouel @piyush-garg
Update dependencies
8c59966 
bitbucket has a change in their calls:

GetContent_11(v.projectKey, runevent.Repository, path, localVarOptionals)
becomes:
GetRawContent(v.projectKey, runevent.Repository, path, localVarOptionals)

bitbucket has a change as well on how to pass files to create..

gitlab changed some of their struct to a proper type and deprecated
GetMergeRequestChanges to ListMergeRequestDiff (filled as TODO in #1470)

Signed-off-by: Chmouel Boudjnah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
refactoring
Milestone
No milestone
Development

No branches or pull requests
1 participant
@chmouel