OCPBUGS-29373: generateHAProxyCertConfigMap: No H2 with dup certs #589

Miciah · 2024-04-30T06:41:09Z

Add a check in generateHAProxyCertConfigMap for whether the same certificate is specified on more than one route, or whether a route specifies the default certificate as a custom certificate. If either of these conditions is detected, then configure HAProxy not to allow HTTP/2 client connections to this route.

Using the same certificate for multiple routes signals to Web browsers that they can do HTTP/2 connection coalescing. This means that a browser could open a connection for one route and then re-use this connection to connect to a second route, which can cause the client's request to go to the wrong backend server. We try to prevent this client behavior by advertising HTTP/2 in TLS ALPN only for routes that have unique certificates.

In general, we assume that if a route specifies a custom certificate, then it is a unique certificate. However, this assumption is not necessarily valid; indeed, it is possible to specify the same custom certificate on multiple routes, or specify the default certificate as an explicit certificate on a route.

After this commit, OpenShift router detects the cases where multiple routes have the same certificate, or where a route specifies the default certificate; in these cases, we prohibit negotiating HTTP/2 using ALPN for those routes. This should prevent connection coalescing in this situation.

openshift-ci-robot · 2024-04-30T06:41:16Z

@Miciah: This pull request references Jira Issue OCPBUGS-29373, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Add a check in generateHAProxyCertConfigMap for whether the same certificate is specified on more than one route, or whether a route specifies the default certificate as a custom certificate. If either of these conditions is detected, then configure HAProxy not to allow HTTP/2 client connections to this route.

Using the same certificate for multiple routes signals to Web browsers that they can do HTTP/2 connection coalescing. This means that a browser could open a connection for one route and then re-use this connection to connect to a second route, which can cause the client's request to go to the wrong backend server. We try to prevent this client behavior by advertising HTTP/2 in TLS ALPN only for routes that have unique certificates.

In general, we assume that if a route specifies a custom certificate, then it is a unique certificate. However, this assumption is not necessarily valid; indeed, it is possible to specify the same custom certificate on multiple routes, or specify the default certificate as an explicit certificate on a route.

After this commit, OpenShift router detects the cases where multiple routes have the same certificate, or where a route specifies the default certificate; in these cases, we prohibit negotiating HTTP/2 using ALPN for those routes. This should prevent connection coalescing in this situation.

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 openshift-eng/jira-lifecycle-plugin repository.

ShudiLi · 2024-04-30T10:21:10Z

tested it with 4.16.0-0.ci.test-2024-04-30-082717-ci-ln-pzlmy6b-latest

1.
% oc version
Client Version: 4.14.0-0.nightly-2023-10-08-220853
Kustomize Version: v5.0.1
Server Version: 4.16.0-0.ci.test-2024-04-30-082717-ci-ln-pzlmy6b-latest
Kubernetes Version: v1.29.0-rc.1.3936+d1ec84aa4c0ca4-dirty

2.
% oc -n openshift-ingress-operator annotate ingresscontrollers/default ingress.operator.openshift.io/default-enable-http2=true
ingresscontroller.operator.openshift.io/default annotate

3.
% oc get ingresses.config/cluster -o go-template --template='{{.spec.domain}}'
apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com

4.
% openssl req -x509 -newkey rsa:2048 -days 365 -addext "basicConstraints = CA:TRUE" -keyout ca.key -out ca.crt -nodes -subj '/CN=example-ca'
openssl req -newkey rsa:2048 -nodes -keyout tls.key -out tls.csr -subj "/CN=*.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com"
 % openssl x509 -req -days 365 -in tls.csr -CA ca.crt -CAcreateserial -CAkey ca.key -out tls.crt -extensions san -extfile tls.cfg
% cat tls.cfg
[san]
subjectAltName = @alt_names
[alt_names]
DNS.1 = console-openshift-console.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com
DNS.2 = oauth-openshift.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com

5.
% oc -n openshift-config create secret tls custom-cert --cert=tls.crt --key=tls.key

6.
% oc patch ingresses.config.openshift.io/cluster --type=merge --patch="
spec:
  componentRoutes:
  - hostname: console-openshift-console.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com
    name: console
    namespace: openshift-console
    servingCertKeyPairSecret:
      name: custom-cert
"
7.
% oc -n openshift-ingress rsh -c router deploy/router-default cat cert_config.map
/var/lib/haproxy/router/certs/openshift-console:console.pem [alpn h2,http/1.1] console-openshift-console.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com
E0430 17:21:13.238031   91435 v3.go:79] EOF

8.
% oc -n openshift-console get routes/console
NAME      HOST/PORT                                                                              PATH   SERVICES   PORT    TERMINATION          WILDCARD
console   console-openshift-console.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com          console    https   reencrypt/Redirect   None
% oc -n openshift-authentication get routes/oauth-openshift
NAME              HOST/PORT                                                                    PATH   SERVICES          PORT   TERMINATION            WILDCARD
oauth-openshift   oauth-openshift.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com          oauth-openshift   6443   passthrough/Redirect   None
% 

9.
% openssl s_client -connect console-openshift-console.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com:443 </dev/null 2>/dev/null | openssl x509 -sha1 -in /dev/stdin -noout -fingerprint
sha1 Fingerprint=8F:64:56:34:54:8A:92:BF:03:C1:67:D0:EF:A3:7F:E3:C8:0D:45:20

% openssl s_client -connect oauth-openshift.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com:443 </dev/null 2>/dev/null | openssl x509 -sha1 -in /dev/stdin -noout -fingerprint
sha1 Fingerprint=6D:AE:B2:75:9C:2A:58:7B:CC:32:80:F7:6C:98:30:4E:4F:D0:46:BA

 % openssl s_client -connect foo.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com:443 </dev/null 2>/dev/null | openssl x509 -sha1 -in /dev/stdin -noout -fingerprint 
sha1 Fingerprint=6D:AE:B2:75:9C:2A:58:7B:CC:32:80:F7:6C:98:30:4E:4F:D0:46:BA

10. open the chrome web browser, import the ca.crt certification, then close all chrome webs

11. open a  chrome web browser again and navigate to https://console-openshift-console.apps.ci-ln-pzlmy6b-76ef8.origin-ci-int-aws.dev.rhcloud.com/dashboards successfully with http2

ShudiLi · 2024-04-30T10:23:37Z

/label qe-approved
thanks

openshift-ci-robot · 2024-04-30T10:23:49Z

@Miciah: This pull request references Jira Issue OCPBUGS-29373, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

Add a check in generateHAProxyCertConfigMap for whether the same certificate is specified on more than one route, or whether a route specifies the default certificate as a custom certificate. If either of these conditions is detected, then configure HAProxy not to allow HTTP/2 client connections to this route.

Using the same certificate for multiple routes signals to Web browsers that they can do HTTP/2 connection coalescing. This means that a browser could open a connection for one route and then re-use this connection to connect to a second route, which can cause the client's request to go to the wrong backend server. We try to prevent this client behavior by advertising HTTP/2 in TLS ALPN only for routes that have unique certificates.

In general, we assume that if a route specifies a custom certificate, then it is a unique certificate. However, this assumption is not necessarily valid; indeed, it is possible to specify the same custom certificate on multiple routes, or specify the default certificate as an explicit certificate on a route.

After this commit, OpenShift router detects the cases where multiple routes have the same certificate, or where a route specifies the default certificate; in these cases, we prohibit negotiating HTTP/2 using ALPN for those routes. This should prevent connection coalescing in this situation.

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 openshift-eng/jira-lifecycle-plugin repository.

frobware

LGTM, just one question about breaking in a loop.

frobware · 2024-04-30T12:15:10Z

pkg/router/router_test.go

+ contains := false
+ for _, line := range strings.Split(string(content), "\n") {
+ if line == entry {
+ contains = true


Can we break once contains is true?

It's just test code, but I can add a break.

Fixed in https://github.com/openshift/router/compare/f86a9ba57bfa08239f396d8bf1fc597c332c1c62..0bc6f5ecf4c52169ca2a802cede164ac0a46f138.

Miciah · 2024-04-30T13:48:18Z

openshift/origin#28757 should fix the e2e-agnostic failure.

Miciah · 2024-05-01T15:18:59Z

/hold
Waiting for openshift/origin#28757.

candita · 2024-05-01T15:35:09Z

/assign @frobware

frobware · 2024-05-03T09:37:50Z

/hold Waiting for openshift/origin#28757.

28757 has merged.
/hold cancel

frobware · 2024-05-03T09:38:30Z

/test e2e-agnostic

The verify jobs is failing because of go fmt differences.

Add a check in generateHAProxyCertConfigMap for whether the same certificate is specified on more than one route, or whether a route specifies the default certificate as a custom certificate. If either of these conditions is detected, then configure HAProxy not to allow HTTP/2 client connections to this route. Using the same certificate for multiple routes signals to Web browsers that they can do HTTP/2 connection coalescing. This means that a browser could open a connection for one route and then re-use this connection to connect to a second route, which can cause the client's request to go to the wrong backend server. We try to prevent this client behavior by advertising HTTP/2 in TLS ALPN only for routes that have unique certificates. In general, we assume that if a route specifies a custom certificate, then it is a unique certificate. However, this assumption is not necessarily valid; indeed, it is possible to specify the same custom certificate on multiple routes, or specify the default certificate as an explicit certificate on a route. After this commit, OpenShift router detects the cases where multiple routes have the same certificate, or where a route specifies the default certificate; in these cases, we prohibit negotiating HTTP/2 using ALPN for those routes. This should prevent connection coalescing in this situation. This commit fixes OCPBUGS-29373. https://issues.redhat.com/browse/OCPBUGS-29373 * pkg/router/template/router.go (templateData): Add a CertificateIndex field to track the number of times each certificate is observed. ((templateRouter).writeConfig): Set CertificateIndex. * pkg/router/template/template_helper.go (generateHAProxyCertConfigMap): Use CertificateIndex to determine whether HTTP/2 should be enabled for the specified route. * pkg/router/router_test.go (TestMain): Configure the certs directory and default certificate. (TestConfigTemplate): Add test cases for two routes with distinct certificates, two routes with the same certificate, and a route that uses the default certificate. (mustCreate, (mustCreate).Apply): Add a cert field so that test cases can specify the route certificate. (mustMatchConfig): Add a mapFile field so that test cases can specify a map file to check. Add comments for all the fields. ((mustMatchConfig).Match): Add a switch to handle matching map files in addition to haproxy.config, using the new matchMapFile and matchConfig helper functions. (matchConfig, matchMapFile): New helper functions.

Miciah · 2024-05-03T14:26:50Z

https://github.com/openshift/router/compare/f86a9ba57bfa08239f396d8bf1fc597c332c1c62..0bc6f5ecf4c52169ca2a802cede164ac0a46f138 adds an aesthetic break statement in a test and fixes gofmt (I hope).

frobware · 2024-05-03T15:02:42Z

/lgtm
/approve

openshift-ci · 2024-05-03T15:04:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware

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

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

openshift-ci · 2024-05-03T17:24:24Z

@Miciah: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci-robot · 2024-05-03T17:28:32Z

@Miciah: Jira Issue OCPBUGS-29373: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-29373 has been moved to the MODIFIED state.

In response to this:

Add a check in generateHAProxyCertConfigMap for whether the same certificate is specified on more than one route, or whether a route specifies the default certificate as a custom certificate. If either of these conditions is detected, then configure HAProxy not to allow HTTP/2 client connections to this route.

Using the same certificate for multiple routes signals to Web browsers that they can do HTTP/2 connection coalescing. This means that a browser could open a connection for one route and then re-use this connection to connect to a second route, which can cause the client's request to go to the wrong backend server. We try to prevent this client behavior by advertising HTTP/2 in TLS ALPN only for routes that have unique certificates.

In general, we assume that if a route specifies a custom certificate, then it is a unique certificate. However, this assumption is not necessarily valid; indeed, it is possible to specify the same custom certificate on multiple routes, or specify the default certificate as an explicit certificate on a route.

After this commit, OpenShift router detects the cases where multiple routes have the same certificate, or where a route specifies the default certificate; in these cases, we prohibit negotiating HTTP/2 using ALPN for those routes. This should prevent connection coalescing in this situation.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2024-05-03T20:46:33Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-haproxy-router-base-container-v4.16.0-202405031950.p0.g56ab14f.assembly.stream.el9 for distgit ose-haproxy-router-base.
All builds following this will include this PR.

openshift-merge-robot · 2024-05-05T04:46:52Z

Fix included in accepted release 4.16.0-0.nightly-2024-05-04-214435

openshift-ci bot requested review from ShudiLi, alebedev87 and knobunc April 30, 2024 06:41

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Apr 30, 2024

frobware reviewed Apr 30, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2024

openshift-ci bot assigned frobware May 1, 2024

openshift-ci-robot mentioned this pull request May 2, 2024

OCPBUGS-29373: test/extended/router/http2: Use unique certs openshift/origin#28757

Merged

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2024

Miciah force-pushed the OCPBUGS-29373-generateHAProxyCertConfigMap-no-H2-with-dup-certs branch from f86a9ba to 0bc6f5e Compare May 3, 2024 14:26

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2024

openshift-merge-bot bot merged commit 56ab14f into openshift:master May 3, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-29373: generateHAProxyCertConfigMap: No H2 with dup certs #589

OCPBUGS-29373: generateHAProxyCertConfigMap: No H2 with dup certs #589

Miciah commented Apr 30, 2024

openshift-ci-robot commented Apr 30, 2024

ShudiLi commented Apr 30, 2024

ShudiLi commented Apr 30, 2024

openshift-ci-robot commented Apr 30, 2024

frobware left a comment

frobware Apr 30, 2024

Miciah Apr 30, 2024

Miciah May 3, 2024

Miciah commented Apr 30, 2024

Miciah commented May 1, 2024

candita commented May 1, 2024

frobware commented May 3, 2024

frobware commented May 3, 2024

Miciah commented May 3, 2024

frobware commented May 3, 2024

openshift-ci bot commented May 3, 2024

openshift-ci bot commented May 3, 2024

openshift-ci-robot commented May 3, 2024

openshift-bot commented May 3, 2024

openshift-merge-robot commented May 5, 2024

OCPBUGS-29373: generateHAProxyCertConfigMap: No H2 with dup certs #589

OCPBUGS-29373: generateHAProxyCertConfigMap: No H2 with dup certs #589

Conversation

Miciah commented Apr 30, 2024

openshift-ci-robot commented Apr 30, 2024

ShudiLi commented Apr 30, 2024

ShudiLi commented Apr 30, 2024

openshift-ci-robot commented Apr 30, 2024

frobware left a comment

Choose a reason for hiding this comment

frobware Apr 30, 2024

Choose a reason for hiding this comment

Miciah Apr 30, 2024

Choose a reason for hiding this comment

Miciah May 3, 2024

Choose a reason for hiding this comment

Miciah commented Apr 30, 2024

Miciah commented May 1, 2024

candita commented May 1, 2024

frobware commented May 3, 2024

frobware commented May 3, 2024

Miciah commented May 3, 2024

frobware commented May 3, 2024

openshift-ci bot commented May 3, 2024

openshift-ci bot commented May 3, 2024

openshift-ci-robot commented May 3, 2024

openshift-bot commented May 3, 2024

openshift-merge-robot commented May 5, 2024