Use http/1.1 for webhooks, metrics and profiling endpoints #1923

yuumasato · 2023-10-18T15:04:03Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This mitigates an HTTP/2 Stream Cancellation Attack that can lead to a denial-of-service.

Which issue(s) this PR fixes:

CVE-2023-44487

Does this PR have test?

Yes, added test cases for Metrics, Webhooks and profiling endpoints checking that they are serving HTTP/1.1

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

NONE

linux-foundation-easycla · 2023-10-18T15:04:08Z

The committers listed above are authorized under a signed CLA.

✅ login: yuumasato / name: Watson Yuuma Sato (0629fb8)

k8s-ci-robot · 2023-10-18T15:04:13Z

Welcome @yuumasato!

It looks like this is your first PR to kubernetes-sigs/security-profiles-operator 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/security-profiles-operator has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-10-18T15:04:14Z

Hi @yuumasato. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Vincent056

/lgtm
thanks for the patch

saschagrunert · 2023-10-19T07:10:47Z

/ok-to-test

codecov-commenter · 2023-10-19T07:17:40Z

Codecov Report

Merging #1923 (0629fb8) into main (3bee9d9) will decrease coverage by 0.05%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1923      +/-   ##
==========================================
- Coverage   49.32%   49.28%   -0.05%     
==========================================
  Files          72       72              
  Lines        7008     7008              
==========================================
- Hits         3457     3454       -3     
- Misses       3420     3422       +2     
- Partials      131      132       +1

cmd/security-profiles-operator/main.go

yuumasato · 2023-11-20T07:47:31Z

/retest

yuumasato · 2023-11-20T15:47:36Z

I have limited runAndRetryPodCMD() to 5 times.
This should allow other tests to run. I want to see if the metrics endpoint name can be resolved.

test/suite_test.go

yuumasato · 2023-11-21T13:47:27Z

@saschagrunert Hi, I have moved the test for webhook and metrics endpoints HTTP versions behind a check for "flakyness".

yuumasato · 2023-11-22T13:31:07Z

I moved the webhook and metrics test case to file test/e2e_flaky_test.go to ensure they are run with make test-flaky-e2e

This mitigates an HTTP/2 Stream Cancellation Attack that can lead to a denial-of-service. Tests are also added to ensure the metrics, webhook and profiling endpoints are serving HTTP/1.1 The profiling HTTP test is Selinux Specific, and the Webhook and Metrics HTTP version tests are marked as flaky, as the urls, for some reason, are not solved in upstream CI.

rhmdnd · 2023-11-22T14:15:27Z

These tests work for me on an OpenShift cluster if I apply #1979

--- PASS: TestSuite (189.72s)
    --- PASS: TestSuite/TestSecurityProfilesOperator_Flaky (188.60s)
        --- SKIP: TestSuite/TestSecurityProfilesOperator_Flaky/cluster-wide:_Seccomp:_Metrics (0.00s)
        --- PASS: TestSuite/TestSecurityProfilesOperator_Flaky/cluster-wide:_SPOD:_Test_webhook_HTTP_version (10.93s)
        --- PASS: TestSuite/TestSecurityProfilesOperator_Flaky/cluster-wide:_SPOD:_Test_Metrics_HTTP_version (8.04s)
        --- PASS: TestSuite/TestSecurityProfilesOperator_Flaky/cluster-wide:_Seccomp:_Verify_profile_binding (26.92s)
        --- SKIP: TestSuite/TestSecurityProfilesOperator_Flaky/cluster-wide:_Seccomp:_Verify_profile_recording_logs (0.00s)
        --- SKIP: TestSuite/TestSecurityProfilesOperator_Flaky/cluster-wide:_Seccomp:_Verify_profile_recording_bpf (0.00s)
        --- SKIP: TestSuite/TestSecurityProfilesOperator_Flaky/namespaced:_Seccomp:_Metrics (0.00s)
        --- PASS: TestSuite/TestSecurityProfilesOperator_Flaky/namespaced:_SPOD:_Test_webhook_HTTP_version (7.79s)
        --- PASS: TestSuite/TestSecurityProfilesOperator_Flaky/namespaced:_SPOD:_Test_Metrics_HTTP_version (8.04s)
PASS
ok      sigs.k8s.io/security-profiles-operator/test     189.726s

Thanks for digging into all this @yuumasato.

rhmdnd

/lgtm

k8s-ci-robot · 2023-11-22T14:16:09Z

@rhmdnd: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

yuumasato · 2023-11-22T14:22:42Z

@rhmdnd Thanks for checking the execution of the flaky tests.

k8s-ci-robot · 2023-11-23T08:22:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert, yuumasato

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~~ [saschagrunert]

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

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Oct 18, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 18, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 18, 2023

k8s-ci-robot requested review from JAORMX and Vincent056 October 18, 2023 15:04

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 18, 2023

Vincent056 approved these changes Oct 18, 2023

View reviewed changes

k8s-ci-robot assigned Vincent056 Oct 18, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2023

yuumasato force-pushed the disable-http2 branch from a0533bf to 20fa3ac Compare October 18, 2023 17:18

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 19, 2023

saschagrunert approved these changes Oct 19, 2023

View reviewed changes

k8s-ci-robot assigned saschagrunert Oct 19, 2023

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 19, 2023

saschagrunert requested changes Oct 19, 2023

View reviewed changes

cmd/security-profiles-operator/main.go Outdated Show resolved Hide resolved

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 19, 2023

yuumasato force-pushed the disable-http2 branch from 20fa3ac to 45c729a Compare October 19, 2023 16:31

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 19, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2023

yuumasato force-pushed the disable-http2 branch 2 times, most recently from dca3a5f to 66c1258 Compare November 17, 2023 18:10

yuumasato force-pushed the disable-http2 branch 2 times, most recently from 41a4a15 to b372999 Compare November 20, 2023 15:04

yuumasato force-pushed the disable-http2 branch from 70b9f36 to 0432492 Compare November 20, 2023 16:03

rhmdnd reviewed Nov 20, 2023

View reviewed changes

test/suite_test.go Outdated Show resolved Hide resolved

yuumasato force-pushed the disable-http2 branch from c345af8 to 52476cd Compare November 21, 2023 13:59

saschagrunert approved these changes Nov 21, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2023

saschagrunert added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 21, 2023

yuumasato force-pushed the disable-http2 branch 2 times, most recently from 122ca4b to 899f3a9 Compare November 22, 2023 13:17

yuumasato force-pushed the disable-http2 branch from 899f3a9 to 0629fb8 Compare November 22, 2023 13:33

rhmdnd reviewed Nov 22, 2023

View reviewed changes

saschagrunert removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 23, 2023

saschagrunert approved these changes Nov 23, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2023

k8s-ci-robot merged commit 47c73e8 into kubernetes-sigs:main Nov 23, 2023
19 checks passed

yuumasato deleted the disable-http2 branch November 23, 2023 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use http/1.1 for webhooks, metrics and profiling endpoints #1923

Use http/1.1 for webhooks, metrics and profiling endpoints #1923

yuumasato commented Oct 18, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 18, 2023 •

edited

Loading

k8s-ci-robot commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

Vincent056 left a comment

saschagrunert commented Oct 19, 2023

codecov-commenter commented Oct 19, 2023 •

edited

Loading

yuumasato commented Nov 20, 2023

yuumasato commented Nov 20, 2023

yuumasato commented Nov 21, 2023

yuumasato commented Nov 22, 2023

rhmdnd commented Nov 22, 2023

rhmdnd left a comment

k8s-ci-robot commented Nov 22, 2023

yuumasato commented Nov 22, 2023

k8s-ci-robot commented Nov 23, 2023

Use http/1.1 for webhooks, metrics and profiling endpoints #1923

Use http/1.1 for webhooks, metrics and profiling endpoints #1923

Conversation

yuumasato commented Oct 18, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR have test?

Special notes for your reviewer:

Does this PR introduce a user-facing change?

linux-foundation-easycla bot commented Oct 18, 2023 • edited Loading

k8s-ci-robot commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

Vincent056 left a comment

Choose a reason for hiding this comment

saschagrunert commented Oct 19, 2023

codecov-commenter commented Oct 19, 2023 • edited Loading

Codecov Report

yuumasato commented Nov 20, 2023

yuumasato commented Nov 20, 2023

yuumasato commented Nov 21, 2023

yuumasato commented Nov 22, 2023

rhmdnd commented Nov 22, 2023

rhmdnd left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 22, 2023

yuumasato commented Nov 22, 2023

k8s-ci-robot commented Nov 23, 2023

yuumasato commented Oct 18, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 18, 2023 •

edited

Loading

codecov-commenter commented Oct 19, 2023 •

edited

Loading