-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
pomerium: init at 0.13.3 #108745
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
pomerium: init at 0.13.3 #108745
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. I have left reviewed the module portion of this PR and left some comments I hope you will find useful. Let me know if you have any questions, etc...
e3c0b5e
to
add12a3
Compare
@lukegb looks like a failed test. |
@aanderse Thanks! Should be ready for review again. |
@lukegb please see above comment: #108745 (comment) |
done! sorry for the delay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some great work. I'm not familiar with pomerium, but the test suite looks solid and the systemd service has an impressive amount of hardening 😃
My only gripe is with the use of LoadCredential. If pomerium does support hot reloads via process signals then I would prefer to see CERTIFICATE[_KEY]_FILE
env vars set to the full path to the cert. This creates a new problem in that you would also need to change how DynamicUser is used. From what I understand of the DynamicUser docs, setting Group to "acme" should be sufficient. As it is now your solution is more secure and in reality Apache has worked similarly in the past and no one complained 😉
Also see my note about the secretsFile permissions. It might make sense to add it to the LoadCredential option too, so that it can be owned and readable only by root.
}; | ||
}; | ||
|
||
# postRun hooks on cert renew can't be used to restart Nginx since renewal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Says "Nginx" here ;)
type = format.type; | ||
}; | ||
|
||
secretsFile = mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the use of DynamicUser in the systemd config, what would be the proper way to secure a secrets file on the local filesystem from other users?
Was just about to try out pomerium. This looks great to me. Thank you. |
I think this pr is ready to be merged. @lukegb can you please update pomerium to v0.13.2 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukegb I just tried pomerium 0.13.3. The proposed changes build on my machine. I also tried the test case. It works great. Thank you for your contribution.
Bump pomerium to v0.13.3. Co-authored-by: contrun <uuuuuu@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
}; | ||
varFlags = concatStringsSep " " (mapAttrsToList (name: value: "-X github.com/pomerium/pomerium/internal/version.${name}=${value}") setVars); | ||
in [ | ||
"-ldflags=${varFlags}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing -s -w.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to be set in pkgs/development/go-modules/generic/default.nix as a default, so it's not like we're overriding it off, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably does not work for every package. Should be set anyway when specifying ldflags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe does not work with all packages. Should be set anyway when specifying ldflags.
Motivation for this change
Packages the Pomerium authenticating HTTP proxy.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)