OCPBUGS-9037: If mTLS is required and the canary receives a "certificate required" TLS error, consider the canary probe successful#961
Conversation
openshift-ci-robot
added
jira/severity-important
|
@rfredette: This pull request references Jira Issue OCPBUGS-9037, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-merge-robot
added
the
needs-rebase
…TLS error, consider the canary probe successful. Without an API to provide the canary with a valid client certificate, the canary probes are guaranteed to fail when mTLS is required by the default ingress controller. However, if the failure is determined to be because the canary is missing a client certificate, it's reasonable to assume that the ingress controller is functional. This change checks specifically for the TLS alert "certificate required", and if the default ingress controller also has mTLS required, the probe function returns nil, indicating a successful probe.
rfredette
force-pushed
the
ocpbugs-9037-canary-mtls
branch
from
46b2fbd to
4dc9007
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@rfredette: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/assign @frobware |
| // "certificate required", it means the canary lacks the client certificate necessary to complete its | ||
| // check. In that case, verify that the router is configured to require mTLS, and if so, assume the | ||
| // router is working as intended. | ||
| if opErr.Err.Error() == "tls: certificate required" { |
There was a problem hiding this comment.
Is this message the same in a FIPS-enabled cluster?
There was a problem hiding this comment.
This message is the same on FIPS, although as we discussed out-of-band, I need to see if the message is the same in all TLS versions since it was added in TLS 1.3.
There was a problem hiding this comment.
Could we summarise the testing you've already done with different TLS profiles?
There was a problem hiding this comment.
Yes, to summarize the testing with TLS profiles:
We only allow users to set the minimum TLS version, not the maximum. The canary checks are between the router and the ingress-operator pods, both of which support TLS 1.3, so regardless of what the minimum is set to, the connection gets negotiated to v1.3, so we get teh expected "certificate required" error.
| defer func() { | ||
| // Reset client TLS. | ||
| if err := kclient.Get(context.TODO(), defaultName, defaultIC); err != nil { | ||
| panic(fmt.Errorf("failed to get default ingresscontroller: %v", err)) |
There was a problem hiding this comment.
I don't think we need to panic here. We can just call t.Fatal.
| } | ||
| defaultIC.Spec.ClientTLS = originalClientTLS | ||
| if err := kclient.Update(context.TODO(), defaultIC); err != nil { | ||
| panic(fmt.Errorf("Failed to restore default ingress configuration: %w", err)) |
There was a problem hiding this comment.
I don't think we need to panic here. We can just call t.Fatal.
|
One concern that I have is:
is this really enough to assert status-code==200? |
|
Closing in favor of #978 |
|
@rfredette: This pull request references Jira Issue OCPBUGS-9037. The bug has been updated to no longer refer to the pull request using the external bug tracker. DetailsIn 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. |
4 participants
Without an API to provide the canary with a valid client certificate, the canary probes are guaranteed to fail when mTLS is required by the default ingress controller. However, if the failure is determined to be because the canary is missing a client certificate, it's reasonable to assume that the ingress controller is functional.
This change checks specifically for the TLS alert "certificate required", and if the default ingress controller also has mTLS required, the probe function returns nil, indicating a successful probe.