systemd: backport tpm fixes

[flokli]

This backports #210896 and #213182 to staging-22.11.

I still need to check if it actually works :-)

Requested in #210896 (comment).

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[flokli]

Hm, this doesn't seem to be sufficient for me. It looks like the systemd patch needs to be updated:

machine # [    6.126209] systemd-cryptsetup[531]: Assertion '(_error) != 0' failed at src/cryptsetup/cryptsetup.c:1327, function attach_luks_or_plain_or_bitlk_by_tpm2(). Aborting.

ping @ElvishJerricco @RaitoBezarius @blitz @ymatsiuk @NickCao

Full build log: https://gist.github.com/flokli/9cdeddd1108eff0c268f3b511fae2a50

[ymatsiuk]

I don't think this patch is really needed for systemd version in 21.11

[flokli]

I don't think this patch is really needed for systemd version in 21.11

It was requested in #210896 (comment).

[ymatsiuk]

Oh I see, the systemd version in staging-22.11 is too old though, not in 21.11 sorry. 🤔 how was that even possible?

[flokli]

Oh I see, the systemd version in staging-22.11 is too old though, not in 21.11 sorry

Not sure I understand. staging-22.11 has 251.11, release-22.11 has 251.10.

This PR doesn't change any of that, it simply adds the ./0019-tpm2_context_init-fix-driver-name-checking.patch patch (which unfortunately is not sufficient).

[ymatsiuk]

Apologies for misleading

I'm a blind fool, confused 21.11 and 22.11 🤦🏻

[NickCao]

This looks like a different issue. Need further investigation.

[NickCao]

Strange that the commit (systemd/systemd-stable@542dbc6) is only in systemd v252.2 or later. How could the tpm2 init patch apply.
Edit: I was mislead by github, it is here: https://github.com/systemd/systemd-stable/blob/v251.11/src/shared/tpm2-util.c

[NickCao]

src/cryptsetup/cryptsetup.c:1327 is https://github.com/systemd/systemd-stable/blob/3402020d17ce9e75167a5415fafb54708edb1ebf/src/cryptsetup/cryptsetup.c#L1327, is our patching mangling with line numbers? A log function can't fail.

[flokli]

I mean, we could reflow this patch manually to make sure, and see if it still regresses. Or peek at the patched file during build, by inserting a cat in the build commands...

[NickCao]

Added cat (awk actually), still on the same line:
systemd> 1327- log_notice_errno(r, "TPM2 operation failed, falling back to traditional unlocking: %m");

[NickCao]

And yes, there is an assertion in that log function....
https://github.com/systemd/systemd/blob/6823b5bb99b605090d967d08586a4ce8fd5f989f/src/basic/log.h#L223

[NickCao]

So this looks like a logic error in the if else clauses.

1. r = attach_luks2_by_tpm2_via_plugin(cd, name, flags);
2. handle ENXIO
3. handle ENOENT
4. handle anything other than EOPNOTSUPP, EAGAIN, and no error just falls under this case
5. we cannot log something that's not an error???

[NickCao]

The issue is silently fixed in systemd/systemd@35ba2b4

[flokli]

So this also should have gone into systemd-stable?

[NickCao]

The commit message says: This finishes the implementation started in commit https://github.com/systemd/systemd/commit/1f895adac287b5f1b6b854caa586093616ccc172 ("cryptsetup: add libcryptsetup TPM2 PIN support"). Maybe tpm2 with pin is just not supported by older versions of systemd, thus the features was not backported.

[flokli]

Thanks for the digging! So certainly not something for us to backport on our own.

@alaviss If you're interested in driving this forward, please open a backport PR in the systemd-stable repo, otherwise I'd propose to only use this in 252 and later.

[alaviss]

I'm not sure why this is not working in the test VM because it is working on my system...

[hesiod]

Could this PR (with the current PATCH) be merged independently of fixing the outstanding cryptsetup issues?
I have a server running 22.11 that is affected by the underlying library path issue, and it is fixed by applying the patch. I'm using systemd credentials protected by the TPM for a number of services including systemd-networkd.service and nix-daemon.service, and since systemd fails starting the services which use LoadCredentialEncrypted without the patch the affected server was not accessible anymore.
I'm currently using an overlay to include the patch, but it would be more convenient for me if this PR is merged. OTOH, I understand if my use case is too specific and you'd rather fix the cryptsetup failure as well in this PR.

[flokli]

Could this PR (with the current PATCH) be merged independently of fixing the outstanding cryptsetup issues? I have a server running 22.11 that is affected by the underlying library path issue, and it is fixed by applying the patch. I'm using systemd credentials protected by the TPM for a number of services including systemd-networkd.service and nix-daemon.service, and since systemd fails starting the services which use LoadCredentialEncrypted without the patch the affected server was not accessible anymore. I'm currently using an overlay to include the patch, but it would be more convenient for me if this PR is merged. OTOH, I understand if my use case is too specific and you'd rather fix the cryptsetup failure as well in this PR.

Oh, i wasn't aware someone was using that. We currently don't have any tests covering it, so it's hard to see things are broken / would get fixed by it.

Can you provide / extend a VM test with your usecase? We could then backport that and the fix, so it'll serve as a nice régression test.

[hesiod]

@flokli I created a new NixOS test systemd-credentials-tpm2.
PRs:

• nixos/tests/systemd-credentials-tpm2: Add tests for systemd credentials #217254 (against release-22.11): fails (as expected) with "TPM2 driver name 'device' not valid, refusing"
• nixos/tests/systemd-credentials-tpm2: Add tests for systemd credentials #217255 (against master): succeeds

Not sure if it makes sense to actually backport the test to stable, but I created the PR anyway so anyone can test it easily.

[flokli]

I approved #217255, and once undrafted and merged, would cherry-pick it into this PR. Thanks!

[hesiod]

@flokli I just realized that the updated commit (with the meta.maintainers entry) won't work on release-22.11 directly since I didn't exist in the maintainers list back then. So you'd need to cherry-pick 62f73c7 (commit adding me to the maintainers list) as well, or you'd have to remove the meta.maintainers entry from the commit.

[flokli]

I'll do some compiling/testing and will then push here.

[flokli]

Alright, this now successfully fixes the new nixosTests.systemd-credentials-tpm2 for 22.11, PTAL.