-
Notifications
You must be signed in to change notification settings - Fork 33
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
systemd-cryptsetup dependency chain #327
base: develop
Are you sure you want to change the base?
Conversation
I am currently maintaining this in our variant repository as well, but would appreciate to get this upstream as discussed. |
Thanks for your contribution, and apologies for the latency -- things are a bit slow on account of the holidays. We'll have a look! |
That's okay! I have some patches to push to this after the holidays. There is an optional dependency on libargon2 also in cryptsetup which is not yet included. IT is a key derivation algorthim which has not yet been fips certified however. We want to use it ourselves as we have no fips requirements as such and it is a provably more secure key derivation algorithm than the pbkdf2 algo which is fips approved. |
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.
Back from vacation now and taking a look!
Are you OK if I push minor fix commits to your branch for this PR? Not sure if you enabled that flag but it could save some round trips for cleanup stuff that might otherwise annoy you.
libblkid = { path = "../util-linux" } | ||
libuuid = { path = "../util-linux" } | ||
libpopt = { path = "../libpopt" } | ||
systemd = { path = "../systemd" } |
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.
Since systemd also depends on cryptsetup, this would create a dependency cycle, which we'd need to break in one of two ways:
- adding a
systemd-bootstrap
package or similar (libsystemd
, ...) with the same sources and a trimmed down build, just enough to bootstrap cryptsetup - adding systemd headers and libraries to the SDK - effectively doing the bootstrap there
From a quick glance at the sources, it looks like the dependency is only used to find the path to the systemd tmpfiles.d directory, which we can specify as an argument to the configure script instead.
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.
Ah yes, that's true! For some reason this doesn't break the world for me, but I did build... one of them first and have probably never cleaned up my build space since.
@@ -0,0 +1,20 @@ | |||
[package] | |||
name = "aws-lc-fips" |
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.
Ideally this package would be named libcrypto
since it provides that shared library.
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.
I did name it this as we are also building libssl as per the comment below. I think for us, we do want libssl if possible, as we will probably set up remote systemd-journal with TLS soonish also (for audit logging).
We could split up the install packages though?
%cross_cmake ../ \ | ||
-GNinja \ | ||
-DCMAKE_BUILD_TYPE=RelWithDebInfo \ | ||
-DBUILD_SHARED_LIBS=ON \ |
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.
If nothing needs libssl.so
, only libcrypto.so
, I'd prefer to disable it to cut down on surface area for potential vulnerabilities:
-DBUILD_SHARED_LIBS=ON \ | |
-DBUILD_SHARED_LIBS=ON \ | |
-DBUILD_LIBSSL=OFF \ |
Firstly, thank you for the review!
I actually don't know how to enable that! But you are welcome to! wrt disabling building libssl.so I think that is a bit hairy, as systemd, with the I am unsure whether tss2 requires libssl or just libcrypto. I can try to see where things break, but it will probably be next week as we are in the middle of delivery this week. |
Co-authored-by: Ben Cressey <[email protected]>
Co-authored-by: Ben Cressey <[email protected]>
Co-authored-by: Ben Cressey <[email protected]>
Co-authored-by: Ben Cressey <[email protected]>
Co-authored-by: Ben Cressey <[email protected]>
Description of changes:
This introduces a chain of dependencies for enabling systemd-cryptsetup. Initially, I did not compile the cryptsetup binary itself and I did not package the systemd-cryptenroll binary as we would embed them in our admin container and in our bootstrap system, but I guess it would be more useful to have them part of the primary system? Willing to discuss though.
I am also unsure whether the devmapper-event library is needed or not.
Testing done:
I have made sure the binaries don't give linking errors when invoked, but I have not tested primary functionality yet. Will happen later.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.