Skip to content

[WIP][1.1.0][BREAKING] Security Patches and General Improvements #18

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

Merged
merged 24 commits into from
Nov 11, 2022

Conversation

Haloghen
Copy link
Contributor

@Haloghen Haloghen commented Nov 6, 2022

  • Bumped Chart version to 1.0.13 (since we both forgot it :P)
  • Moved account data from ConfigMap to Secret, supporting an existing secret to provide username, password and token
  • Moved server password from ConfigMap to Secret, supporting an existing secret to provide game_password
  • Moved rcon password from ConfigMap to Secret, supporting an existing secret to provide rcon.password
  • Moved account data from server_settings. to account. in values.yaml
  • Moved server password from server_settings. to serverPassword. in values.yaml
  • Added account.accountSecret field to values.yaml
  • Added rcon.passwordSecret field to values.yaml
  • Added serverPassword.passwordSecret field to values.yaml
  • Set spec.template.spec.hostNetwork to false if the Service is different from a NodePort due to security best practices
  • Added a check for annotations inside values.yaml to avoid creating an empty metadata.annotations inside the Factorio Service
  • Removed rconpw from the ConfigMap and moved it to a dedicated Secret
  • Removed account data and game password from server-settings.json, adding them to said JSON via the InitContainer
  • Updated mod-downloader-configmap.yaml to fetch account data from Secrets

- Bumped Chart version to 1.0.13 (since we both forgot it :P)
- Moved account data from ConfigMap to Secret, supporting an existing secret to provide `username`, `password` and `token`
- Moved server password from ConfigMap to Secret, supporting an existing secret to provide `game_password`
- Moved rcon password from ConfigMap to Secret, supporting an existing secret to provide `rcon.password`
- Moved account data from `server_settings.` to `account.` in values.yaml
- Moved server password from `server_settings.` to `serverPassword.` in values.yaml
- Added `account.accountSecret` field to values.yaml
- Added `rcon.passwordSecret` field to values.yaml
- Added `serverPassword.passwordSecret` field to values.yaml
- Set `spec.template.spec.hostNetwork` to false if the Service is different from a NodePort due to security best practices
- Added a check for annotations inside values.yaml to avoid creating an empty `metadata.annotations` inside the Factorio Service
- Removed `rconpw` from the ConfigMap and moved it to a dedicated Secret
- Removed account data and game password from server-settings.json, adding them to said JSON via the InitContainer
- Updated mod-downloader-configmap.yaml to fetch account data from Secrets
@Haloghen Haloghen requested a review from SQLJames as a code owner November 6, 2022 16:22
Copy link
Owner

@SQLJames SQLJames left a comment

Choose a reason for hiding this comment

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

This looks good. Appreciate the changes :D

@SQLJames
Copy link
Owner

SQLJames commented Nov 6, 2022

There are a few issues that came up during the linting and test.

@Haloghen
Copy link
Contributor Author

Haloghen commented Nov 6, 2022

Yeah I know, fixing one thing at a time, unfortunately CT won't tell you evrything that's wrong at the same time, but it stops at the first error it notices.

@Haloghen
Copy link
Contributor Author

Haloghen commented Nov 6, 2022

@SQLJames this issue is going to need a bit more reasearch, I'll keep going tomorrow. If you have any idea why it won't boot with default values, please feel free to share it.

The first error reported in the logs is here.

@Haloghen
Copy link
Contributor Author

Haloghen commented Nov 7, 2022

@SQLJames I can happily announce I finally fixed the testing error, it was due to a non-functional test pod. I changed it to test RCON functionality, which proves that the server itself is working too. Push incoming with various fixes

- Changed helm test from wget (not useful with UDP ports) to RCON
- Changed deployment.yaml, service.yaml and rcon-service.yaml so that Factorio listens on its default port, but the 'external' port is customizable via the services
- Added CI values to be used when using CT install
- Fixed GH Workflow when merging into main branch
@Haloghen
Copy link
Contributor Author

Haloghen commented Nov 7, 2022

I must also add that this release is breaking for existing installations, should we ignore that and just release it as a major release or should we continue to support old installation by including the values I removed as 'deprecated' and to be definitively removed? Keep in mind that I had to move the account infos out of server_settings to avoid including them into the ConfigMap, since they're private information and should be stored only in a Secret

@Haloghen
Copy link
Contributor Author

Haloghen commented Nov 7, 2022

@SQLJames it is fully working now, let me know what you think about this and I will adjust the code accordingly. My suggestion is to release it as a new major release (e.g. version 1.1.0) since usually who uses FluxCD, ArgoCD or other CD solutions for helm (like me) doesn't use "latest" as chart version and will update manually once this release goes public.

@Haloghen Haloghen requested a review from SQLJames November 7, 2022 13:43
@SQLJames
Copy link
Owner

SQLJames commented Nov 8, 2022

@SQLJames it is fully working now, let me know what you think about this and I will adjust the code accordingly. My suggestion is to release it as a new major release (e.g. version 1.1.0) since usually who uses FluxCD, ArgoCD or other CD solutions for helm (like me) doesn't use "latest" as chart version and will update manually once this release goes public.

This is a valid point, I do think having the version being bumped from a patch to a new minor release would make sense with the breaking changes. I didn't think about that.
Would you mind updating the readme to reflect the changes to the chart, specifically the quick start and the mods section also?

@Haloghen Haloghen closed this Nov 8, 2022
@Haloghen Haloghen deleted the feature/security-patches branch November 8, 2022 09:11
@Haloghen Haloghen restored the feature/security-patches branch November 8, 2022 09:17
@Haloghen Haloghen reopened this Nov 8, 2022
@Haloghen
Copy link
Contributor Author

Haloghen commented Nov 8, 2022

Error on my side, deleted the wrong branch so the PR closed, I apoligize.

This is a valid point, I do think having the version being bumped from a patch to a new minor release would make sense with the breaking changes. I didn't think about that. Would you mind updating the readme to reflect the changes to the chart, specifically the quick start and the mods section also?

Working on it now. Since this has become a fully-fledged minor release, I'm going to add a couple more things that I think would improve this repository readability.

@Haloghen Haloghen changed the title [1.0.13] Security Patches for Various Aspects [1.1.0][BREAKING] Security Patches and General Improvements Nov 8, 2022
@Haloghen Haloghen changed the title [1.1.0][BREAKING] Security Patches and General Improvements [WIP][1.1.0][BREAKING] Security Patches and General Improvements Nov 9, 2022
@Haloghen
Copy link
Contributor Author

Haloghen commented Nov 9, 2022

I thinks it is ready for merging now, before merging I want to write a proper changelog of what changed and then I will request approval again.

@Haloghen
Copy link
Contributor Author

Haloghen commented Nov 9, 2022

Changelog

Breaking Changes

  • Moved account data from server_settings. to account. in values.yaml
  • Moved server password from server_settings. to serverPassword. in values.yaml

Non-Breaking Changes

  • Added account.accountSecret field to values.yaml
  • Added rcon.passwordSecret field to values.yaml
  • Added serverPassword.passwordSecret field to values.yaml
  • Changed default rcon.password to CHANGEMECHANGEME, which should be changed anyway
  • Changed default map_gen_settings.autoplace_controls from the standard values to {}, since by default map
    generation follows standard parameters if not overriden
  • Changed default admin_list, white_list and ban_list to [] instead of nil

Technical Changes

  • Fixed workflow not working in main branch
  • Created test values for chart-tester install action
  • Fixed non-working test pod, replaced wget with rcon, testing server rcon responsiveness, which in turn demonstrates server functionality
  • Moved account data from ConfigMap to Secret, supporting an existing secret to provide username, password and token
  • Moved server password from ConfigMap to Secret, supporting an existing secret to provide game_password
  • Moved rcon password from ConfigMap to Secret, supporting an existing secret to provide rcon.password
  • Set spec.template.spec.hostNetwork to false if the Service is different from a NodePort due to security best practices
  • Added a check for annotations inside values.yaml to avoid creating an empty metadata.annotations inside the Factorio Service
  • Removed rconpw from the ConfigMap and moved it to a dedicated Secret
  • Removed account data and game password from server-settings.json, adding them to said JSON via the InitContainer
  • Updated mod-downloader-configmap.yaml to fetch account data from Secrets
  • Changed services' internal port to be fixed, and external port binding editable to have a consistent setup

@SQLJames SQLJames merged commit 520b2b8 into SQLJames:main Nov 11, 2022
@SQLJames
Copy link
Owner

Sorry for the delay in getting back to this, Busy week at work with some deployments. I appreciate the work you contributed back to the project. This goes a long way in securing the chart and making it more robust :)

@Haloghen Haloghen deleted the feature/security-patches branch November 11, 2022 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants