Handle whitespace in keylime.conf

[greyspectrum]

Signed-off-by: greyspectrum [email protected]

Closes #408

[ansasaki]

/packit retest-failed

[ansasaki]

/packit test

[lkatalin]

This seems like a valid error and fix to me insofar as Keylime should not be trying to switch into a user:group that is left unspecified. In general it seems like whitespace in the config file should not be read in or treated as if it should be a legitimate value.

@ansasaki I think the error with testing here results from the expectation that setting payload_script to empty should be okay (here), but with this new change that counts as an error. Maybe the test could be updated? I don't know my way around the Packit tests very well so I have been trying to check whether this is the issue but I'm not certain. What do you think is best?

[ansasaki]

This seems like a valid error and fix to me insofar as Keylime should not be trying to switch into a user:group that is left unspecified. In general it seems like whitespace in the config file should not be read in or treated as if it should be a legitimate value.

I agree that there is an issue in run_as option handling of whitespace, but I disagree that whitespace is invalid for all options. The default configuration file contains whitespaces/empty value in many options, usually with the semantics that it should use the default value. With this change we are effectively changing the configuration file semantics, making required to have a non-whitespace value for all options.

@ansasaki I think the error with testing here results from the expectation that setting payload_script to empty should be okay (here), but with this new change that counts as an error. Maybe the test could be updated? I don't know my way around the Packit tests very well so I have been trying to check whether this is the issue but I'm not certain. What do you think is best?

The best for me would be to fix the handling of whitespace/empty value instead of making it an invalid value for all options. I would make the semantics that whitespace/empty value means to fallback to the default value for all options (but add a warning message that an empty/whitespace value was found)

[greyspectrum]

@lkatalin @ansasaki This should be in good shape now. Let me know if you'd like further changes.

[lkatalin]

LGTM

[lkatalin]

@ansasaki Is this along the lines of what you were thinking?

[ansasaki]

@ansasaki Is this along the lines of what you were thinking?

Couldn't we modify config_get() to use trim() to remove any bogus white spaces instead of explicitly checking if all chars in the string are white spaces? Then the existing check for empty string would've caught this. Moreover, I think cleaning the white spaces from the strings obtained from the configuration should be appropriate for all options.

[lkatalin]

Then the existing check for empty string would've caught this.

@ansasaki Which check is this? I've tried this change and trim() on its own does not fix the error because the config values are still being assigned this way (debug print statement inserted):

 INFO  keylime_agent::common > value for key keylime_ca is: default
 INFO  keylime_agent::common > value for key revocation_actions is: 
 INFO  keylime_agent::common > value for key run_as is: 

So the code will still call run_as on the whitespace (which seems like it should not happen) and give the unhelpful error below:

ERROR keylime_agent::permissions  > Invalid parameter format:  cannot be parsed as 'user:group'
ERROR keylime_agent::permissions  > Invalid parameter format:  cannot be parsed as 'user:group'

Instead the earlier change to check for whitespace before calling permissions::run_as() seemed to fix this.

[ansasaki]

Then the existing check for empty string would've caught this.

@ansasaki Which check is this?

I meant this check:

rust-keylime/src/common.rs

Lines 462 to 472 in 5b9072a

	let run_as = if permissions::get_euid() == 0 {
	match config_get(&conf_name, &conf, "cloud_agent", "run_as") {
	Ok(user_group) => Some(user_group),
	Err(_) => {
	warn!("Cannot drop privileges since 'run_as' is empty or missing in 'cloud_agent' section of keylime.conf.");
	None
	}
	}
	} else {
	None
	};

I misread the code and thought it would be caught here, but config_get() does not return Err() for empty values, only for missing options.

I've tried this change and trim() on its own does not fix the error because the config values are still being assigned this way (debug print statement inserted):

 INFO  keylime_agent::common > value for key keylime_ca is: default
 INFO  keylime_agent::common > value for key revocation_actions is: 
 INFO  keylime_agent::common > value for key run_as is: 

So the code will still call run_as on the whitespace (which seems like it should not happen) and give the unhelpful error below:

ERROR keylime_agent::permissions  > Invalid parameter format:  cannot be parsed as 'user:group'
ERROR keylime_agent::permissions  > Invalid parameter format:  cannot be parsed as 'user:group'

Instead the earlier change to check for whitespace before calling permissions::run_as() seemed to fix this.

Right, config_get() doesn't judge if empty is valid or not, so we need to fix both the handling of the empty string in the check I mentioned above and trim the string in config_get(). Probably the best is to check user_group.is_empty() before returning Some(user_group) in the Ok() case.

We should also fix the message in the Err() case as it is misleading: Err() is returned when the option is missing, but not when the option is empty or blank.

[lkatalin]

@ansasaki Thanks for checking, makes sense to me.
@greyspectrum Let us know if you have questions.

[greyspectrum]

@ansasaki This should look more like what you outlined in your last comment now.