Move `fscrypt.conf`'s format to TOML about fscrypt HOT 20 CLOSED

@fristonio Sure. I have extremely busy since long time so couldn't work on it.

from fscrypt.

@fristonio My initial thought was to have an intermediate period where we still read (but do not write) fscrypt.conf files as JSON. That way existing users are not immediatly broken. We would log a warning when reading as JSON, encouraging users to rerun sudo fscrypt setup to regenerate the config file as TOML. Before 1.0, we would then remove JSON related reading completely.

As the filename will not change (/etc/fscrypt.conf), I think we should keep our detection story simple. Just try to read the file as TOML, if it fails, try again using the existing JSON reading code.

from fscrypt.

@fristonio Some issues I'd also like to see fixed when we move to TOML:

• The compatiblity parameter currently doesn't really do anything, and should probobly be removed in the new format. Haveing a comma seperated list of parameters (instead of seperate options for each) is not a good idea. I'll submit a patch to remove any functionality related to this "legacy" parameter.
• We need to make sure we will always be able to read old versions of the config file with new versions of fscrypt. This means that (like proto3), missing options should take on a sane default value.
• We should probobly add a comment explaining the file at the top.
• We should make sure the contents and filenames options display as strings (like AES_256_XTS) as opposed to numbers
• We should add a version parameter to account for the eventual introduction of v2 fscrypt kernel policies. This would probobly be a later patch as it isn't tied to a format change, but something for us to be thinking about.

from fscrypt.

@josephlr Can I work on this issue ?

from fscrypt.

@vbmade2000 Of course! The necessary steps are laid out above, but if you have any specific questions you can ask them here or shoot me an email ([email protected]).

from fscrypt.

@josephlr Cool.

from fscrypt.

@josephlr I would suggest https://github.com/pelletier/go-toml as it looks stable and active. Also it is used in dep utility. Let me know if you have anything better.

from fscrypt.

@vbmade2000 Either https://github.com/pelletier/go-toml or https://github.com/BurntSushi/toml is a fine choice. Looking at the two it does seem like go-toml supports a more recent version of the TOML specification and is more active.

from fscrypt.

For go-toml, I would recommend using the Marshal/Unmarshal functionality, rather than the tree traversal functionality.

from fscrypt.

@josephlr Sure. I'll check go-toml then.

from fscrypt.

@vbmade2000 are you still working on this?
@josephlr If he is not, can I take this up?

from fscrypt.

@josephlr I think reading and creating toml formatted config file is quite straight forward using go-toml. But how are we planning to provide check for JSON formatted config file? Either we can completely remove support for JSON or we can use version(Set by Makefile) to either use JSON or TOML formatted conf file(in this case we will have multiple dependencies for config file parsing).

from fscrypt.

Just try to read the file as TOML, if it fails, try again using the existing JSON reading code.

This looks good enough to me. I will submit a PR for shifting the config file to TOML format.

The compatiblity parameter currently doesn't really do anything, and should probobly be removed in the new format. Haveing a comma seperated list of parameters (instead of seperate options for each) is not a good idea. I'll submit a patch to remove any functionality related to this "legacy" parameter.

Should I remove the compatibility parameter altogether or are you going to fill in patch for this?

We need to make sure we will always be able to read old versions of the config file with new versions of fscrypt. This means that (like proto3), missing options should take on a sane default value.

Since the config structure is auto generated by protobuf(https://github.com/google/fscrypt/blob/master/metadata/metadata.pb.go) and has protobuf tags, I think in order to support sane default values with TOML we will need to use golang/protobuf for parsing those tags, or is there a better way to do this?

We should probobly add a comment explaining the file at the top.

This is with reference to fscrypt.conf file right?

We should make sure the contents and filenames options display as strings (like AES_256_XTS) as opposed to numbers.

This is the current behavior if I am not wrong. :/

We should add a version parameter to account for the eventual introduction of v2 fscrypt kernel policies. This would probobly be a later patch as it isn't tied to a format change, but something for us to be thinking about.

Sounds good :)

@josephlr

from fscrypt.

I have been looking at the conversion of config to TOML lately, but I think using JSON gives us a lot of flexibility when being used with the protobuf(which we are using for all our configuration) due to the presence of native support for json in golang/protobuf. If we want to move to TOML we should either write a similar wrapper for TOML in golang/protobuf or have custom tags in config structure which we will parse with go-toml. What is your opinion for this @josephlr

from fscrypt.

@fristonio Looking over this, I think we should try the following approch:

• Define the Config struct as a Go Struct instead of a Protobuf
• Put JSON and TOML tags on this structure
• Make sure we're backwards/forwards compatible regardless of format
• Remove the Config message from metadata.proto

Note that the above solution might be tricky, because the Config message has protobuf submessages. If it doesn't work, we'll either:

• Add a TOML wrapper to golang/protobuf
• Just keep the Config as JSON (And maybe look into other ways of adding comments)

from fscrypt.

Yeah, this makes sense. I will try with the above approach and will let you know my findings.

Config message has protobuf submessages

So we can write go structs for them too 😕

Also do I need to look elsewhere while removing Config message from metadata.proto. I mean any other place where it is used, I am not much familiar with the codebase as of now.

from fscrypt.

The main problem with writing go structs for SourceType, HashingCosts, and EncryptionOptions is that they are used elsewhere in fscrypt (as part of the binary metadata), so we would run into a "multiple points of maintinace" issue by duplicating them (one in .go one in .proto).

Could TOML still work without duplicating all the structs into Go?

Also do I need to look elsewhere while removing Config message from metadata.proto

Nope, it's main use is in metadata/config.go, everywhere else it's an opaque type I belive.

from fscrypt.

Actually, I was wrong about the use of the Config message in other parts of fscrypt. It's used here, here, and probobly also in other places. This isn't too bad, as moving it to a Go struct would be fine as long as we keep the field names the same.

from fscrypt.

Could TOML still work without duplicating all the structs into Go?

Yeah it will work with the existing protobuf messages struct but we will remain with the same problem of not being able to use custom tags while encoding or decoding say optional values for example.

This isn't too bad, as moving it to a Go struct would be fine as long as we keep the field names the same.

So what are you suggesting should we do, should we port all of the protobuf messages in Config to golang structs and change them in required places in the code? @josephlr

from fscrypt.

Closing this as many of the issues brought up in this issue would make migrating to TOML a good amount of work (especially if we want to avoid breaking back-compat). Looking over the reasoning with fresh eyes, the benefits don't seem worth it.

The main think I wanted was comment support, but there are ways to just make JSON support comments, so that's probably what we will do.

from fscrypt.