elastic / package-spec

EPR package specifications
Other
18 stars 73 forks source link

[Change Proposal] csp-rule-template asset type #263

Closed eyalkraft closed 2 years ago

eyalkraft commented 2 years ago

The problem

We at the CSPM (cloud security posture management) team in Elastic are starting to build a new CSP product into the Elastic stack. One of the very basic building blocks of our CSP solution are rules - these rules define certain requirements in the realm of cloud security. For example: "AWS account is not allowed to have public S3 buckets". A collection of these rules compose a Benchmark, for example the "Kubernetes CIS Benchmark". Each one of our rules could be enabled/disabled by the user. These rules are different by nature from the security-rule type of assets that exist today, As the current security-rules belong to the detection engine and run periodically in Kibana(/Elasticsearch?), and our rules are not part of the detection engine and are destined to run on the Elastic Agent. To represent these rules in Kibana we intend registering a new saved object type.

Possible solutions

1. Introduce a new asset type called csp-rule to the package-spec.

Required changes:

Pros:

Cons:

2. Use the existing asset type security-rule for our rules as well + Refactor the detection engine rules in Kibana

Required changes:

Pros:

Cons:

3. Use the existing asset type security-rule for our rules as well + Leave the rules in the detection engine

Required changes:

Pros:

Cons:

Wrap up

Either option 1 or 2 seem reasonable to me. We'd love to get some input from the detection engine team about their thoughts regarding option 2.

Stakeholders

Please feel free to tag relevant people CSPM team: @elastic/cloud-posture-security Integrations: @mtojek Detection engine: @elastic/security-detections-response-rules

kfirpeled commented 2 years ago

Thanks @eyalkraft for opening this proposal

Worth mentioning that on our roadmap we plan to have several benchmarks like: CIS for EKS, GCP and Azure, SOC2, HIPAA, GDPR, PCI. In case you wish to change the terminology and would like more context.

Currently the logic behind these rules are built-in inside our agent integration. However, we could think of use-cases where a user can add his own kind csp-rule that the agent would validate. And introduce his own kind of benchmarks rules.

amitkanfer commented 2 years ago

Does it make sense to avoid the term CSP and perhaps introduce something more related to the architecture? (eg. Agent rules or similar) Just a thought. Thanks for documenting this @eyalkraft

mtojek commented 2 years ago

Hi,

I understand that this issue is just for discussing the format of the csp-rule and it doesn't touch installation: elastic/kibana#124149, correct?

Let me ping other folks for comments:

@dover - Fleet UI @ruflin - Data Collection @brokensound77 - who used to push updates to security_detection_engine

eyalkraft commented 2 years ago

Hey @mtojek thanks for your reply. Correct, This issue is not about the installation of this asset.

It's also not about the format of csp-rule but about wether it should exist as an independent type of saved-object and asset or should it be a sub-type of the existing security-rule saved-object type and asset.

ruflin commented 2 years ago

These rules are different by nature from the security-rule type of assets that exist today, As the current security-rules belong to the detection engine and run periodically in Kibana(/Elasticsearch?), and our rules are not part of the detection engine and are destined to run on the Elastic Agent

More me, this makes it clear it should be a different saved object type. Likely the structure of the data is also different?

The name security-rule was given before these "new rules" were around. So it might be that the name is not ideal anymore. But it should not imply that the rules must be mixed.

For the discussion it would also be useful to share some example json docs with the content here to see how different these are from security rules.

@amitkanfer This is an interesting thought. I would first stay with a more specific name to not overgeneralise too early but it is something we should keep in mind during design.

eyalkraft commented 2 years ago

Correct, the structure of the data is different. Here is an example from a PR by @orouz

  {
    id: '1.1.1',
    name: 'Ensure that the API server pod specification file permissions are set to 644 or more restrictive (Automated)',
    description: 'Disable anonymous requests to the API server',
    rationale:
      'When enabled, requests that are not rejected by other configured authentication methods\nare treated as anonymous requests. These requests are then served by the API server. You\nshould rely on authentication to authorize access and disallow anonymous requests.\nIf you are using RBAC authorization, it is generally considered reasonable to allow\nanonymous access to the API Server for health checks and discovery purposes, and hence\nthis recommendation is not scored. However, you should consider whether anonymous\ndiscovery is an acceptable risk for your purposes.',
    impact: 'Anonymous requests will be rejected.',
    default_value: 'By default, anonymous access is enabled.',
    remediation:
      'Edit the API server pod specification file /etc/kubernetes/manifests/kubeapiserver.yaml on the master node and set the below parameter.\n--anonymous-auth=false',
    benchmark: { name: 'CIS', version: '1.4.1' },
    tags: [],
    enabled: true,
    muted: false,
    rego_code: '',
  },
  {
    id: '1.1.2',
    name: 'Ensure that the --basic-auth-file argument is not set (Scored)',
    description: 'Do not use basic authentication',
    rationale:
      'Basic authentication uses plaintext credentials for authentication. Currently, the basic\nauthentication credentials last indefinitely, and the password cannot be changed without\nrestarting API server. The basic authentication is currently supported for convenience.\nHence, basic authentication should not be used',
    impact:
      'You will have to configure and use alternate authentication mechanisms such as tokens and\ncertificates. Username and password for basic authentication could no longer be used.',
    default_value: 'By default, basic authentication is not set',
    remediation:
      'Follow the documentation and configure alternate mechanisms for authentication. Then,\nedit the API server pod specification file /etc/kubernetes/manifests/kubeapiserver.yaml on the master node and remove the --basic-auth-file=<filename>\nparameter.',
    benchmark: { name: 'CIS', version: '1.4.1' },
    tags: [],
    enabled: true,
    muted: false,
    rego_code: '',
  }
orouz commented 2 years ago

has a general asset type - saved_object been considered?

i imagine it comes with considerations of its own, but it would solve our problem and perhaps others too.

brokensound77 commented 2 years ago

Thanks for tagging me. I would be in favor of option one in order to avoid mixing rule sets which are completely unrelated. We still push regularly to that integration to support our out of band releases for the detection engine.

joshdover commented 2 years ago

Please see my comment here on a path forward that requires less architectural change: https://github.com/elastic/kibana/issues/124149#issuecomment-1030013986

joshdover commented 2 years ago

One thing I'd like to note is that these objects should probably not be Kibana Space-specific since they serve as templates for the entire cluster. We do not have a field in the spec for this today, but I'd recommend we add a namespaceType field which defaults to single and also allows agnostic which would match Kibana's naming of this feature.

joshdover commented 2 years ago

One thing I'd like to note is that these objects should probably not be Kibana Space-specific since they serve as templates for the entire cluster. We do not have a field in the spec for this today, but I'd recommend we add a namespaceType field which defaults to single and also allows agnostic which would match Kibana's naming of this feature.

Thinking about this more, I don't think we need a spec change for this. It's actually specified by the Saved Object type definition in the Kibana code itself and I don't think we need any special properties on the actual objects in the package in order to accomplish this. I think we can defer on this and revisit if I'm indeed wrong.

kfirpeled commented 2 years ago

Hey @joshdover, if the integrations are kibana space specific I think it would be best to keep the csp templates kibana-space specific as well.

Otherwise, updating the integration version on one space would effect also on the other space. which might not be expected by the user.

if integrations are not kibana-space specific, I'm less worried about it

eyalkraft commented 2 years ago

Following the decisions made on the related issue, Renaming this issue and opening a PR to close it.