Unencrypted children of encrypted datasets are dangerous

[FiloSottile]

In preparation for migrating to 0.8 and native encryption I read up about the feature, and was very reassured by reading that once I pick an encryption root, it was easy to tell what's encrypted and what not: everything under the root is encrypted. If I pick the pool root as encryption root, there won't be ever any doubt about encryption status!

Then I found #8870. That safety is being removed, and now I'm worried what will happen whenever I move or receive a dataset. The dynamics of sending and receiving properties are not simple, and I assume I'll make mistakes. If I end up creating or receiving an unencrypted dataset, I am not even sure how I'll ever recover, because even if I delete the dataset, the data will still be on the platters. I might have to stick to LUKS if I don't have the time to learn everything about properties in rename, recv and create.

I understand there were some convenience use cases, but removing these safeties defeats an important use case of disk encryption: making sure there is never anything unencrypted on the platter. (And even without encrypting the whole pool, "everything under tank/enc is encrypted" was such a valuable safety property.)

I know making safety arguments is not popular with users, as the ones that come to the issue tracker will feel (maybe correctly) that they understand the system, they would not make such a mistake, and you are just limiting their options. However, the pool of users is much much wider than that, and I think moving or receiving an unencrypted dataset in an encrypted pool will become a common fatal misconfiguration, reducing the value of a great security feature.

Safety is just as important as security. Please consider reverting the change.

[fmyhr]

How about using a tunable to control this behavior? (Then the argument will become what the default setting of the tunable should be...)

[ghfields]

This was discussed here: #8737

[FiloSottile]

A tunable where the default behavior is to disallow unencrypted children of encrypted datasets
would provide most of the safety to average users. It's just a matter of complexity, which I don't have an opinion on.

[rlaager]

A tunable would work, and this may be a legitimate case for one, since there are two diametrically opposed use cases. I'm certainly not against one. The key thing for me is that I want to be able to encrypt by default, but explicitly exclude particular datasets to avoid paying a performance penalty on the (by volume) majority of the data in the pool.

[maxximino]

Just introducing a tunable leaves a potential risk about the value of the tunable in the past: there is no way to audit that it has never been flipped to the "unsecure" state and there is no encrypted data on the disk. I am in favor of introducing the tunable with the safe default, but IMHO it should also write a "encryption_tainted" property on encrypted datasets (recursively up the path up to encryption root!) where and when it has been used(When effectively creating an unencrypted dataset, not just flipping the value) , to indicate that some unencrypted data is still potentially lurking in the pool. Removing the unencrypted dataset is not enough. Not even a trim is guaranteed to remove everything, see zfs_trim_extent_bytes_min parameter. (and potentially other issues in the disk format that I'm not aware of).

…

On Sun, Dec 29, 2019, 06:30 Richard Laager ***@***.***> wrote: A tunable would work, and this may be a legitimate case for one, since there are two diametrically opposed use cases. I'm certainly not against one. The key thing for me is that I want to be able to encrypt by default, but explicitly exclude particular datasets to avoid paying a performance penalty on the (by volume) majority of the data in the pool. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9116>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEKVAKMTHFZZWJ3O36SNSTQ3AYXTANCNFSM4II54TXA> .

[scineram]

Any dataset can be mounted anywhere so this is all bs.

[stale]

This issue has been automatically marked as "stale" because it has not had any activity for a while. It will be closed in 90 days if no further activity occurs. Thank you for your contributions.

[ghost]

I think allowing unencrypted child datasets of encrypted datasets is fine as long as encryption is enabled by default, that is, if rpool/sys is encrypted, a simple command zfs create rpool/sys/dataset should produce an encrypted dataset. For your information, this is currently the default behavior, as illustrated in various Root on ZFS guides.

Regarding the point of @scineram, it's not that the mountpoint that matters, it's that any child dataset can be moved out of the nested structure with zfs rename. With the above example, we can zfs rename rpool/sys/dataset rpool/newsys, thereby canceling the nested structure.

Summing up, we should keep the current defaults, they are

• create encrypted child datasets by default
• allow creating unencrypted child datasets when -o encryption=off is specified upon creation

[stale]

This issue has been automatically marked as "stale" because it has not had any activity for a while. It will be closed in 90 days if no further activity occurs. Thank you for your contributions.