Placeholders break encryption key inheritance of root_fs, unencrypted data is leaked

Hello, I have the following setup and use case:

Source: A server at a secure-ish location w/o encryption
Pull destination: A NAS at a shared location with native encryption enabled at pool/backup.
The source's tank/a and tank/a/b/c are replicated, but tank/a/b is not. Zrepl will create a placeholder for it.

Expected behavior: All files under pool/backup are encrypted and the received filesystems inherit pool/backup's keys.

Actual behavior: Zrepl explicitly disabled encryption for tank/a/b, causing unencrypted data to be written/leaked onto the destination's hardware. Eek!

Indeed it is impossible to have tank/a/b/c encrypted because the inheritance chain is broken by the placeholder. Setting encrypting properties explicitly can work, but is clunky at best. Setting encryption=aes-256-gcm in properties.inherit at least makes the whole thing fail instead of leaking unencrypted data.

Analysis and Source-Level Fix

I found that this behavior was intentionally introduced in commit 8ff83f2 to serve issue #342. IMO, that behavior is dangerous. Please never disable my encryption automatically :-(.

Removing lines 92-96 in zfs/placeholder.go resolves the issue: The placeholder is created with default properties, inherits its encryption, and the plain send stream is re-encrypted like expected.

Recommendation to fix:

Make placeholder properties configurable :-). Afterwards, drop the hardcoded and dangerous encryption=off completely.

If the user expects an encrypted send stream, then the user is free to set e.g. placeholders.properties = {encryption: off} in their job. This info would nicely fit into existing docs regarding encrypted sending. Otherwise, placeholders simply inherit their properties and are thus free of surprises.

Bonus: canmount and similar properties become configurable with placeholders for free.

Configs

Source:

  - name: foo
    type: source
    serve:
      type: tcp
      listen: 10.42.1.1:8888
      clients:
        10.42.1.32: example
    filesystems:
      "tank/a<": true
      "tank/a/b": false
      "tank/a/b/c<": true
    snapshotting:
      type: periodic
      interval: 15m
      prefix: zrepl_

Pull:

jobs:
  - name: bar
    type: pull
    root_fs: pool/backup
    recv:
      properties:
        override:
          canmount: noauto
          mountpoint: none
        inherit:
          - keylocation
          - keyformat
          - encryption
    connect:
      type: tcp
      address: 10.42.1.1:8888
    interval: 15m
    pruning:
      keep_sender:
        - type: not_replicated
        - type: last_n
          count: 10
        - type: grid
          grid: 1x1h(keep=all) | 24x1h | 30x1d
          regex: "zrepl_.*"
        - type: regex
          regex: "^manual_.*"
      keep_receiver:
        - type: grid
          grid: 1x1h(keep=all) | 24x1h | 30x1d | 3x30d
          regex: "zrepl_.*"

@InsanePrawn

Thanks for this bug report and apologies for not getting back to this sooner.
It is really unfortunate that we did not revisit the placeholder's encryption override when we introduced support for encrypted sends.
What we actually want, but didn't get, is an -o encryption=off but don't inherit that flag.
Which doesn't exist.

Your workaround of removing the hardcoded -o encryption=off for placeholders is viable.

And I like your subsequent proposal even better:

If the user expects an encrypted send stream, then the user is free to set e.g. placeholders.properties = {encryption: off} in their job. This info would nicely fit into existing docs regarding encrypted sending. Otherwise, placeholders simply inherit their properties and are thus free of surprises.

Bonus: canmount and similar properties become configurable with placeholders for free.

Good idea.

However: IIRC the problem with encrypted placeholders is that we can't zfs recv -F them in-place (it's a limitation of ZFS native encryption). The code that "handles" that case is this beauty here:

zrepl/endpoint/endpoint.go

Lines 790 to 874 in 009bd41

    
           var peek bytes.Buffer 
        
           var MaxPeek = envconst.Int64("ZREPL_ENDPOINT_RECV_PEEK_SIZE", 1<<20) 
        
           log.WithField("max_peek_bytes", MaxPeek).Info("peeking incoming stream") 
        
           if _, err := io.Copy(&peek, io.LimitReader(receive, MaxPeek)); err != nil { 
        
           	log.WithError(err).Error("cannot read peek-buffer from send stream") 
        
           } 
        
           var peekCopy bytes.Buffer 
        
           if n, err := peekCopy.Write(peek.Bytes()); err != nil || n != peek.Len() { 
        
           	panic(peek.Len()) 
        
           } 
        
           log.WithField("opts", fmt.Sprintf("%#v", recvOpts)).Debug("start receive command") 
        
           snapFullPath := to.FullPath(lp.ToString()) 
        
           if err := zfs.ZFSRecv(ctx, lp.ToString(), to, chainedio.NewChainedReader(&peek, receive), recvOpts); err != nil { 
        
           	// best-effort rollback of placeholder state if the recv didn't start 
        
           	_, resumableStatePresent := err.(*zfs.RecvFailedWithResumeTokenErr) 
        
           	disablePlaceholderRestoration := envconst.Bool("ZREPL_ENDPOINT_DISABLE_PLACEHOLDER_RESTORATION", false) 
        
           	placeholderRestored := !ph.IsPlaceholder 
        
           	if !disablePlaceholderRestoration && !resumableStatePresent && recvOpts.RollbackAndForceRecv && ph.FSExists && ph.IsPlaceholder && clearPlaceholderProperty { 
        
           		log.Info("restoring placeholder property") 
        
           		if phErr := zfs.ZFSSetPlaceholder(ctx, lp, true); phErr != nil { 
        
           			log.WithError(phErr).Error("cannot restore placeholder property after failed receive, subsequent replications will likely fail with a different error") 
        
           			// fallthrough 
        
           		} else { 
        
           			placeholderRestored = true 
        
           		} 
        
           		// fallthrough 
        
           	} 
        
           	// deal with failing initial encrypted send & recv 
        
           	if _, ok := err.(*zfs.RecvDestroyOrOverwriteEncryptedErr); ok && ph.IsPlaceholder && placeholderRestored { 
        
           		msg := `cannot automatically replace placeholder filesystem with incoming send stream - please see receive-side log for details` 
        
           		err := errors.New(msg) 
        
           		log.Error(msg) 
        
           		log.Error(`zrepl creates placeholder filesystems on the receiving side of a replication to match the sending side's dataset hierarchy`) 
        
           		log.Error(`zrepl uses zfs receive -F to replace those placeholders with incoming full sends`) 
        
           		log.Error(`OpenZFS native encryption prohibits zfs receive -F for encrypted filesystems`) 
        
           		log.Error(`the current zrepl placeholder filesystem concept is thus incompatible with OpenZFS native encryption`) 
        
           		tempStartFullRecvFS := lp.Copy().ToString() + ".zrepl.initial-recv" 
        
           		tempStartFullRecvFSDP, dpErr := zfs.NewDatasetPath(tempStartFullRecvFS) 
        
           		if dpErr != nil { 
        
           			log.WithError(dpErr).Error("cannot determine temporary filesystem name for initial encrypted recv workaround") 
        
           			return nil, err // yes, err, not dpErr 
        
           		} 
        
           		log := log.WithField("temp_recv_fs", tempStartFullRecvFS) 
        
           		log.Error(`as a workaround, zrepl will now attempt to re-receive the beginning of the stream into a temporary filesystem temp_recv_fs`) 
        
           		log.Error(`if that step succeeds: shut down zrepl and use 'zfs rename' to swap temp_recv_fs with local_fs, then restart zrepl`) 
        
           		log.Error(`replication will then resume using resumable send+recv`) 
        
           		tempPH, phErr := zfs.ZFSGetFilesystemPlaceholderState(ctx, tempStartFullRecvFSDP) 
        
           		if phErr != nil { 
        
           			log.WithError(phErr).Error("cannot determine placeholder state of temp_recv_fs") 
        
           			return nil, err // yes, err, not dpErr 
        
           		} 
        
           		if tempPH.FSExists { 
        
           			log.Error("temp_recv_fs already exists, assuming a (partial) initial recv to that filesystem has already been done") 
        
           			return nil, err 
        
           		} 
        
           		recvOpts.RollbackAndForceRecv = false 
        
           		recvOpts.SavePartialRecvState = true 
        
           		rerecvErr := zfs.ZFSRecv(ctx, tempStartFullRecvFS, to, chainedio.NewChainedReader(&peekCopy), recvOpts) 
        
           		if _, isResumable := rerecvErr.(*zfs.RecvFailedWithResumeTokenErr); rerecvErr == nil || isResumable { 
        
           			log.Error("completed re-receive into temporary filesystem temp_recv_fs, now shut down zrepl and use zfs rename to swap temp_recv_fs with local_fs") 
        
           		} else { 
        
           			log.WithError(rerecvErr).Error("failed to receive the beginning of the stream into temporary filesystem temp_recv_fs") 
        
           			log.Error("we advise you to collect the error log and current configuration, open an issue on GitHub, and revert to your previous configuration in the meantime") 
        
           		} 
        
           		log.Error(`if you would like to see improvements to this situation, please open an issue on GitHub`) 
        
           		return nil, err 
        
           	} 
        
           	log. 
        
           		WithError(err). 
        
           		WithField("opts", fmt.Sprintf("%#v", recvOpts)). 
        
           		Error("zfs receive failed") 
        
           	return nil, err 
        
           }

If, after an successful initial replication, a user replaces a placeholder with the real dataset in the sender's filesystems filter, then they'll have to juggle around datasets themselves, as described in the error messages.
I guess that's a trade-off that we'll just have to live with. (cc @InsanePrawn @Ranlvor )

WIP: fix encrypt-on-recv

Appreciate the response. This looks like an unfortunate limitation of OpenZFS indeed, I was not aware of it.

Do you happen to have an OpenZFS ticket to link to this one and explain our use case? I cannot find a mention of this limitation in man zfs-recv, nor do I find open bugs under the terms recv F. Chances are that this is not intended/documented behavior.

In the commit linked above I played around with an alternative approach to handling the situation implicitly on the receiver.
The advantage is that it doesn't leak the concept of placeholders to the user.
The disadvantage is that we cannot cleanly disambiguate the following two use cases:

encrypted-send-to-untrusted-receiver The sender sends encrypted datasets (send.encrypted = true) and the receiver doesn't know / load the key for the received datasets.
unencrypted-send-encrypting-receiver The sender sends unencrypted streams but the receiver inherits encryption etc. from root_fs or a parent thereof. (That's the use case that prompted this issue).

This could be addressed by requiring the user to indicate their intention explicitly in the recv-side config, e.g., through an option recv.encryped = false | true | re-rencrypt-from-root_fs.
If recv.encrypted=re-rencrypt-from-root_fs, zrepl would then make sure that only unencrypted sends are accepted, that root_fs's keystatus=available, and placeholders created without -o encryption=off.

Does that sound better or worse than leaking the concept of placeholders to the user?

NB: the receiver will know whether the send is encrypted or not based on Tbd metadata that the sender adds to the RecvRequest proto message . The receiver can trust the sender on this because, if the sender lies about the encryptedness of the send stream, it only hurts them, not the receiver.

If recv.encrypted=re-rencrypt-from-root_fs, zrepl would then make sure that only unencrypted sends are accepted, that root_fs's keystatus=available, and placeholders created without -o encryption=off.
Does that sound better or worse than leaking the concept of placeholders to the user?

I like explicit better than implicit, so indicating whether I expect an unencrypted stream and want it re-encrypted sounds reasonable to me. This is better indeed than leaving configuration of what in the end is an implementation detail to the user, like I would originally have suggested.

Then the last open question is: how do we deal with existing configs & recv-side ZFS state under root_fs? What is the default for recv.encrypted?
Also, ideally we want to automatically notify existing users that are affected by this issue once they upgrade (while not breaking their setup(?)).

Apologies for letting this sit unanswered.

I am afraid I have no user-friendly suggestion for your question. It /may/ be reasonable here to break things and have the user decide how to resume operation when a placeholder with encryption=off exists under an encrypted root_fs from a previous zrepl version.

The user's options are to either purge the affected fs and re-sync with recv.encrypted=re-rencrypt-from-root_fs, or explicitly set recv.encrypted=off and thus keep the previous version's behavior.

None of the options recv.encryped = false | true | re-rencrypt-from-root_fs discussed so far seem to work as default. The sender by default sends an unencrypted stream, so re-rencrypt-from-root_fs would be closest to what I would pick as default, except that it breaks existing workflows due to the keystatus=available constraint.

Let's reevaluate possible values of recv.encrypted; false, true, and re-encrypt IMO leave too much room for interpretation and do not work as default. Hypothetically speaking, do I set it to true when receiving an encrypted stream, or when I want the received stream stored in encrypted state? What is the difference between true and re-encrypt?

How about inherit-from-root_fs instead of re-rencrypt-from-root_fs, dropping the keystatus=available constraint? Afterwards, my understanding of your option choices is that we could replace true by raw (= I expect to receive a raw encrypted stream), and rename false to something more explicit that indicates that encrypt will only be disabled at and below placeholders.

@infinisil

Hm, how about

unencrypted
- fails if send stream is encrypted
- placeholders: with -o encryption=off
- handling of existing state: fails if recv-side fs exists and is encrypted
sender_encrypted
- fails if the send stream is unencrypted
- placeholders: with -o encryption=off
- handling of existing state: fails if recv-side fs exists and is unencrypted
encrypt_with_rootfs_encryption_root
- fails if the send stream is encrypted
- placeholders: no -o
- handling of existing state:
  - if recv-side fs exists: fails if the recv-side fs's encryptionroot is not root_fs or one of root_fs's parents
  - if recv-side fs does not exists: fails if recv-side fs's immediate parent's encryptionroot is not root_fs or one of root_fs's parents
    - the immediate parent is certain to exist because we serialize initial recvs / placeholder creation along the recv-side dataset hierarchy

People who use send.raw=true with a mixed set of encrypted and unencrypted filesystems will observe "undefined behavior" / "strange errors" for the time being. (cc @infinisil is that a problem for your use case? I don't know other people who use send.raw=true ).

Sounds rather complicated to me. Not sure if I understood this correctly, but would we not achieve the same functionality by just mirroring the raw property of the sender? After all, that is all that seems to be relevant for this issue, everything else is encryption sanity checking that I'd rather move into a separate property.

recv.raw = true | false (default: false)

true: placeholders with -o encryption=off
false: placeholder properties are inherited, fails if send stream is encrypted/raw

Additionally, an optional property recv.encrypted = true | false (default: false) could contain the encryption root validation described by you above. If enabled, zrepl would refuse to store an unencrypted recv stream on an unencrypted device (w/o re-encryption).

Notification mechanism about this issue: Fail if placeholders with encryption=off exist but raw = false (default).

I like the idea of untangling the two aspects of placeholders and encryption root validation.
But I think recv.raw overloads the raw term - it's easy to confuse with send.raw.
Which are not the same thing according to the behavior that you specified above.

I think that, in essence, recv.raw is only slightly more abstract than recv.placeholders.encryption = off | inherit. So maybe we should leak the concept to the user after all?

But I think recv.raw overloads the raw term - it's easy to confuse with send.raw.
Which are not the same thing according to the behavior that you specified above.

That was pretty much exactly my point -- set them to the same thing on both sides and zrepl does the right thing automatically. It is an abstraction after all.

Either way we have come full circle :). Whether it's a recv.placeholders.encryption = off | inherit option or abstraction/magic under recv.raw seems to be a cosmetic and style choice at this point. The values mostly map 1:1 to each other.

I like explicit better than implicit, so if I had to pick I would go for an explicit recv.placeholders.encryption and explain the reason for requiring configuration here to the user in zrepl's docs. Zrepl can also notify affected users by erroring out if it observes a placeholder with encryption=off despite recv.placeholders.encryption being undefined.

@InsanePrawn

I like explicit better than implicit, so if I had to pick I would go for an explicit recv.placeholders.encryption and explain the reason for requiring configuration here to the user in zrepl's docs. Zrepl can also notify affected users by erroring out if it observes a placeholder with encryption=off despite recv.placeholders.encryption being undefined.

Ok, I'm convinced. @InsanePrawn thoughts?

@InsanePrawn

I like explicit better than implicit, so if I had to pick I would go for an explicit recv.placeholders.encryption and explain the reason for requiring configuration here to the user in zrepl's docs. Zrepl can also notify affected users by erroring out if it observes a placeholder with encryption=off despite recv.placeholders.encryption being undefined.

Ok, I'm convinced. @InsanePrawn thoughts?

Sounds reasonable.
The implicit default value of recv.placeholders.encryption would be inherit(ing the encryption root from its recv-side parent) to match send.raw = off, right?

fix encrypt-on-receive with placeholders

squash into "fix encrypt-on-receive with placeholders"

@mologie

problame/develop as of 79af285 contains the change that makes initial replications work without specifying recv.placeholder.encryption if they don't have any gaps on the sending side filter.

@mologie @razielgn are you interested in testing this? I have added a bunch of platformtests, that should keep surprises at bay. I think it's safe to upgrade the daemon, and if things break, just switch back the binary.

@problame

@problame Sure! I should be able to give it a try this weekend. Thanks for the effort!

@problame

@problame Ok I tested the commit with this config:

---
global:
  logging:
    - type: stdout
      level: debug
      format: human
jobs:
- name: test-sink
  root_fs: test-sink/zrepl-encrypted-sink
  serve:
    listener_name: test-sink
    type: local
  recv:
    placeholder:
      encryption: inherit # Without this I was getting an error
  type: sink
- connect:
    client_identity: nas
    dial_timeout: 2s
    listener_name: test-sink
    type: local
  filesystems:
    vault/backups/postgres: true
    vault/backups/gmail: true
  name: nas-test-backups
  pruning:
    keep_receiver:
    - grid: 6x30d
      regex: ^zrepl_test_
      type: grid
    keep_sender:
    - type: not_replicated
  snapshotting:
    interval: 168h # 7 days
    prefix: zrepl_test_
    type: periodic
  type: push

Vol test-sink/zrepl-encrypted-sink was created using zfs create -o encryption=on -o keyformat=passphrase -o keylocation=prompt test-sink/zrepl-encrypted-sink.
After zrepl has finished syncing, I could see that all created subvolumes reported encryption=on using zfs get -r encryption test-sink/zrepl-encrypted-sink

encryption: inherit # Without this I was getting an error

To be expected since nas-test-backups.filesystems has gaps.
Was the error message sufficiently helpful?

After zrepl has finished syncing, I could see that all created subvolumes reported encryption=on using zfs get -r encryption test-sink/zrepl-encrypted-sink

Great 👍

Yeah the error message immediately guided me into the right direction. Thanks for taking the time and implementing this patch!

Glad it's working. Hope to release a 0.5-rc1 soon.

fix encrypt-on-receive with placeholders

Navigation Menu

Repository navigation

Placeholders break encryption key inheritance of root_fs, unencrypted data is leaked #504

Analysis and Source-Level Fix

Configs

7 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	var peek bytes.Buffer
	var MaxPeek = envconst.Int64("ZREPL_ENDPOINT_RECV_PEEK_SIZE", 1<<20)
	log.WithField("max_peek_bytes", MaxPeek).Info("peeking incoming stream")
	if _, err := io.Copy(&peek, io.LimitReader(receive, MaxPeek)); err != nil {
	log.WithError(err).Error("cannot read peek-buffer from send stream")
	}
	var peekCopy bytes.Buffer
	if n, err := peekCopy.Write(peek.Bytes()); err != nil \|\| n != peek.Len() {
	panic(peek.Len())
	}

	log.WithField("opts", fmt.Sprintf("%#v", recvOpts)).Debug("start receive command")

	snapFullPath := to.FullPath(lp.ToString())
	if err := zfs.ZFSRecv(ctx, lp.ToString(), to, chainedio.NewChainedReader(&peek, receive), recvOpts); err != nil {

	// best-effort rollback of placeholder state if the recv didn't start
	_, resumableStatePresent := err.(*zfs.RecvFailedWithResumeTokenErr)
	disablePlaceholderRestoration := envconst.Bool("ZREPL_ENDPOINT_DISABLE_PLACEHOLDER_RESTORATION", false)
	placeholderRestored := !ph.IsPlaceholder
	if !disablePlaceholderRestoration && !resumableStatePresent && recvOpts.RollbackAndForceRecv && ph.FSExists && ph.IsPlaceholder && clearPlaceholderProperty {
	log.Info("restoring placeholder property")
	if phErr := zfs.ZFSSetPlaceholder(ctx, lp, true); phErr != nil {
	log.WithError(phErr).Error("cannot restore placeholder property after failed receive, subsequent replications will likely fail with a different error")
	// fallthrough
	} else {
	placeholderRestored = true
	}
	// fallthrough
	}

	// deal with failing initial encrypted send & recv
	if _, ok := err.(*zfs.RecvDestroyOrOverwriteEncryptedErr); ok && ph.IsPlaceholder && placeholderRestored {
	msg := `cannot automatically replace placeholder filesystem with incoming send stream - please see receive-side log for details`
	err := errors.New(msg)
	log.Error(msg)

	log.Error(`zrepl creates placeholder filesystems on the receiving side of a replication to match the sending side's dataset hierarchy`)
	log.Error(`zrepl uses zfs receive -F to replace those placeholders with incoming full sends`)
	log.Error(`OpenZFS native encryption prohibits zfs receive -F for encrypted filesystems`)
	log.Error(`the current zrepl placeholder filesystem concept is thus incompatible with OpenZFS native encryption`)

	tempStartFullRecvFS := lp.Copy().ToString() + ".zrepl.initial-recv"
	tempStartFullRecvFSDP, dpErr := zfs.NewDatasetPath(tempStartFullRecvFS)
	if dpErr != nil {
	log.WithError(dpErr).Error("cannot determine temporary filesystem name for initial encrypted recv workaround")
	return nil, err // yes, err, not dpErr
	}

	log := log.WithField("temp_recv_fs", tempStartFullRecvFS)
	log.Error(`as a workaround, zrepl will now attempt to re-receive the beginning of the stream into a temporary filesystem temp_recv_fs`)
	log.Error(`if that step succeeds: shut down zrepl and use 'zfs rename' to swap temp_recv_fs with local_fs, then restart zrepl`)
	log.Error(`replication will then resume using resumable send+recv`)

	tempPH, phErr := zfs.ZFSGetFilesystemPlaceholderState(ctx, tempStartFullRecvFSDP)
	if phErr != nil {
	log.WithError(phErr).Error("cannot determine placeholder state of temp_recv_fs")
	return nil, err // yes, err, not dpErr
	}
	if tempPH.FSExists {
	log.Error("temp_recv_fs already exists, assuming a (partial) initial recv to that filesystem has already been done")
	return nil, err
	}

	recvOpts.RollbackAndForceRecv = false
	recvOpts.SavePartialRecvState = true
	rerecvErr := zfs.ZFSRecv(ctx, tempStartFullRecvFS, to, chainedio.NewChainedReader(&peekCopy), recvOpts)
	if _, isResumable := rerecvErr.(*zfs.RecvFailedWithResumeTokenErr); rerecvErr == nil \|\| isResumable {
	log.Error("completed re-receive into temporary filesystem temp_recv_fs, now shut down zrepl and use zfs rename to swap temp_recv_fs with local_fs")
	} else {
	log.WithError(rerecvErr).Error("failed to receive the beginning of the stream into temporary filesystem temp_recv_fs")
	log.Error("we advise you to collect the error log and current configuration, open an issue on GitHub, and revert to your previous configuration in the meantime")
	}

	log.Error(`if you would like to see improvements to this situation, please open an issue on GitHub`)
	return nil, err
	}

	log.
	WithError(err).
	WithField("opts", fmt.Sprintf("%#v", recvOpts)).
	Error("zfs receive failed")

	return nil, err
	}

Placeholders break encryption key inheritance of root_fs, unencrypted data is leaked #504

Description

Analysis and Source-Level Fix

Configs

Activity

problame commented on Aug 6, 2021

mologie commented on Aug 9, 2021

problame commented on Aug 12, 2021

mologie commented on Aug 13, 2021

problame commented on Aug 13, 2021

mologie commented on Aug 22, 2021

problame commented on Aug 23, 2021

mologie commented on Aug 23, 2021

problame commented on Aug 23, 2021

mologie commented on Aug 30, 2021

problame commented on Sep 25, 2021

InsanePrawn commented on Sep 29, 2021

7 remaining items

problame commented on Dec 9, 2021

razielgn commented on Dec 9, 2021

razielgn commented on Dec 12, 2021

problame commented on Dec 12, 2021

razielgn commented on Dec 12, 2021

problame commented on Dec 12, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions