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.

Sign up for GitHub

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

Jump to bottom

chore: enforce pod security standards #895

Merged
merged 3 commits into from
Jun 6, 2024
Merged

chore: enforce pod security standards #895

merged 3 commits into from
Jun 6, 2024

Conversation

SdgJlbl
Copy link
Contributor

@SdgJlbl SdgJlbl commented Apr 29, 2024 •
edited

Fixes FL-1453

Description

This PR introduces the following changes to the security context:

  • for all containers and initContainers:
    • spec.containers[*].securityContext.allowPrivilegeEscalation=false
    • spec.containers[*].securityContext.runAsNonRoot=true
    • spec.containers[*].securityContext.seccompProfile.type=RuntimeDefault
    • spec.containers[*].securityContext.capabilities.drop=["ALL"]

The exceptions are : add-cert initContainer in several pods (if the local CA feature is used), wait-registry initContainer and kaniko initContainer in the registry-prepopulate pod.

  • for all pods:
    • spec.securityContext.runAsNonRoot=true
    • spec.securityContext.seccompProfile.type= RuntimeDefault

The exceptions are the kaniko pods spawned by the builder.

How to test:

Deploy locally (with skaffold), and then apply labels:

kubectl label --dry-run=server --overwrite ns org-2 pod-security.kubernetes.io/enforce=baseline

or

kubectl label --dry-run=server --overwrite ns org-2 pod-security.kubernetes.io/enforce=restricted

The baseline profile should yield no warning.
The restricted profile will show a few warnings, we are trying to remove.

@SdgJlbl SdgJlbl requested a review from a team as a code owner April 29, 2024 13:57
@linear Linear
Copy link

linear bot commented Apr 29, 2024

FL-1453 Ensure capabilities of substra-backend pods are restricted to the minimum

@SdgJlbl SdgJlbl changed the title Chore: enforce pod security standards chore: enforce pod security standards Apr 29, 2024
@SdgJlbl SdgJlbl marked this pull request as draft April 29, 2024 14:15
@ThibaultFy
Copy link
Member

/e2e --tests sdk

@Owlfred
Copy link

Owlfred commented Apr 29, 2024 •
edited

End to end tests: ❌ FAILURE

Jobs status:

“Boy, that escalated quickly.” ― Ron Burgundy, Anchorman: The Legend of Ron Burgundy

@ThibaultFy
Copy link
Member

It seems to miss a permission for accessing substra namespace

Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: could not get information about the resource Namespace "substra" in namespace "": namespaces "substra" is forbidden: User "e2e-tests@connect-314908.iam.gserviceaccount.com" cannot get resource "namespaces" in API group "" in the namespace "substra": requires one of ["container.namespaces.get"] permission(s).

@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Apr 30, 2024

It seems to miss a permission for accessing substra namespace

Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: could not get information about the resource Namespace "substra" in namespace "": namespaces "substra" is forbidden: User "e2e-tests@connect-314908.iam.gserviceaccount.com" cannot get resource "namespaces" in API group "" in the namespace "substra": requires one of ["container.namespaces.get"] permission(s).

The namespace.yaml is just an example to test, but I'll remove it from this PR, since it's confusing.

@SdgJlbl SdgJlbl force-pushed the chore/enforce-psa branch 6 times, most recently from c4971f1 to 9564fe3 Compare May 28, 2024 09:38
@Substra Substra deleted a comment from Owlfred May 28, 2024
@Substra Substra deleted a comment from Owlfred May 28, 2024
@Substra Substra deleted a comment from Owlfred May 28, 2024
@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented May 29, 2024

/e2e --tests=sdk,doc

@Owlfred
Copy link

Owlfred commented May 29, 2024 •
edited

End to end tests: ❌ FAILURE

Jobs status:

Too bad.

@Substra Substra deleted a comment from Owlfred May 29, 2024
@Substra Substra deleted a comment from Owlfred May 29, 2024
@thbcmlowk
Copy link
Contributor

/e2e --tests=sdk,doc

@Owlfred
Copy link

Owlfred commented May 29, 2024 •
edited

End to end tests: ✔️ SUCCESS

Aw yeah!

@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented May 29, 2024

/e2e --tests=sdk,doc

@Owlfred
Copy link

Owlfred commented May 29, 2024 •
edited

End to end tests: ✔️ SUCCESS

@SdgJlbl SdgJlbl marked this pull request as ready for review May 29, 2024 13:28
guilhem-barthes
guilhem-barthes approved these changes May 29, 2024
View reviewed changes
charts/substra-backend/values.yaml
Comment on lines -944 to -952
podSecurityContext:
runAsNonRoot: true
seccompProfile:
type: RuntimeDefault
containerSecurityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it not working there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe related to the chart version update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this has been lost in the various trials / rebase

The new containerSecurityContext is more restrictive, and since it's the only container in the pod, the podSecurityContext should not matter, but I'll add it again to be explicit.

thbcmlowk
thbcmlowk approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@thbcmlowk thbcmlowk left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we?

  1. Testing the charts in a alpha version before merging (once dev is available)
  2. Merging then testing the charts + code in alpha on dev

I would recommend going for (1) as the code update is very minor.

@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented May 30, 2024

/e2e --tests=sdk,doc

@Owlfred
Copy link

Owlfred commented May 30, 2024 •
edited

End to end tests: ✔️ SUCCESS

Congratulations!

@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented May 30, 2024

Are we?

1. Testing the charts in a `alpha` version before merging (once dev is available)

2. Merging then testing the charts + code in `alpha` on dev

I would recommend going for (1) as the code update is very minor.

I would also be in favour of 1 (and the whole thing will be tested at the next release anyway)

@SdgJlbl
chore: add and complete pod security context
3ebb47d 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl
chore: remove MinIO & Redis PDB that is not supported by the CI runner
f84aaf9 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl
chore: add seccompprofile to compute function pods
327ddb0 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Jun 6, 2024

Tested on dev with Camelyon, the compute plans ran correctly.
Screenshot 2024-06-06 at 16 01 30

@SdgJlbl SdgJlbl merged commit a16e76e into main Jun 6, 2024
10 checks passed
@SdgJlbl SdgJlbl deleted the chore/enforce-psa branch June 6, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guilhem-barthes guilhem-barthes guilhem-barthes approved these changes

@thbcmlowk thbcmlowk thbcmlowk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@SdgJlbl @ThibaultFy @Owlfred @thbcmlowk @guilhem-barthes