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

doc: TRG 4.07: Read-Only Filesystem (DRAFT) #414

Merged
merged 20 commits into from
Nov 22, 2023

Conversation

SSIRKC
Copy link
Contributor

@SSIRKC SSIRKC commented Oct 4, 2023

Description

This is a TRG docs addition as announced by the consortia security team. This commit introduces the read-only filesystem security context as a new recommended setting.

This pull only adresses the docs.

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

…am. This commit introduces the read-only filesystem security topic as a new mandatory check for Tractus-X source code configuration.
@SSIRKC SSIRKC added the documentation Improvements or additions to documentation label Oct 4, 2023
Added line at the end
@SSIRKC
Copy link
Contributor Author

SSIRKC commented Oct 4, 2023

Created by the consortia security team.


## Description

Whether this container has a read-only root filesystem. Default is false.
Copy link
Contributor

Choose a reason for hiding this comment

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

The description should explain potentially more than just a read-only filesystem. like implications, best practices (what to do with log files, what to do in other cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the input! I will add to that

@AngelikaWittek
Copy link
Contributor

Hi,
please have a look to our draft /change process:
https://eclipse-tractusx.github.io/docs/release

Thanks
Angelika

Added better description as suggested and added release guideline suggestions.
@SSIRKC
Copy link
Contributor Author

SSIRKC commented Oct 5, 2023

Added the requested suggestions.

SSIRKC added 5 commits October 5, 2023 15:28
Added trailing spaces
Removed spaces
removed space
changed directory to trg 0 as requested
@SSIRKC SSIRKC changed the title TRG Addition: Read-Only Filesystem | doc doc: TRG 4.07: Read-Only Filesystem (DRAFT) Oct 6, 2023
@SSIRKC
Copy link
Contributor Author

SSIRKC commented Oct 6, 2023

Fixed typos, mandatory date and directory.

Copy link
Contributor

@drcgjung drcgjung left a comment

Choose a reason for hiding this comment

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

Maybe we could add a hint how to nevertheless mount a (temporary) writable folder if the container executables need to spool something:

Temporary Folders (If needed)

In the case that an executable in your container should need a temporary folder for logging or spooling purposes, you can mount a writable emptydir volume as follows:

apiVersion: v1  
kind: Pod  
metadata:  
  name: temporary-folder
spec:  
  containers:  
   - name: sample-container-which-needs-temporary-folder
      volumeMounts:
            - name: tempfolder
              mountPath: /tmp
              readOnly: false
      volumes:
        - name: tempfolder
          emptyDir: {}

@evegufy
Copy link
Contributor

evegufy commented Oct 6, 2023

@SSIRKC I appreciate the effort of improving the securityContext! I'd need to test the change.
Still, wouldn't it make sense to integrate this in this TRG here?: https://github.com/SSIRKC/eclipse-tractusx.github.io/blob/patch-1/docs/release/trg-4/trg-4-03.md

@florianrusch-zf
Copy link
Contributor

Additional note: This must be able to be overridden via the Helm Charts. Of course it should be true by default, but the human operator should be able to decide whether to keep it or not. E.g. OpenShift needs some special security settings regarding securityContexts.

Furthermore, this TRG could also mention the other security context settings and recommend best practices. For example, we already have a good example in the EDC Helm Chart:

Co-authored-by: Florian Rusch (ZF Friedrichshafen AG) <[email protected]>
@SSIRKC
Copy link
Contributor Author

SSIRKC commented Oct 10, 2023

Maybe we could add a hint how to nevertheless mount a (temporary) writable folder if the container executables need to spool something:

Temporary Folders (If needed)

In the case that an executable in your container should need a temporary folder for logging or spooling purposes, you can mount a writable emptydir volume as follows:

apiVersion: v1  
kind: Pod  
metadata:  
  name: temporary-folder
spec:  
  containers:  
   - name: sample-container-which-needs-temporary-folder
      volumeMounts:
            - name: tempfolder
              mountPath: /tmp
              readOnly: false
      volumes:
        - name: tempfolder
          emptyDir: {}

I will add this to the document :)

added tmp mount insturctions by CJung
@SSIRKC
Copy link
Contributor Author

SSIRKC commented Oct 10, 2023

@evegufy I appreciate the effort of improving the securityContext! I'd need to test the change. Still, wouldn't it make sense to integrate this in this TRG here?: https://github.com/SSIRKC/eclipse-tractusx.github.io/blob/patch-1/docs/release/trg-4/trg-4-03.md

Hi Evelyn,

yes this is absolutely correct. My source from OWASP also puts this topics together in "weak configurations". It would make sense to rename 4-03 to weak configurations and consolidate all topics. For now I set up a new sub-chapter to keep it a bit more simple and less overwhelming for now. But this is certainly a good idea to consolidate these at some point :)

Added implementation samples by Florian
@SSIRKC
Copy link
Contributor Author

SSIRKC commented Oct 10, 2023

Additional note: This must be able to be overridden via the Helm Charts. Of course it should be true by default, but the human operator should be able to decide whether to keep it or not. E.g. OpenShift needs some special security settings regarding securityContexts.

Furthermore, this TRG could also mention the other security context settings and recommend best practices. For example, we already have a good example in the EDC Helm Chart:

Hi @florianrusch-zf ,

thanks for the great references. I didnt know about these. I added them to the implementations section a further reference.

@SSIRKC
Copy link
Contributor Author

SSIRKC commented Oct 10, 2023

Update 10.10: All requested changes applied or checked.

Copy link
Contributor

@florianrusch-zf florianrusch-zf left a comment

Choose a reason for hiding this comment

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

I would recommend reworking this whole TRG so that it doesn't just focus on the read-only file system. Instead, it should focus on setting the securityContext in the right way and also give guidance on how to do that in relation to our helm charts, because we don't provide any simple k8s resources.

WDYT?

docs/release/trg-0/trg-4-07.md Outdated Show resolved Hide resolved
Co-authored-by: Florian Rusch (ZF Friedrichshafen AG) <[email protected]>
@SSIRKC
Copy link
Contributor Author

SSIRKC commented Oct 13, 2023

I would recommend reworking this whole TRG so that it doesnt just focus on the read-only file system. Instead, it should focus on setting the securityContext` in the right way and also give guidance on how to do that in relation to our helm charts, because we don't provide any simple k8s resources.

WDYT?

Hi Florian,

we can have a more general TRG also as Evelyn stated there is 4.03 that contains also settings about the security context.
This could be consolidated and be called "security context".

However, I actually like that the TRGs are short reads and rather simple to understand.
I also talked with Siggi about this one and the goal disussed was to make it easy to read.

But I think this could be a democratic decision.
If you think we should merge you can comment on this thread or give a thumps up to merge the TRGs.

Copy link
Contributor Author

@SSIRKC SSIRKC left a comment

Choose a reason for hiding this comment

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

Changes applied. The consideration of merging was discussed in the office hours. The idea will wont be considered due to the mentioned reasons of keeping the articles smaller and simpler and fitting them better into the checklist format. Making them iteratively more efficient.

@SSIRKC
Copy link
Contributor Author

SSIRKC commented Nov 2, 2023

The pull request was held open for feedback and suggestions til the mentioned date. All feedback was considered and applied or further discussed. I request to merge these changes into 4.07 @Siegfriedk

@evegufy
Copy link
Contributor

evegufy commented Nov 8, 2023

@SSIRKC Just already giving a heads up: depending on the base image a product uses there will be more enabling needed than just adding 'readOnlyRootFilesystem: true' to the securityContext.

For instance, products using nginx-unprivileged will encounter the following error:
[emerg] 1#1: mkdir() "/tmp/proxy_temp" failed (30: Read-only file system)
nginx: [emerg] mkdir() "/tmp/proxy_temp" failed (30: Read-only file system)
Presumable solution:
image
https://blog.asksven.io/posts/securing-kubernetes-configuration/

Also for the dotnet base image, an additional tweak is necessary but that's not relevant for other teams.

Added fixes to overwrite tmp
@SSIRKC
Copy link
Contributor Author

SSIRKC commented Nov 9, 2023

Hi @evegufy,

thanks for the feedback. I added the settings to make tmp being able to be overwritten and mounted.
Also thanks for the link I also figured the NET_RAW setting might be another common issue.

Can you explain what issue is caused for dotnet containing setups?
I would add this aswell since this is a valid issue that has been found.

Thanks for your help! It is very important to iron out the issues with your feedback.
Kind regards
@SSIRKC

added blanks
@evegufy
Copy link
Contributor

evegufy commented Nov 9, 2023

Hi @evegufy,

thanks for the feedback. I added the settings to make tmp being able to be overwritten and mounted. Also thanks for the link I also figured the NET_RAW setting might be another common issue.

Can you explain what issue is caused for dotnet containing setups? I would add this aswell since this is a valid issue that has been found.

Thanks for your help! It is very important to iron out the issues with your feedback. Kind regards @SSIRKC

Hi @SSIRKC

sure, I'm glab to help.
Just be aware, that those are presumable solutions, I didn't try them out after encountering the errors due to time constraints as I just wanted to get an impression of the effort of implementing this TRG which in the best case could have been just adding 'readOnlyRootFilesystem: true' to the securityContext.

Regarding the dotnet base image, I encountered the following error:
'Failed to create CoreCLR, HRESULT: 0x8007000E'
The presumable solution is to set the COMPlus_EnableDiagnostics env var to 0 .

@Siegfriedk
Copy link
Contributor

@SSIRKC pls write to me on teams if it is ready to merge

@Siegfriedk Siegfriedk merged commit c8df6df into eclipse-tractusx:main Nov 22, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants