Skip to content

Add support for encrypted credentials#19995

Merged
poettering merged 14 commits intosystemd:mainfrom
poettering:cred-tool
Jul 8, 2021
Merged

Add support for encrypted credentials#19995
poettering merged 14 commits intosystemd:mainfrom
poettering:cred-tool

Conversation

@poettering
Copy link
Copy Markdown
Member

@poettering poettering commented Jun 22, 2021 •
edited
Loading

This adds LoadCredentialEncrypted= and SetCredentialEncrypted=, similar in style to LoadCredential=/SetCredential=, but taking encrypted credentials. These credentials are symmetrically encrypted with a TPM2 key and/or a key stored in /var/lib/systemd/, and only decrypted the instant a service needing them is started.

This has the benefit that credentials can remain encrypted on disk, until they are needed. It also has the benefit that these credentials can be stored on untrusted storage (e.g. inside an initrd or an ESP) and still be used reasonably securely. Moreover, to steal a credential it is no longer sufficient to copy the storage disk, but the TPM2 chip has be stolen too. Additionally, it has the benefit that the keys can be pasted directly inside of unit files this way. Unit files are considered to be readable by unprivileged users after all, but if any embedded credentials are encrypted they aren't compromised by this.

Focus is on making it trivially easy to bind credentials to the TPM2, but provide a reasonably secure fallback in environments where TPM2 is not available.

This adds a tool "systemd-creds" for encrypting/decrypting the credentials easily.

Docs and integration tests still missing.

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jun 22, 2021

This pull request introduces 2 alerts when merging 39c85df into b905f3b - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@bluca
Copy link
Copy Markdown
Member

bluca commented Jun 22, 2021

Nicea feature! And I have a use case for this coming up, so yay! Will look in detail tomorrow, in the meanwhile the CI is unhappy:

==32344==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000015 (pc 0x7f8678a988f2 bp 0x7ffc668f5900 sp 0x7ffc668f5800 T0)
==32344==The signal is caused by a READ memory access.
==32344==Hint: address points to the zero page.
    #0 0x7f8678a988f1 in extract_first_word ../../../../src/basic/extract-word.c:35
    #1 0x5596d24076f2 in config_parse_set_credential ../../../../src/core/load-fragment.c:4493
    #2 0x7f86787d07f4 in next_assignment ../../../../src/shared/conf-parser.c:137
    #3 0x7f86787d181e in parse_line ../../../../src/shared/conf-parser.c:244
    #4 0x7f86787d2a40 in config_parse ../../../../src/shared/conf-parser.c:384
    #5 0x5596d23cc1af in LLVMFuzzerTestOneInput ../../../../src/core/fuzz-unit-file.c:74
    #6 0x5596d23ccb10 in main ../../../../src/fuzz/fuzz-main.c:50
    #7 0x7f86771d80b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #8 0x5596d23cab5d in _start (/home/runner/work/systemd/systemd/build/test/fuzz/sanitize-address-undefined-fuzzers/fuzz-unit-file+0x573b5d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ../../../../src/basic/extract-word.c:35 in extract_first_word

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jun 23, 2021

This pull request introduces 2 alerts when merging 6f4ea2e into b905f3b - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jun 23, 2021

This pull request introduces 2 alerts when merging abfe702 into b905f3b - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jun 23, 2021

This pull request introduces 2 alerts when merging 1cbae50 into b905f3b - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@poettering
Copy link
Copy Markdown
Member Author

poettering commented Jun 24, 2021

Added docs and test. This is ready for review I guess. Post 249 material of course.

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jun 24, 2021

This pull request introduces 2 alerts when merging c2c4253 into 6222acc - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jun 24, 2021

This pull request introduces 2 alerts when merging 1da349a into b80ef40 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jun 24, 2021

This pull request introduces 2 alerts when merging 09c9453 into 86e24d6 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jun 24, 2021

This pull request introduces 2 alerts when merging 9806867 into eb70d94 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

bluca
bluca reviewed Jun 25, 2021
View reviewed changes
bluca
bluca reviewed Jun 25, 2021
View reviewed changes
keszybz
keszybz reviewed Jun 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot. It's genius in its simplicity and versatility.

I think we shouldn't merge this before v249 though, to give it a bit of time to settle. I'll do a proper review later.

bluca
bluca reviewed Jun 25, 2021
View reviewed changes
bluca
bluca reviewed Jun 25, 2021
View reviewed changes
bluca
bluca reviewed Jun 25, 2021
View reviewed changes
bluca
bluca reviewed Jun 25, 2021
View reviewed changes
@bluca
Copy link
Copy Markdown
Member

bluca commented Jul 1, 2021

Force pushed a new version, addressing all issues (except for the dbus docs one)

Maybe we should ask Github to enhance CoPilot to create those docs for us!

bluca
bluca approved these changes Jul 1, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@bluca bluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jul 1, 2021

This pull request introduces 2 alerts when merging 556b682 into 66e6128 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@poettering poettering added this to the v250 milestone Jul 1, 2021
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jul 7, 2021

This pull request introduces 2 alerts when merging 47b2b66 into f627855 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

poettering added 14 commits July 8, 2021 09:28
@poettering
sd-id128: make sure sd_id128_get_machine_app_specific() logic also wo…
011d129 
…rks without "khash"

So, as it turns out AF_ALG is turned off in a lot of kernels/container
environments, including our CI. Hence, if we link against OpenSSL
anyway, let's just use that client side. It's also faster.

One of those days we should drop the khash code, and ust use OpenSSL,
once the licensing issues are resolved.
@poettering
blockdev-util: add fd-based flavour of get_block_device()
0bfef8b
@poettering
fs-util: add fd-based flavour of path_is_encrypted()
91358db
@poettering
chattr-util: generalize chattr manipulation for files with secrets fr…
c1631ee 
…om journalctl

This moves the code for setting chattr file attributes appropriate for
"secrets" files from journalctl into generic chattr-util.c code so that
we can use it elsewhere.

Also, let's reuse the "bitwise" logic already implemented in the chattr
code, instead of doing it again.
@poettering
fileio: optionally allow interpreting file size as limit
7b0da71
@poettering
hexdecoct: optionally, line break base64 encoded data
82b4ec4
@poettering
util: move src/basic/creds-util.[ch] → src/shared/
8f860b4 
This is preparation for adding encryption support to the credentials
logic, and we thus would like to add more deps. Let's hence move things
from src/basic/ to src/shared, so that we can rely on the OpenSSL
utilities already in src/shared.
@poettering
creds-util: add infra for encrypting/decrypting credentials
21bc0b6
@poettering
creds: add a new tool for listing/showing/encrypting/decrypting crede…
5945640 
…ntials
@poettering
pid1: add support for encrypted credentials
43144be
@poettering
test: extend credentials test to cover encrypted credentials
c69620e
@poettering
man: add man page for "systemd-creds"
c1017f6
@poettering
man: document the new (Load|Set)CredentialEncrypted= settings
8a6a781
@poettering
update TODO
199b097
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jul 8, 2021

This pull request introduces 2 alerts when merging 199b097 into 105a424 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed needs-rebase labels Jul 8, 2021
@poettering poettering changed the title RFC: Add support for encrypted credentials Add support for encrypted credentials Jul 8, 2021
@poettering poettering merged commit 19755bc into systemd:main Jul 8, 2021
jameshilliard
jameshilliard reviewed Sep 23, 2021
View reviewed changes
TODO

* credentials system:
- acquire from kernel command line
- acquire from EFI variable?
Copy link
Copy Markdown
Contributor

@jameshilliard jameshilliard Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea a lot, I have to support a number of boards that don't have dedicated TPM's where it would be quite handy to be able to bind authentication credentials to motherboards, this could be used for a number of use cases such as:

  • ensuring that one can't accidentally duplicate authentication credentials when cloning hard drives by forcing a regeneration of credentials if decryption fails due to a missing EFI variable
  • simplify secure hard drive reuse/resale during upgrade cycles in cases where the hard disks are replaced with their associated motherboards being kept in operation
  • providing a clean transition path to more secure TPM based credential storage

Copy link
Copy Markdown
Member Author

@poettering poettering Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be delighted to review a patch for this

Copy link
Copy Markdown
Contributor

@jameshilliard jameshilliard Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder...would this actually be possible to do already by just redirecting /var/lib/systemd/credentials.secret to something in /sys/firmware/efi/efivars?

Not sure how best/where to implement this though, would we want to have this integrated with systemd-boot in some way or just pull it from efivars once booted?

@ju1m ju1m mentioned this pull request Oct 23, 2021
Closed
26 tasks
@bluca bluca mentioned this pull request Dec 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keszybz keszybz keszybz left review comments

@bluca bluca bluca approved these changes

+1 more reviewer

@jameshilliard jameshilliard jameshilliard left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed journal new-feature pid1 tpm2

Milestone

v250

Development

Successfully merging this pull request may close these issues.

4 participants

@poettering @bluca @keszybz @jameshilliard