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

nixos/keycloak: Security fixes + misc #121778

Merged
merged 9 commits into from
May 21, 2021
Merged

nixos/keycloak: Security fixes + misc #121778

merged 9 commits into from
May 21, 2021

Conversation

talyz
Copy link
Contributor

@talyz talyz commented May 5, 2021 •
edited
Loading

Motivation for this change

Security fixes for the keycloak module:

Miscellaneous improvements:

  • Implement better bash error handling
  • Move all database*-options into a database group / attribute
  • Split the certificatePrivateKeyBundle option into sslCertificate and sslCertificateKey
  • Improve readability by adding executables to PATH instead of referring to them directly
  • Test the HTTPS support
  • Add myself as module maintainer
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 5, 2021
@talyz talyz requested a review from ngerstle May 5, 2021 09:10
@talyz talyz added the 1.severity: security Issues which raise a security issue, or PRs that fix one label May 5, 2021
@talyz talyz mentioned this pull request May 5, 2021
Open
42 tasks
@talyz talyz force-pushed the keycloak-security branch from e585123 to 586f895 Compare May 14, 2021 08:38
@talyz talyz mentioned this pull request May 16, 2021
Closed
@talyz talyz force-pushed the keycloak-security branch from a5ad25b to 5ee43e2 Compare May 19, 2021 07:39
@talyz talyz requested review from a user and aanderse May 19, 2021 07:46
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 labels May 19, 2021
talyz added 9 commits May 21, 2021 13:08
@talyz
nixos/keycloak: Set the postgresql database password securely
d6727d2 
Feeding `psql` the password on the command line leaks it through the
`psql` process' `/proc/<pid>/cmdline` file. Using `echo` to put the
command in a file and then feeding `psql` the file should work around
this, since `echo` is a bash builtin and thus shouldn't spawn a new
process.
@talyz
nixos/keycloak: Improve bash error handling
c2bebf4
@talyz
nixos/keycloak: Set umask before copying sensitive files
8309368 
`install` copies the files before setting their mode, so there could
be a breif window where the secrets are readable by other users
without a strict umask.
@talyz
nixos/keycloak: Improve readablility by putting executables in PATH
d748c86
@talyz
nixos/keycloak: Add myself to maintainers
58614f8
@talyz
nixos/keycloak: frontendUrl always needs to be suffixed with /
83e406e 
In some places, Keycloak expects the frontendUrl to end with `/`, so
let's make sure it always does.
@talyz
nixos/keycloak: keycloak.database* -> keycloak.database.*
dbf91bc 
Move all database options to their own group / attribute. This makes
the configuration clearer and brings it in line with most other modern
modules.
@talyz
nixos/keycloak: Split certificatePrivateKeyBundle into two options
ba00b09 
Instead of requiring the user to bundle the certificate and private
key into a single file, provide separate options for them. This is
more in line with most other modules.
@talyz
keycloak.tests: Test HTTPS support
2d8a870
@talyz talyz force-pushed the keycloak-security branch from 5ee43e2 to 2d8a870 Compare May 21, 2021 11:10
@etu etu merged commit e9cca93 into NixOS:master May 21, 2021
@talyz talyz deleted the keycloak-security branch May 21, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngerstle ngerstle Awaiting requested review from ngerstle

@aanderse aanderse Awaiting requested review from aanderse

Assignees
No one assigned
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@talyz @etu