Improve configuration and change format to TOML #449

ansasaki · 2022-09-19T19:47:02Z

This introduces several improvements to the agent configuration.

The file format is modified from INI style files to TOML formatted
files.
The configuration options names were modified to follow the
enhancement proposal:
- https://github.com/keylime/enhancements/blob/master/72_config_and_simplify_tls.md
The configuration is constructed using an hierarchical organization
following the order:
- The default hardcoded values are loaded
- If a configuration file is found in /usr/etc/keylime/agent.conf, it
  is loaded and any option set overrides the default values
- Load configuration snippets from /usr/etc/keylime/agent.conf.d/*,
  overriding the previous values
- Load the configuration from /etc/keylime/agent.conf, overriding
  previous values
- Load configuration snippets from /etc/keylime/agent.conf.d/*,
  overriding previous values
- Finally any configuration option can be overriden via environment
  variable, by using the 'KEYLIME_AGENT_' prefix. For example,
  KEYLIME_AGENT_PORT=9003 will override the 'port' option from the
  'agent' section with the value '9003'.

It is possible to override all file sources with a single file by setting the KEYLIME_AGENT_CONFIG environment variable. If it is set, the configuration files and snippets are ignored and all options are loaded from the file pointed by KEYLIME_AGENT_CONFIG. The environment variable overrides still apply, meaning it is still possible to override the options from the file pointed by KEYLIME_AGENT_CONFIG using the environment variables with the KEYLIME_AGENT_ prefix.

This was implemented using new dependency crates: config and glob.

Other changes:

Added support to using existing key (optionally protected by a password) and certificate
Add the EK public key hash to the stored TPM data (AK) to allow verifying if the TPM where the data is being loaded is the same that generated the data (this is to avoid errors as it is not possible to load the data generated by a TPM using another TPM)

Follow the naming specified in the configuration enhancement: https://github.com/keylime/enhancements/blob/master/72_config_and_simplify_tls.md Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Keep only the data path as part of the configuration structure. Then open and load the data when needed. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Separate the key and certificate from the Agent Data and store in separate files. Add the configuration options 'server_key' and 'server_cert' to allow the user to use existing key and certificate. If the files set to 'server_key' or 'server_cert' don't exist, new files are created to store the generated key or certificate. Otherwise the existing files will not be overwritten. This also unify the handling of file paths in the configuration file for the files stored in the 'keylime_dir' by introducing the config_get_file_path() function with the following behavior: - If the option is set as "default" or left empty, the default value is used - If the option is not an absolute path, it is treated as relative from the directory set in the 'keylime_dir' configuration option - If the option is set with an absolute path, it is used without change Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Include as part of the stored Agent Data the hash of the public EK of the TPM that generated the AK. Before loading the AK, check if the current TPM is the same TPM that generated the information to avoid loading errors. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

ueno · 2022-09-19T22:23:02Z

If a configuration file is found in /usr/etc/keylime-agent.conf, it
is loaded and any option set overrides the default values

Perhaps I should have mentioned it earlier but I wonder if /usr/etc is a standard place to install such configuration. FHS doesn't specify it and on my systems: /usr/etc is only owned by npm on Fedora, and it does not exist on Debian.

Add the configuration option server_key_password to allow setting a password to be used to encrypt the generated key when writing to a file and to decrypt the key when loading from a file. If the server_key_password is set as empty, the key is assumed to be unencrypted. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

ansasaki · 2022-09-19T22:51:02Z

If a configuration file is found in /usr/etc/keylime-agent.conf, it
is loaded and any option set overrides the default values

Perhaps I should have mentioned it earlier but I wonder if /usr/etc is a standard place to install such configuration. FHS doesn't specify it and on my systems: /usr/etc is only owned by npm on Fedora, and it does not exist on Debian.

I can drop this. I just added this path to make it similar to the python configuration.

ueno · 2022-09-19T23:12:41Z

If a configuration file is found in /usr/etc/keylime-agent.conf, it
is loaded and any option set overrides the default values

Perhaps I should have mentioned it earlier but I wonder if /usr/etc is a standard place to install such configuration. FHS doesn't specify it and on my systems: /usr/etc is only owned by npm on Fedora, and it does not exist on Debian.

I can drop this. I just added this path to make it similar to the python configuration.

Was there any discussion on this on the Python side? If not and it's not too late, maybe good to revisit; I would suggest /usr/share instead. See also: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_filesystem_layout

ansasaki · 2022-09-19T23:30:15Z

If a configuration file is found in /usr/etc/keylime-agent.conf, it
is loaded and any option set overrides the default values

Perhaps I should have mentioned it earlier but I wonder if /usr/etc is a standard place to install such configuration. FHS doesn't specify it and on my systems: /usr/etc is only owned by npm on Fedora, and it does not exist on Debian.

I can drop this. I just added this path to make it similar to the python configuration.

Was there any discussion on this on the Python side? If not and it's not too late, maybe good to revisit; I would suggest /usr/share instead. See also: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_filesystem_layout

I think the main supporter of using the /usr/etc/ was @aplanas who can give the reasoning for it.

aplanas · 2022-09-20T08:30:48Z

FHS doesn't specify it and on my systems

Depends on the source. There is a tendency of using it, that is also referenced here:

https://tldp.org/LDP/Linux-Filesystem-Hierarchy/html/usr.html

I think the main supporter of using the /usr/etc/ was @aplanas who can give the reasoning for it.

I hope that I am not alone here!

/usr/etc it is used to store sane defaults configuration for services. This is provided by the distributor and are not mean to be changed by the user nor the admin, for that it is /etc.

Ideally the service will know how to merge the snipped in /etc together with the version stored in /usr/etc, merging both versions (with priority on the one provided by the admin in /etc). The benefits are:

Once a package gets updated, the new /usr/etc will be updated too (no rpmsave, rpmnew). So if there is a new option or an old one with a safest value, it will be updated with the package
If the user removes the version in /etc, the service will take the default values

A longer explanation, with more examples can be found here: https://kubic.opensuse.org/blog/2019-12-05-usr-etc/

For doing the merge in services that do not support it by default, we are using libeconf, and doing upstream commits to support it.

ansasaki · 2022-09-20T09:32:37Z

/packit test

aplanas · 2022-09-20T09:31:02Z

src/config.rs

+pub static DEFAULT_KEYLIME_DIR: &str = "/var/lib/keylime";
+pub static DEFAULT_SERVER_KEY: &str = "server-private.pem";
+pub static DEFAULT_SERVER_CERT: &str = "server-cert.crt";
+pub static DEFAULT_SERVER_KEY_PASSWORD: &str = "";


Should be None ...?

aplanas · 2022-09-20T09:39:08Z

src/config.rs

+            enc_keyname: DEFAULT_ENC_KEYNAME.to_string(),
+            extract_payload_zip: DEFAULT_EXTRACT_PAYLOAD_ZIP,
+            server_key: Some("default".to_string()),
+            server_key_password: Some(


... because this put Some("") (there is a password, that is the empty string), that is different from None (no password was provided)

The empty string is considered "no password". Check:
https://github.com/ansasaki/rust-keylime/blob/036ab2a392022641a8603fddb405b58dd36430b8/src/crypto.rs#L77-L101
and
https://github.com/ansasaki/rust-keylime/blob/036ab2a392022641a8603fddb405b58dd36430b8/src/crypto.rs#L54-L72

ansasaki · 2022-09-20T10:04:19Z

/packit retest-failed

ansasaki · 2022-09-20T10:32:41Z

/packit test

This introduces several improvements to the agent configuration. - The file format is modified from INI style files to TOML formatted files. - The configuration is constructed using an hierarchical organization following the order: - The default hardcoded values are loaded - If a configuration file is found in /usr/etc/keylime-agent.conf, it is loaded and any option set overrides the default values - Load configuration snippets from /usr/etc/keylime-agent.conf.d/*, overriding the previous values - Load the configuration from /etc/keylime-agent.conf, overriding previous values - Load configuration snippets from /etc/keylime-agent.conf.d/*, overriding previous values - Finally any configuration option can be overriden via environment variable, by using the 'KEYLIME_AGENT_' prefix. For example, KEYLIME_AGENT_PORT=9003 will override the 'port' option from the 'agent' section with the value '9003'. This was introduced by using an added dependency, the 'config' crate using the 'toml' feature for the toml file format. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Add back the support for overriding the 'keylime_dir' option by setting the 'KEYLIME_DIR' environment variable. Note that it is also possible to override the 'keylime_dir' option by setting the 'KEYLIME_AGENT_KEYLIME_DIR' environment variable, although 'KEYLIME_DIR' has priority. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

This is not currently used, but will be important in future to check configuration file compatibility and upgrades. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

ansasaki · 2022-09-20T13:45:20Z

@lkatalin @ueno @aplanas I think the only pending decision here is about using /usr/etc or not.

Please let me know if there are other points to be modified.

Before merging, we need to drop the last commit which temporarily changes the tests repository.

ashcrow · 2022-09-20T14:05:42Z

I think the only pending decision here is about using /usr/etc or not.

Quick reminder that /usr is read-only on CoreOS.

lkatalin · 2022-09-20T14:28:52Z

I did a first pass and it seemed good to me, I can do another one shortly.

aplanas · 2022-09-20T14:31:25Z

the only pending decision here is about using /usr/etc or not

My vote is for /usr/etc. We can add /usr/lib (like systemd) or /usr/share as new search paths later if there are other standards.

Quick reminder that /usr is read-only on CoreOS.

Exactly. /usr/etc will be deployed with the image, never edited by the user (that should edit /etc)

The python components expect the configuration files to be in /etc/keylime/*.conf and the configuration snippets to be in /etc/keylime/*.conf.d This patch modifies the paths where the configuration files are searched to align with the python components. In the case, the agent configuration will be installed in /etc/keylime/agent.conf and the snippets are searched in /etc/keylime/agent.conf.d/ Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

ansasaki · 2022-09-20T19:30:08Z

/packit test

ansasaki · 2022-09-20T20:22:07Z

/packit test

ansasaki · 2022-09-20T21:43:56Z

/packit retest-failed

lkatalin

This LGTM and I appreciate that the config locations will match the Python version. Hopefully this will help with the CI.

ueno · 2022-09-21T01:37:24Z

@aplanas thanks for the detailed explanation!

A longer explanation, with more examples can be found here: https://kubic.opensuse.org/blog/2019-12-05-usr-etc/

For doing the merge in services that do not support it by default, we are using libeconf, and doing upstream commits to support it.

I wasn't aware of the UsrEtc effort. It makes sense from the package update point of view, though other distributions currently don't seem to catch up; as mentioned in the link to the Fedora packaging guideline:

Fedora follows the Filesystem Hierarchy Standard with regards to filesystem layout, with exceptions noted below. The FHS defines where files should be placed on the system.
[...]
Fedora does not allow new directories directly under / or /usr without FPC approval.

That said, I guess it shouldn't be a problem as long as the Fedora package doesn't ship any files under /usr/etc.

ansasaki · 2022-09-21T08:17:02Z

Fedora follows the Filesystem Hierarchy Standard with regards to filesystem layout, with exceptions noted below. The FHS defines where files should be placed on the system.
[...]
Fedora does not allow new directories directly under / or /usr without FPC approval.

That said, I guess it shouldn't be a problem as long as the Fedora package doesn't ship any files under /usr/etc.

I'll make sure the Fedora package doesn't ship any files there.

kkaarreell · 2022-09-21T09:26:42Z

Maybe the location could be configurable at build time, so that config templates could be shipped also to a different location (and used from there).

aplanas · 2022-09-21T09:27:54Z

Maybe the location could be configurable at build time

That is a good idea

ansasaki added 4 commits September 19, 2022 21:17

Rename the configuration options

cf8cd30

Follow the naming specified in the configuration enhancement: https://github.com/keylime/enhancements/blob/master/72_config_and_simplify_tls.md Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Remove the agent TPM data from the config struct

40aee81

Keep only the data path as part of the configuration structure. Then open and load the data when needed. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

keylime-bot requested review from puiterwijk, ueno and ashcrow September 19, 2022 19:47

keylime-bot assigned ueno, ashcrow and puiterwijk Sep 19, 2022

ansasaki force-pushed the toml branch from da8eca0 to 721a2fd Compare September 19, 2022 21:46

ansasaki force-pushed the toml branch from 721a2fd to b2dd6e8 Compare September 19, 2022 22:35

ansasaki force-pushed the toml branch from b2dd6e8 to 8968db9 Compare September 19, 2022 22:52

ansasaki force-pushed the toml branch from 8968db9 to 637b81f Compare September 19, 2022 23:26

ansasaki force-pushed the toml branch 2 times, most recently from fcbd834 to b544401 Compare September 19, 2022 23:41

aplanas reviewed Sep 20, 2022

View reviewed changes

ansasaki added 2 commits September 20, 2022 13:44

ansasaki force-pushed the toml branch from 37a1a41 to e1c3916 Compare September 20, 2022 11:45

ansasaki mentioned this pull request Sep 20, 2022

Adapt the tests for the new rust agent configuration format RedHat-SP-Security/keylime-tests#202

Merged

config: Add configuration file version

4071212

This is not currently used, but will be important in future to check configuration file compatibility and upgrades. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

ansasaki force-pushed the toml branch from e1c3916 to f691292 Compare September 20, 2022 12:40

ansasaki force-pushed the toml branch from 0afb2a8 to c827868 Compare September 20, 2022 18:17

lkatalin approved these changes Sep 21, 2022

View reviewed changes

ansasaki force-pushed the toml branch from c827868 to 97b0d26 Compare September 21, 2022 08:43

ansasaki merged commit 9dfb597 into keylime:master Sep 21, 2022

This was referenced Sep 21, 2022

keylime-agent.conf should default to run_as = keylime:tss #439

Closed

Test Rust agent with TOML-based config file #424

Closed

lkatalin mentioned this pull request Sep 21, 2022

Roadmap for official Rust agent keylime/keylime#986

Closed

21 tasks

ansasaki deleted the toml branch November 21, 2022 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve configuration and change format to TOML #449

Improve configuration and change format to TOML #449

ansasaki commented Sep 19, 2022 •

edited

Loading

ueno commented Sep 19, 2022

ansasaki commented Sep 19, 2022

ueno commented Sep 19, 2022 •

edited

Loading

ansasaki commented Sep 19, 2022 •

edited

Loading

aplanas commented Sep 20, 2022

ansasaki commented Sep 20, 2022

aplanas Sep 20, 2022

aplanas Sep 20, 2022

ansasaki Sep 20, 2022

ansasaki commented Sep 20, 2022

ansasaki commented Sep 20, 2022

ansasaki commented Sep 20, 2022

ashcrow commented Sep 20, 2022

lkatalin commented Sep 20, 2022

aplanas commented Sep 20, 2022

ansasaki commented Sep 20, 2022

ansasaki commented Sep 20, 2022

ansasaki commented Sep 20, 2022

lkatalin left a comment

ueno commented Sep 21, 2022

ansasaki commented Sep 21, 2022

kkaarreell commented Sep 21, 2022

aplanas commented Sep 21, 2022

Improve configuration and change format to TOML #449

Improve configuration and change format to TOML #449

Conversation

ansasaki commented Sep 19, 2022 • edited Loading

ueno commented Sep 19, 2022

ansasaki commented Sep 19, 2022

ueno commented Sep 19, 2022 • edited Loading

ansasaki commented Sep 19, 2022 • edited Loading

aplanas commented Sep 20, 2022

ansasaki commented Sep 20, 2022

aplanas Sep 20, 2022

Choose a reason for hiding this comment

aplanas Sep 20, 2022

Choose a reason for hiding this comment

ansasaki Sep 20, 2022

Choose a reason for hiding this comment

ansasaki commented Sep 20, 2022

ansasaki commented Sep 20, 2022

ansasaki commented Sep 20, 2022

ashcrow commented Sep 20, 2022

lkatalin commented Sep 20, 2022

aplanas commented Sep 20, 2022

ansasaki commented Sep 20, 2022

ansasaki commented Sep 20, 2022

ansasaki commented Sep 20, 2022

lkatalin left a comment

Choose a reason for hiding this comment

ueno commented Sep 21, 2022

ansasaki commented Sep 21, 2022

kkaarreell commented Sep 21, 2022

aplanas commented Sep 21, 2022

ansasaki commented Sep 19, 2022 •

edited

Loading

ueno commented Sep 19, 2022 •

edited

Loading

ansasaki commented Sep 19, 2022 •

edited

Loading