Remove mlock and replace with cgroups #363
Conversation
IohannesArnold
force-pushed
the
remove-mlock
branch
from
3a723b1
to
4e81291
Compare
|
Okay, @cipherboy I'm now ready for review and merging. The bao binary still builds; since not all the tests passed before, I didn't learn how to get the whole test suite running, and I'm not sure if I broke anything more. Many of these deleted lines are trivial; for review, the more substantial edits (if you trust me) are:
For the last of these, the website docs addition to discussion swap disabling or encryption on various OS's, I added a "Post-installation hardening" section to the "Installing OpenBao" page. I think this leaves something to be desired, but I think trying to do any better would lead to a more general reorganization of the docs. So I include my less-than-fully-satisfactory docs in this PR. Bigger changes to the docs are now on my mind for another one. 😉 |
|
@IohannesArnold I think #362 should fix the tests; I'm tracking down the last issue with the race tests, but having a hard time reproducing it locally... If you'll rebase after that lands we can probably get passing tests here :-) |
There was a problem hiding this comment.
This looks fairly good, thank you @IohannesArnold!
Some comments inline. What do you think about controlling cgroups directly in the binary? Would this work inside a container or not easily? Thinking something like https://pkg.go.dev/github.com/containerd/cgroups or https://pkg.go.dev/github.com/opencontainers/runc/libcontainer/cgroups, though I've not looked into how to use either. Was wondering if there might be more useful options to constrain external plugins with...
command/server/test-fixtures/config_custom_response_headers_multiple_listeners.hcl
Show resolved
Hide resolved
I don't have any experience with this either, so I can't say much. My gut reaction is that swap is really an OS feature, so considering how to secure it should be an OS concern, not the concern of a process. I was surprised how easy it was to find ways of encrypting swap for Windows and macOS. |
There was a problem hiding this comment.
Looking very good overall, thanks @IohannesArnold. I have two smaller remarks wrt. the BSD targets we support. Also I see this as a potential breaking change. Please provide a changelog entry and mark it as such
IohannesArnold
force-pushed
the
remove-mlock
branch
2 times, most recently
from
f8b84a1
to
4ba698b
Compare
|
Thanks @JanMa -- I've rebased against main HEAD (although it's since drifted again), got all the tests to pass, and believe I have addressed the issues that you and Alex have raised. I think this should be good to go for final approval. |
|
@IohannesArnold Looks like |
IohannesArnold
force-pushed
the
remove-mlock
branch
from
4ba698b
to
2fa5bd5
Compare
IohannesArnold
force-pushed
the
remove-mlock
branch
from
2fa5bd5
to
fb4f158
Compare
|
@JanMa Commit-final-FINAL-v2-for-real 😝 |
In [RFC openbao#354](openbao#354), the mlock implementation inherited from Vault was deemed buggy. Here it is ripped out of all core OpenBao code. A few stubs are retained for compatibility's sake: 1. The config file parser will still parse the setting "disable_mlock". It will do nothing when set to true, and it will immediately error if set to false (i.e. the user is explicitly expecting mlock to be enabled). 2. The dynamicSystemView struct has a MlockEnabled method so it can still implement pluginutil.RunnerUtil. This method now just returns false. All mlock code is RETAINED in all ./sdk files, because the question is not yet settled whether plugins built against the *OpenBao SDK* should be binary-compatible with Vault. If this is eventually resolved in the negative, most of the mlock related code in the SDK can be stubbed out. As mlock is no longer used, Docker-related scripts have also had setcap calls removed. In place of mlock, documentation has been added to draw attention to the danger of sensitive information leaking through swap space and stress the importance of disabling or encrypting swap on any platform, or on Linux, changing the cgroupv2 setting memory.swap.max to 0. This last option has also been included in the example systemd service file. Signed-off-by: John Arnold <[email protected]>
IohannesArnold
force-pushed
the
remove-mlock
branch
from
fb4f158
to
e53b2bb
Compare
|
🫠 |
|
Merged, thanks @IohannesArnold! \o/ And thanks for the review, @JanMa! |
In RFC #354, the mlock implementation inherited from Vault was deemed buggy. Here it is ripped out of all core OpenBao code. A few stubs are retained for compatibility's sake:
All mlock code is RETAINED in all ./sdk files, because the question is not yet settled whether plugins built against the OpenBao SDK should be binary-compatible with Vault. If this is eventually resolved in the negative, most of the mlock related code in the SDK can be subbed out.
As mlock is no longer used, Docker-related scripts have also had setcap calls removed.
In place of mlock, documentation has been added to draw attention to the danger of sensitive information leaking through swap space and stress the importance of disabling or encrypting swap on any platform, or on Linux, changing the cgroupv2 setting memory.swap.max to 0. This last option has also been included in the example systemd service file.