Mitigate potential Slowloris attacks by setting ReadHeaderTimeout in all http.Server instances

[wallrj]

A recent security audit has shown that there are some http servers in cert-manager where the ReadHeaderTimeout setting is not set.
The risk is low but if we add sensible default ReadHeaderTimeout values it will slightly mitigate the risk of "slowloris" style DDOS attacks.
I have used the same 32s values as are used in the Kubernetes api-server and which were then adopted by controller-runtime.
Kyverno uses slightly lower 30s values but I can't find any documentation explaining how that value was chosen.

To reveal the problem, I temporarily added the golangci-lint-action to this branch, configured it to check for G112, and observed the resulting CI failures.

I added the missing the ReadHeaderTimeout values and re-ran the action and observed that the golangci-lint-action passed.

Finally I removed the golangci-lint action, in the interests of keeping this patch small enough that it can be easily backported to release-1.12 and release-1.13.

The cert-manager maintainers decided that this change is low risk and sufficiently important that it should be included in a 1.12 and 1.13 patch release.
We recognise that this change in itself will not fix the problem but it is a small improvement and it allows us to pre-empt the security audit report, which will now hopefully report that the issue has been addressed.

Testing

# pprof server now times out stalled connections after 32s
$ time telnet 127.0.0.1 6060
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
Connection closed by foreign host.

real    0m32.013s
user    0m0.005s
sys     0m0.000s

# The metrics and healthz servers were already configured with ReadHeaderTimeout of 8s
$ time telnet 127.0.0.1 9402
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
Connection closed by foreign host.

real    0m8.005s
user    0m0.000s
sys     0m0.004s

$ time telnet 127.0.0.1 9403
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
Connection closed by foreign host.

real    0m8.006s
user    0m0.003s
sys     0m0.001s

Example Failure

• https://github.com/cert-manager/cert-manager/actions/runs/7116007299/job/19373411374?pr=6534

Example code from other projects

• https://github.com/knative/eventing/blob/2ee2699c9c04aa34bb8e5481f4df84785afb9d1f/pkg/adapter/apiserver/adapter.go#L127-L134

• https://github.com/kubernetes/kubernetes/blob/9e028b40b9e970142191259effe796b3dab39828/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go#L165-L173

• https://github.com/kyverno/kyverno/blob/e1eb9f6cdb067c6cd657864d976ec4ca98e8e437/pkg/webhooks/server.go#L164-L196

Links

• https://en.wikipedia.org/wiki/Slowloris_(computer_security)
• https://www.cloudflare.com/en-gb/learning/ddos/ddos-attack-tools/slowloris/
• https://golangci-lint.run/usage/linters/#gosec
• https://github.com/securego/gosec#available-rules
• https://github.com/search?q=org%3Aknative+slowloris&type=code
• Set idle and readheader timeouts kubernetes/kubernetes#103958
• add http limit for the Slowloris attack karmada-io/karmada#3951
• set ReadHeaderTimeout for http server karmada-io/karmada#2594
• ⚠️ Refactor manager to avoid race conditions and provide clean shutdown kubernetes-sigs/controller-runtime#1695
• https://github.com/search?q=repo%3Akyverno%2Fkyverno%20ReadHeaderTimeout&type=code
• chore: fix golangcilint timeout kyverno/kyverno#4388
• Add a rule for Slowloris Attack  securego/gosec#807
• https://knative.dev/blog/events/security-audit-2023/
• https://github.com/knative/docs/blob/main/reports/ADA-knative-security-audit-2023.pdf

Mitigate potential Slowloris attacks by setting ReadHeaderTimeout in all http.Server instances

[wallrj]

golangci-lint isn't checking the sub-packages:

• Add support for multiple subdirs each with their own go.mod golangci/golangci-lint#828

# ~/projects/cert-manager/cert-manager/cmd/controller$ 
golangci-lint run ./... --disable-all --enable gosec
app/controller.go:109:22: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)
                profilerServer := &http.Server{
                        Handler: profilerMux,
                }

[wallrj]

Examples of slowloris warnings after enabling golangci-lint in all the sub-modules:

• https://github.com/cert-manager/cert-manager/actions/runs/7117953447?pr=6534

[wallrj]

Without this, the linter fails to compile the file:

$ golangci-lint run --config .golangci.ci.yaml ./test/integration/...
test/integration/versionchecker/versionchecker_test.go:42:12: pattern testdata/test_manifests.tar: no matching files found (typecheck)
//go:embed testdata/test_manifests.tar

• Cannot easily disable typecheck golangci/golangci-lint#2912

[wallrj]

/retest

[inteon]

@wallrj could we add golangci-lint to the verify make target? I feel like this GH action makes things quite a bit more complex.

[gaby]

@wallrj Saw the tag and i was so confused 😂 Recently I started using this instead to keep it cleaner:

Example for go monorepo scanned with gosec

jobs:
  detect-changes:
    runs-on: ubuntu-latest
    outputs:
      matrix: ${{ steps.changed-files.outputs.all_changed_files }}
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0
      - name: Changed Files
        uses: tj-actions/changed-files@v40
        id: changed-files
        with:
          files_ignore: |
            .github/**
            **.md
          json: true
          escape_json: false
          dir_names: true
          dir_names_exclude_current_dir: true

  gosec-scan:
    runs-on: ubuntu-latest
    needs: detect-changes
    env:
      GO111MODULE: on
    strategy:
      matrix:
        modules: ${{ fromJSON(needs.detect-changes.outputs.matrix) }}
    steps:
      - name: Fetch Repository
        uses: actions/checkout@v4
      - name: Install Go
        uses: actions/setup-go@v4
        with:
          go-version: 'stable'
          check-latest: true
          cache: false
      - name: Install gosec
        run: go install github.com/securego/gosec/v2/cmd/gosec@latest
      - name: Run gosec
        working-directory: ${{ matrix.modules }}
        run: gosec ./...

[inteon]

/cherrypick release-1.12

[jetstack-bot]

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.12 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[inteon]

/cherrypick release-1.13

[jetstack-bot]

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.13 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[inteon]

/approve
/lgtm

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jetstack-bot]

@inteon: #6534 failed to apply on top of branch "release-1.12":

Applying: Add ReadHeaderTimeout to all http.Server where that setting is missing
Using index info to reconstruct a base tree...
A	cmd/cainjector/app/controller.go
M	cmd/controller/app/controller.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/controller/app/controller.go
CONFLICT (content): Merge conflict in cmd/controller/app/controller.go
CONFLICT (modify/delete): cmd/cainjector/app/controller.go deleted in HEAD and modified in Add ReadHeaderTimeout to all http.Server where that setting is missing. Version Add ReadHeaderTimeout to all http.Server where that setting is missing of cmd/cainjector/app/controller.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add ReadHeaderTimeout to all http.Server where that setting is missing
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jetstack-bot]

@inteon: #6534 failed to apply on top of branch "release-1.13":

Applying: Add ReadHeaderTimeout to all http.Server where that setting is missing
Using index info to reconstruct a base tree...
A	cmd/cainjector/app/controller.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): cmd/cainjector/app/controller.go deleted in HEAD and modified in Add ReadHeaderTimeout to all http.Server where that setting is missing. Version Add ReadHeaderTimeout to all http.Server where that setting is missing of cmd/cainjector/app/controller.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add ReadHeaderTimeout to all http.Server where that setting is missing
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[wallrj]

/kind bug

[wallrj]

@wallrj Saw the tag and i was so confused 😂 Recently I started using this instead to keep it cleaner:

@gaby Thank you for responding. I considered running gosec directly, but figured that we will benefit from all the other linters supported by golangci-lint-action, so long as we can keep the output concise and readable. Anyway I moved the introduction of that GitHub action into a separate PR and I found a way to run it on all sub-modules without using a matrix:

• Add the golangci-lint GitHub action #6537