Bug 1843856: Sanitize TLS config that has key bundled with cert #136
Conversation
openshift-ci-robot
added
bugzilla/severity-high
|
@Miciah: This pull request references Bugzilla bug 1843856, which is invalid:
Comment In response to this:
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. |
openshift-ci-robot
added
the
approved
|
/bugzilla refresh @openshift/openshift-team-network-edge, please review. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@Miciah: This pull request references Bugzilla bug 1843856, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
openshift-ci-robot
removed
the
bugzilla/invalid-bug
| } else { | ||
| if len(tlsConfig.Key) > 0 { | ||
| if len(keyBytes) > 0 { | ||
| keyBytes = append(keyBytes, byte('\n')) |
There was a problem hiding this comment.
I noticed there isn't a unit test that covers this branch. I point this out because I am slightly confused as to what's going on here. If tlsConfig.Key is set and splitCertKey yields private keyBytes from tlsConfig.Certificate, then keyBytes will become a new-line delimited buffer of both of the keys? Could you elaborate a little bit so I can understand?
There was a problem hiding this comment.
Right, we'll move any private parts that were in tlsConfig.Certificate to tlsConfig.Key, preserving any existing private parts that were already in tlsConfig.Key. I'm not sure whether it would be better just to ignore keyBytes if tlsConfig.Key is already set, or override tlsConfig.Key with keyBytes if the latter is nonempty, but I think this is the safest approach, keeping in mind that we don't want to break existing routes that may be doing weird things in tlsConfig.
There was a problem hiding this comment.
Understood. I agree this approach sounds like the way to go.
| if err := pem.Encode(publicBuf, block); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| case "EC PRIVATE KEY", "RSA PRIVATE KEY": |
There was a problem hiding this comment.
(im probably just out of the loop here, but...) Are EC and RSA the only acceptable types of private keys here?
There was a problem hiding this comment.
That's right. More to the point, splitCertKey expects sanitized PEM data, meaning PEM data that came from sanitizePEM, and sanitizePEM (by way of knownBlockDecoders and privateKeyBlockVerifier) always uses either the block type "RSA PRIVATE KEY" or "EC PRIVATE KEY" for private keys.
Miciah
force-pushed
the
BZ1843856-sanitize-TLS-config-that-has-key-bundled-with-cert
branch
from
74185d8
to
a5e9560
Compare
Modify the extended validation logic to copy any private key found in the certificate field of a route's TLS configuration to its key field. Extended validation validates a route's TLS configuration using any key that is specified in either the TLS configuration's certificate field or its key field. As a result, a route that specifies the certificate and key together in the certificate field may pass extended validation and be admitted. However, the certificate manager only wrote the certificate out if the TLS configuration had a nonempty key field. As a result, a route with a valid certificate and key could be admitted but the certificate and key not written out, which would cause HAProxy to fail to load. This commit fixes bug 1843856. https://bugzilla.redhat.com/show_bug.cgi?id=1843856 * pkg/router/routeapihelpers/validation.go (splitCertKey): New function. Take sanitized PEM data and split it into public and private parts. (ExtendedValidateRoute): Use splitCertKey to parse out any private parts that are in the certificate field of the TLS configuration. Prepend any private parts found in the certificate data to the TLS configuration's key. * pkg/router/routeapihelpers/validation_test.go: Add test cases.
Miciah
force-pushed
the
BZ1843856-sanitize-TLS-config-that-has-key-bundled-with-cert
branch
from
a5e9560
to
2dfa244
Compare
|
I added some comments, removed a spurious newline that the new logic was inserting when both |
|
@Miciah: This pull request references Bugzilla bug 1843856, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
|
New changes look good! Thanks for adding the additional unit test. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah, sgreene570 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:
Approvers can indicate their approval by writing |
|
@Miciah: All pull requests linked via external trackers have merged: openshift/router#136. Bugzilla bug 1843856 has been moved to the MODIFIED state. In response to this:
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. |
|
/cherry-pick release-4.5 |
|
@Miciah: new pull request created: #150 In response to this:
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. |
|
/cherry-pick release-4.4 |
|
/cherry-pick release-4.3 |
|
@Miciah: new pull request created: #151 In response to this:
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. |
|
@Miciah: new pull request created: #152 In response to this:
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. |
mrunalp
added
the
cherry-pick-approved
|
@Miciah: new pull request could not be created: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between openshift:release-4.5 and openshift-cherrypick-robot:cherry-pick-136-to-release-4.5"}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"} In response to this:
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. |
|
@Miciah: failed to push cherry-picked changes in GitHub: pushing failed, output: "To https://github.com/openshift-cherrypick-robot/router\n ! [rejected] cherry-pick-136-to-release-4.4 -> cherry-pick-136-to-release-4.4 (non-fast-forward)\nerror: failed to push some refs to 'https://openshift-cherrypick-robot:CENSORED@github.com/openshift-cherrypick-robot/router'\nhint: Updates were rejected because the tip of your current branch is behind\nhint: its remote counterpart. Integrate the remote changes (e.g.\nhint: 'git pull ...') before pushing again.\nhint: See the 'Note about fast-forwards' in 'git push --help' for details.\n", error: exit status 1 In response to this:
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. |
|
@Miciah: failed to push cherry-picked changes in GitHub: pushing failed, output: "To https://github.com/openshift-cherrypick-robot/router\n ! [rejected] cherry-pick-136-to-release-4.3 -> cherry-pick-136-to-release-4.3 (non-fast-forward)\nerror: failed to push some refs to 'https://openshift-cherrypick-robot:CENSORED@github.com/openshift-cherrypick-robot/router'\nhint: Updates were rejected because the tip of your current branch is behind\nhint: its remote counterpart. Integrate the remote changes (e.g.\nhint: 'git pull ...') before pushing again.\nhint: See the 'Note about fast-forwards' in 'git push --help' for details.\n", error: exit status 1 In response to this:
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. |
Modify the extended validation logic to copy any private key found in the certificate field of a route's TLS configuration to its key field.
Extended validation validates a route's TLS configuration using any key that is specified in either the TLS configuration's certificate field or its key field. As a result, a route that specifies the certificate and key together in the certificate field may pass extended validation and be admitted. However, before this commit, the certificate manager only wrote the certificate out if the TLS configuration had a nonempty key field. As a result, a route with a valid certificate and key could be admitted but the certificate and key not written out, which would cause HAProxy to fail to load.
pkg/router/routeapihelpers/validation.go(splitCertKey): New function. Take sanitized PEM data and split it into public and private parts.(
ExtendedValidateRoute): UsesplitCertKeyto parse out any private parts that are in the certificate field of the TLS configuration. Prepend any private parts found in the certificate data to the TLS configuration's key.pkg/router/routeapihelpers/validation_test.go: Add test cases.