-
Notifications
You must be signed in to change notification settings - Fork 756
CMP-3566: Ensure a CLF exists and check for secure URLs #14031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
CMP-3566: Ensure a CLF exists and check for secure URLs #14031
Conversation
Change rule's existence checks to any_exist. This means that any ClusterLogForwarder that has a url key, needs to comply. If no such forwarder exists, it is fine. Previously, the rule would enforce that at least one such forwarder existed.
|
verification pass. More details seen from the Jira ticket https://issues.redhat.com/browse/CMP-3566
|
|
/packit retest-failed |
| # By using the objects filter we ensure we are getting the object to query for its url. | ||
| filepath: "{{{ openshift_filtered_path('/apis/observability.openshift.io/v1/namespaces/openshift-logging/clusterlogforwarders', 'try [.items[].spec.outputs[][]|objects|select(.url != null).url] catch []') }}}" | ||
| yamlpath: "[:]" | ||
| check_existence: any_exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the rule will pass if no log forwarding is enabled?
https://github.com/OVALProject/Language/blob/master/docs/oval-common-schema.md#ExistenceEnumeration
Do we want at_least_one_exists here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the rule will pass if no log forwarding is enabled?
Yes, 🫠
But at_least_one_exists doesn't work either:
If only AzureMonitor is configured, no ClusterLogForwarder will be returned, and the posture is still PASS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhmdnd But If I'm pedantic, this rule is about ensuring that log forwarding uses TLS.
If no logs are forwarded, is it a PASS or FAIL?
There is a rule about enabling log forwarding: https://github.com/ComplianceAsCode/content/blob/master/applications/openshift/api-server/audit_log_forwarding_enabled/rule.yml
Unfortunately it is not automated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When at_least_one_exists was set and only AzureMonitor is configured, the rule will FAIL.
% oc get ccr -n openshift-compliance| grep audit-log-forwarding-uses-tls
ocp4-stig-audit-log-forwarding-uses-tls FAIL medium
upstream-ocp4-stig-audit-log-forwarding-uses-tls FAIL medium
% oc get clusterlogforwarders.observability.openshift.io clf-71770 -o=jsonpath={.spec.outputs} -n openshift-logging | jq -r
[
{
"azureMonitor": {
"authentication": {
"sharedKey": {
"key": "shared_key",
"secretName": "azure-secret-71770"
}
},
"customerId": "816936a6-c9f8-40f2-bda1-986582acd354",
"logType": "case71770app_log"
},
"name": "azure-app",
"type": "azureMonitor"
},
{
"azureMonitor": {
"authentication": {
"sharedKey": {
"key": "shared_key",
"secretName": "azure-secret-71770"
}
},
"customerId": "816936a6-c9f8-40f2-bda1-986582acd354",
"logType": "case71770infra_log"
},
"name": "azure-infra",
"type": "azureMonitor"
},
{
"azureMonitor": {
"authentication": {
"sharedKey": {
"key": "shared_key",
"secretName": "azure-secret-71770"
}
},
"customerId": "816936a6-c9f8-40f2-bda1-986582acd354",
"logType": "case71770audit_log"
},
"name": "azure-audit",
"type": "azureMonitor"
}
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try one thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be addressed now, this rule ensures that at least one secure CLF exists.
|
Maybe the same fix should be done for the old api too? |
Given that support for Logging Operator 5.x is ending soon: And that version 6.x only use observability API: I'll not update the old logging rule. |
@xiaojiey @Anna-Koudelkova Are you okay with this? |
|
The yaml template can't generate the OVAL check we need to be able to assess the clusterlogforwarders correctly. Wee need a check that is capable of ensuring that: - there is at least one clusterlogforwarder - Any clusterlogforwarder object that has .url, is secure
|
verification FAIL for the unsecure url udp://rsyslog.e2e-test-vector-syslog-cnrqt.svc:514:
|
The cluster logging operator added a bunch of new forwarding implementations that allow users to ship their logs to various platforms and components. Some ClusterLogForwarders use a url, others use a host. In cases where we can inspect the url, we want to check to make sure it's not using an insecure protocol (http, tcp, udp). In other cases, where the forwarder is shipping logs to a dedicated service (like AzureMonitor), we can't actually inspect the protocol because the Azure forwarding configuration only exposes a `host` attribute, which doesn't use a protocol as part of the host string. Instead, it's baked into the forwarding implementation. This adds complexity to the rule because we need to: - Check that at least one ClusterLogForwarder exists - Each ClusterLogForwarder is configured to encrypt traffic to the forwarding endpoint - Short-circuit the check for special case forwarders, like AzureMonitor, that don't specify the protocol in the endpoint url/host Instead of looking for secure endpoints in each forwarder, which aren't implemented consistently, this commit reverses the logic so that it asserts no "insecure" endpoints are in a forwarding configuration. This works better for cases like AzureMonitor because if the rule doesn't find a `url` in the forwarder, is has nothing to compare the protocol check to, which means it passes. If a forwarder is configured to use plain old `http`, it will fail because the check asserts none exist against regular expression modeling unencrypted protocols. At the same time, we're maintaining the behavior where the rule fails is no forwarders exist at all. I believe this is ultimately due to the fact that "any_exists" OVAL checks will PASS if no pattern matches are made (filtering a log forwarder with url=http://example.com will not match a regular expression only looking for secure protocols, resuling in a PASS when it should actually fail due to how "any_exists" handles non-existent matches).
283557f to
9073244
Compare
|
@yuumasato: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
/packit retest-failed |
|
Verification fail.
|
Description:
Rationale:
The cluster can be configured with log forwarders that are by default secure and use TLS, for example the AzureMonitor log forwarder. They don't have an URL key.
The existing check only checks the
urlof CLF that have it. If a CLF doesn't have it, we don't check it.Problem is that we cannot differentiate between non-existent CLF, and CLF without
urlkey. So we need to add a check for CLF existence.Fixes https://issues.redhat.com/browse/CMP-3566