Skip to content
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

Update securityContext for all deployments #90

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Jeroen0494
Copy link
Contributor

Hi,

Changes

  • Set namespace for all resources, previously missing
  • Allow setting a path for the ingresses
  • Update the security context for all resources

Explanation
This PR changes quite a few things.Containers will run with the least amount of privileges required to run.

  • Metabase will run as a non-root user by default.
  • All simple cURL container will run as non-root by default
  • Drop all capabilities where possible. This will reduce the attack surface of containers running as root.

User facing changes
Exiting installations will fail to run without also updating the owners of files already existing on the filesystem. To mitigate this, we can do one of two things:

  • List user facing change in the release notes
  • Create an init container which chowns all of the files and sets the appropriate context. This doesn't always work, because for example when Metabase is run as a non-root user, permissions are also different, not just owner and group.

@Jeroen0494
Copy link
Contributor Author

Rebased on current main, forgot to sync initially.

@Jeroen0494
Copy link
Contributor Author

The Metabase doesn't seem to be configured when navigating to the URL. This will need some more investigating. Which component does the setup for this database?

@Jeroen0494 Jeroen0494 force-pushed the feat/security branch 2 times, most recently from 382893a to 9618767 Compare March 3, 2023 20:51
@he2ss
Copy link
Member

he2ss commented Apr 6, 2023

Hi @Jeroen0494,

Thanks for doing this PR.
Is it ready for review ?

@Jeroen0494
Copy link
Contributor Author

Hi @Jeroen0494,

Thanks for doing this PR. Is it ready for review ?

Hi, yes it is ready.

@LaurenceJJones LaurenceJJones requested a review from he2ss May 9, 2023 12:22
@he2ss
Copy link
Member

he2ss commented May 24, 2023

Hi, Sorry for the delay.

I tested the PR, It's not working when you enable a notification plugin.
You may need more capabilities to add so crowdsec can fork and exec.

@Jeroen0494
Copy link
Contributor Author

Hi, Sorry for the delay.

I tested the PR, It's not working when you enable a notification plugin. You may need more capabilities to add so crowdsec can fork and exec.

Hi, thank you for testing it!
Can you provide me with more details? Which component, logging, describe etc.

@he2ss
Copy link
Member

he2ss commented Jun 5, 2023

Hi @Jeroen0494,

Sorry for the delay. Yes, so basically, to reproduce the behavior:

  • enable slack notifications using the values.yaml
config:
  profiles.yaml: |
    name: default_ip_remediation
    #debug: true
    filters:
    - Alert.Remediation == true && Alert.GetScope() == "Ip"
    decisions:
    - type: ban
      duration: 4h
    notifications:
      - myslack
    on_success: break
  notifications:
    myslack.yaml: |
      type: slack
      name: myslack
      log_level: info
      format: |
        {{range . -}}
        {{$alert := . -}}
        {{range .Decisions -}}
        {{if $alert.Source.Cn -}}
        :flag-{{$alert.Source.Cn}}: <https://www.whois.com/whois/{{.Value}}|{{.Value}}> will get {{.Type}} for next {{.Duration}} for triggering {{.Scenario}} on machine '{{$alert.MachineID}}'. <https://www.shodan.io/host/{{.Value}}|Shodan>{{end}}
        {{if not $alert.Source.Cn -}}
        :pirate_flag: <https://www.whois.com/whois/{{.Value}}|{{.Value}}> will get {{.Type}} for next {{.Duration}} for triggering {{.Scenario}} on machine '{{$alert.MachineID}}'.  <https://www.shodan.io/host/{{.Value}}|Shodan>{{end}}
        {{end -}}
        {{end -}}
      webhook: <slack_webhook>
  • helm install crowdsec

On the installation, the crowdsec-lapi pod will crash with this error :

time="10-05-2023 10:21:56" level=fatal msg="api server init: unable to run local API: while loading plugin: fork/exec /usr/local/lib/crowdsec/plugins/notification-slack: operation not permitted"
time="10-05-2023 10:21:56" level=fatal msg="api server init: unable to run local API: while loading plugin: fork/exec /usr/local/lib/crowdsec/plugins/notification-slack: operation not permitted"

Because of not enough permissions

@he2ss
Copy link
Member

he2ss commented Feb 14, 2024

Hi @Jeroen0494,

Did you get any chance to look into the issue about the permissions ?

@Jeroen0494
Copy link
Contributor Author

Hi, eh, no I have not because I completely forgot about this PR.
I'll make the capabilities variable, and set the default in the values.yaml instead.

@flo-mic
Copy link
Contributor

flo-mic commented Feb 29, 2024

what is the current status of this PR? Would like to see this merged for security hardenings of crowdsec. Currently the chart is deploying the workloads with to privileged permissions

@Jeroen0494
Copy link
Contributor Author

Haven't gotten to it yet.

@Jeroen0494
Copy link
Contributor Author

There, I've moved all of the security settings to the values.yaml file, so you can override them if your plugins need more permissions.

@he2ss he2ss added this to the crowdsec-0.10 milestone Apr 2, 2024
@he2ss
Copy link
Member

he2ss commented Apr 2, 2024

Hi @Jeroen0494,

Thanks for the update. Two last suggestions we have on this PR :

  1. You can undo your change on the chart version. We will start making ourselves the release versions to include multiple features at once.

  2. As the securityContext is a new feature introduced. We think it will be a bad idea for user experience if we activate it by default. Especially if the users use the notifications plugins. What we imagine is that the securityContext is disabled by default, If enabled with a new value (for example in root: enableSecurityContext: true), then we apply the securityContext. What do you think about this?

@github-actions github-actions bot added needs/kind Kind label required needs/area labels Apr 2, 2024
@flo-mic
Copy link
Contributor

flo-mic commented Apr 23, 2024

@Jeroen0494 do you find some time to complete the last 2 suggestions from he2ss?

@he2ss he2ss modified the milestones: crowdsec-0.10, cr, crowdsec-0.11 Apr 23, 2024
Signed-off-by: Jeroen Rijken <[email protected]>
@Jeroen0494
Copy link
Contributor Author

Hi @Jeroen0494,

Thanks for the update. Two last suggestions we have on this PR :

  1. You can undo your change on the chart version. We will start making ourselves the release versions to include multiple features at once.
  2. As the securityContext is a new feature introduced. We think it will be a bad idea for user experience if we activate it by default. Especially if the users use the notifications plugins. What we imagine is that the securityContext is disabled by default, If enabled with a new value (for example in root: enableSecurityContext: true), then we apply the securityContext. What do you think about this?

I can do it one of two ways.

  • Use a boolean to enable or disable the security context
  • Put all security context variable in the values file, but commented out (or at least, comment out the invasive ones)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/area needs/kind Kind label required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants