Hi!
A couple of changes since v1:
- Dropped the dentry->d_flags change in ceph_mkdir(). Thanks to Xiubo
suggestion, patch 0001 now skips calling ceph_fscrypt_prepare_context()
if we're handling a snapshot.
- Added error handling to ceph_get_snapdir() in patch 0001 (Jeff had
already pointed that out but I forgot to include that change in previous
revision).
- Rebased patch 0002 to the latest wip-fscrypt branch.
- Added some documentation regarding snapshots naming restrictions.
As before, in order to test this code the following PRs are required:
mds: add protection from clients without fscrypt support #45073
mds: use the whole string as the snapshot long name #45192
mds: support alternate names for snapshots #45224
mds: limit the snapshot names to 240 characters #45312
Luís Henriques (3):
ceph: add support for encrypted snapshot names
ceph: add support for handling encrypted snapshot names
ceph: update documentation regarding snapshot naming limitations
Documentation/filesystems/ceph.rst | 10 ++
fs/ceph/crypto.c | 158 +++++++++++++++++++++++++----
fs/ceph/crypto.h | 11 +-
fs/ceph/inode.c | 31 +++++-
4 files changed, 182 insertions(+), 28 deletions(-)
Hi Luis,
There has another issue you need to handle at the same time.
Currently only the empty directory could be enabled the file encryption,
such as for the following command:
$ fscrypt encrypt mydir/
But should we also make sure that the mydir/.snap/ is empty ?
Here the 'empty' is not totally empty, which allows it should allow long
snap names exist.
Make sense ?
- Xiubo
On 3/16/22 12:19 AM, Luís Henriques wrote:
> Hi!
>
> A couple of changes since v1:
>
> - Dropped the dentry->d_flags change in ceph_mkdir(). Thanks to Xiubo
> suggestion, patch 0001 now skips calling ceph_fscrypt_prepare_context()
> if we're handling a snapshot.
>
> - Added error handling to ceph_get_snapdir() in patch 0001 (Jeff had
> already pointed that out but I forgot to include that change in previous
> revision).
>
> - Rebased patch 0002 to the latest wip-fscrypt branch.
>
> - Added some documentation regarding snapshots naming restrictions.
>
> As before, in order to test this code the following PRs are required:
>
> mds: add protection from clients without fscrypt support #45073
> mds: use the whole string as the snapshot long name #45192
> mds: support alternate names for snapshots #45224
> mds: limit the snapshot names to 240 characters #45312
>
> Luís Henriques (3):
> ceph: add support for encrypted snapshot names
> ceph: add support for handling encrypted snapshot names
> ceph: update documentation regarding snapshot naming limitations
>
> Documentation/filesystems/ceph.rst | 10 ++
> fs/ceph/crypto.c | 158 +++++++++++++++++++++++++----
> fs/ceph/crypto.h | 11 +-
> fs/ceph/inode.c | 31 +++++-
> 4 files changed, 182 insertions(+), 28 deletions(-)
>
On 3/17/22 7:11 PM, Luís Henriques wrote:
> Xiubo Li <[email protected]> writes:
>
>> On 3/17/22 6:01 PM, Jeff Layton wrote:
>>> I'm not sure we want to worry about .snap directories here since they
>>> aren't "real". IIRC, snaps are inherited from parents too, so you could
>>> do something like
>>>
>>> mkdir dir1
>>> mkdir dir1/.snap/snap1
>>> mkdir dir1/dir2
>>> fscrypt encrypt dir1/dir2
>>>
>>> There should be nothing to prevent encrypting dir2, but I'm pretty sure
>>> dir2/.snap will not be empty at that point.
>> If we don't take care of this. Then we don't know which snapshots should do
>> encrypt/dencrypt and which shouldn't when building the path in lookup and when
>> reading the snapdir ?
> In my patchset (which I plan to send a new revision later today, I think I
> still need to rebase it) this is handled by using the *real* snapshot
> parent inode. If we're decrypting/encrypting a name for a snapshot that
> starts with a '_' character, we first find the parent inode for that
> snapshot and only do the operation if that parent is encrypted.
Yeah, this is correct. And in my previous patches it worked well.
>
> In the other email I suggested that we could prevent enabling encryption
> in a directory when there are snapshots above in the hierarchy.
I think this is incorrect. Or once there has a snapshot in the root
directory, then you couldn't enable encryption any more in any subdirs ...
> But now
> that I think more about it, it won't solve any problem because you could
> create those snapshots later and then you would still need to handle these
> (non-encrypted) "_name_xxxx" snapshots anyway.
You only need to take care of the *real* or local snapshots.
-- Xiubo
>
> Cheers,
Xiubo Li <[email protected]> writes:
> Hi Luis,
>
> There has another issue you need to handle at the same time.
>
> Currently only the empty directory could be enabled the file encryption, such as
> for the following command:
>
> $ fscrypt encrypt mydir/
>
> But should we also make sure that the mydir/.snap/ is empty ?
>
> Here the 'empty' is not totally empty, which allows it should allow long snap
> names exist.
>
> Make sense ?
Right, actually I had came across that question in the past but completely
forgot about it.
Right now we simply check the dir stats to ensure a directory is empty.
We could add an extra check in ceph_crypt_empty_dir() to ensure that there
are no snapshots _above_ that directory (i.e. that there are no
"mydir/.snap/_name_xxxxx").
Unfortunately, I don't know enough of snapshots implementation details to
understand if it's a problem to consider a directory as being empty (in
the fscrypt context) when there are these '_name_xxx' directories. My
feeling is that this is not a problem but I really don't know.
Do you (or anyone) have any ideas/suggestions?
Cheers,
--
Luís
>
> - Xiubo
>
>
> On 3/16/22 12:19 AM, Luís Henriques wrote:
>> Hi!
>>
>> A couple of changes since v1:
>>
>> - Dropped the dentry->d_flags change in ceph_mkdir(). Thanks to Xiubo
>> suggestion, patch 0001 now skips calling ceph_fscrypt_prepare_context()
>> if we're handling a snapshot.
>>
>> - Added error handling to ceph_get_snapdir() in patch 0001 (Jeff had
>> already pointed that out but I forgot to include that change in previous
>> revision).
>>
>> - Rebased patch 0002 to the latest wip-fscrypt branch.
>>
>> - Added some documentation regarding snapshots naming restrictions.
>>
>> As before, in order to test this code the following PRs are required:
>>
>> mds: add protection from clients without fscrypt support #45073
>> mds: use the whole string as the snapshot long name #45192
>> mds: support alternate names for snapshots #45224
>> mds: limit the snapshot names to 240 characters #45312
>>
>> Luís Henriques (3):
>> ceph: add support for encrypted snapshot names
>> ceph: add support for handling encrypted snapshot names
>> ceph: update documentation regarding snapshot naming limitations
>>
>> Documentation/filesystems/ceph.rst | 10 ++
>> fs/ceph/crypto.c | 158 +++++++++++++++++++++++++----
>> fs/ceph/crypto.h | 11 +-
>> fs/ceph/inode.c | 31 +++++-
>> 4 files changed, 182 insertions(+), 28 deletions(-)
>>
>
On 3/17/22 8:01 PM, Jeff Layton wrote:
> On Thu, 2022-03-17 at 11:11 +0000, Lu?s Henriques wrote:
>> Xiubo Li <[email protected]> writes:
>>
>>> On 3/17/22 6:01 PM, Jeff Layton wrote:
>>>> I'm not sure we want to worry about .snap directories here since they
>>>> aren't "real". IIRC, snaps are inherited from parents too, so you could
>>>> do something like
>>>>
>>>> mkdir dir1
>>>> mkdir dir1/.snap/snap1
>>>> mkdir dir1/dir2
>>>> fscrypt encrypt dir1/dir2
>>>>
>>>> There should be nothing to prevent encrypting dir2, but I'm pretty sure
>>>> dir2/.snap will not be empty at that point.
>>> If we don't take care of this. Then we don't know which snapshots should do
>>> encrypt/dencrypt and which shouldn't when building the path in lookup and when
>>> reading the snapdir ?
>> In my patchset (which I plan to send a new revision later today, I think I
>> still need to rebase it) this is handled by using the *real* snapshot
>> parent inode. If we're decrypting/encrypting a name for a snapshot that
>> starts with a '_' character, we first find the parent inode for that
>> snapshot and only do the operation if that parent is encrypted.
>>
>> In the other email I suggested that we could prevent enabling encryption
>> in a directory when there are snapshots above in the hierarchy. But now
>> that I think more about it, it won't solve any problem because you could
>> create those snapshots later and then you would still need to handle these
>> (non-encrypted) "_name_xxxx" snapshots anyway.
>>
> Yeah, that sounds about right.
>
> What happens if you don't have the snapshot parent's inode in cache?
> That can happen if you (e.g.) are running NFS over ceph, or if you get
> crafty with name_to_handle_at() and open_by_handle_at().
>
> Do we have to do a LOOKUPINO in that case or does the trace contain that
> info? If it doesn't then that could really suck in a big hierarchy if
> there are a lot of different snapshot parent inodes to hunt down.
>
> I think this is a case where the client just doesn't have complete
> control over the dentry name. It may be better to just not encrypt them
> if it's too ugly.
>
> Another idea might be to just use the same parent inode (maybe the
> root?) for all snapshot names. It's not as secure, but it's probably
> better than nothing.
Does it allow to have different keys for the subdirs in the hierarchy ?
From my test it doesn't.
If so we can always use the same oldest ancestor in the hierarchy, who
has encryption key, to encyrpt/decrypt all the .snap in the hierarchy,
then just need to lookup oldest ancestor inode only once.
-- Xiubo
I'm not sure we want to worry about .snap directories here since they
aren't "real". IIRC, snaps are inherited from parents too, so you could
do something like
mkdir dir1
mkdir dir1/.snap/snap1
mkdir dir1/dir2
fscrypt encrypt dir1/dir2
There should be nothing to prevent encrypting dir2, but I'm pretty sure
dir2/.snap will not be empty at that point.
-- Jeff
On Thu, 2022-03-17 at 13:27 +0800, Xiubo Li wrote:
> Hi Luis,
>
> There has another issue you need to handle at the same time.
>
> Currently only the empty directory could be enabled the file encryption,
> such as for the following command:
>
> $ fscrypt encrypt mydir/
>
> But should we also make sure that the mydir/.snap/ is empty ?
>
> Here the 'empty' is not totally empty, which allows it should allow long
> snap names exist.
>
> Make sense ?
>
> - Xiubo
>
>
> On 3/16/22 12:19 AM, Lu?s Henriques wrote:
> > Hi!
> >
> > A couple of changes since v1:
> >
> > - Dropped the dentry->d_flags change in ceph_mkdir(). Thanks to Xiubo
> > suggestion, patch 0001 now skips calling ceph_fscrypt_prepare_context()
> > if we're handling a snapshot.
> >
> > - Added error handling to ceph_get_snapdir() in patch 0001 (Jeff had
> > already pointed that out but I forgot to include that change in previous
> > revision).
> >
> > - Rebased patch 0002 to the latest wip-fscrypt branch.
> >
> > - Added some documentation regarding snapshots naming restrictions.
> >
> > As before, in order to test this code the following PRs are required:
> >
> > mds: add protection from clients without fscrypt support #45073
> > mds: use the whole string as the snapshot long name #45192
> > mds: support alternate names for snapshots #45224
> > mds: limit the snapshot names to 240 characters #45312
> >
> > Lu?s Henriques (3):
> > ceph: add support for encrypted snapshot names
> > ceph: add support for handling encrypted snapshot names
> > ceph: update documentation regarding snapshot naming limitations
> >
> > Documentation/filesystems/ceph.rst | 10 ++
> > fs/ceph/crypto.c | 158 +++++++++++++++++++++++++----
> > fs/ceph/crypto.h | 11 +-
> > fs/ceph/inode.c | 31 +++++-
> > 4 files changed, 182 insertions(+), 28 deletions(-)
> >
>
--
Jeff Layton <[email protected]>
On Thu, 2022-03-17 at 11:11 +0000, Lu?s Henriques wrote:
> Xiubo Li <[email protected]> writes:
>
> > On 3/17/22 6:01 PM, Jeff Layton wrote:
> > > I'm not sure we want to worry about .snap directories here since they
> > > aren't "real". IIRC, snaps are inherited from parents too, so you could
> > > do something like
> > >
> > > mkdir dir1
> > > mkdir dir1/.snap/snap1
> > > mkdir dir1/dir2
> > > fscrypt encrypt dir1/dir2
> > >
> > > There should be nothing to prevent encrypting dir2, but I'm pretty sure
> > > dir2/.snap will not be empty at that point.
> >
> > If we don't take care of this. Then we don't know which snapshots should do
> > encrypt/dencrypt and which shouldn't when building the path in lookup and when
> > reading the snapdir ?
>
> In my patchset (which I plan to send a new revision later today, I think I
> still need to rebase it) this is handled by using the *real* snapshot
> parent inode. If we're decrypting/encrypting a name for a snapshot that
> starts with a '_' character, we first find the parent inode for that
> snapshot and only do the operation if that parent is encrypted.
>
> In the other email I suggested that we could prevent enabling encryption
> in a directory when there are snapshots above in the hierarchy. But now
> that I think more about it, it won't solve any problem because you could
> create those snapshots later and then you would still need to handle these
> (non-encrypted) "_name_xxxx" snapshots anyway.
>
Yeah, that sounds about right.
What happens if you don't have the snapshot parent's inode in cache?
That can happen if you (e.g.) are running NFS over ceph, or if you get
crafty with name_to_handle_at() and open_by_handle_at().
Do we have to do a LOOKUPINO in that case or does the trace contain that
info? If it doesn't then that could really suck in a big hierarchy if
there are a lot of different snapshot parent inodes to hunt down.
I think this is a case where the client just doesn't have complete
control over the dentry name. It may be better to just not encrypt them
if it's too ugly.
Another idea might be to just use the same parent inode (maybe the
root?) for all snapshot names. It's not as secure, but it's probably
better than nothing.
--
Jeff Layton <[email protected]>
On 3/17/22 6:14 PM, Luís Henriques wrote:
> Xiubo Li <[email protected]> writes:
>
>> Hi Luis,
>>
>> There has another issue you need to handle at the same time.
>>
>> Currently only the empty directory could be enabled the file encryption, such as
>> for the following command:
>>
>> $ fscrypt encrypt mydir/
>>
>> But should we also make sure that the mydir/.snap/ is empty ?
>>
>> Here the 'empty' is not totally empty, which allows it should allow long snap
>> names exist.
>>
>> Make sense ?
> Right, actually I had came across that question in the past but completely
> forgot about it.
>
> Right now we simply check the dir stats to ensure a directory is empty.
> We could add an extra check in ceph_crypt_empty_dir() to ensure that there
> are no snapshots _above_ that directory (i.e. that there are no
> "mydir/.snap/_name_xxxxx").
>
> Unfortunately, I don't know enough of snapshots implementation details to
> understand if it's a problem to consider a directory as being empty (in
> the fscrypt context) when there are these '_name_xxx' directories. My
> feeling is that this is not a problem but I really don't know.
>
> Do you (or anyone) have any ideas/suggestions?
There is no need to care about the long snap names in .snap, because
they are all from the parent snaprealms.
What you need to make sure is that there shouldn't have any local
snapshot before encrypting the directory.
If we don't make sure about this then when encrypting/decrypting the
snapshot names you will hit errors in theory.
But I didn't test this yet, you can try.
-- Xiubo
> Cheers,
On 3/17/22 6:14 PM, Luís Henriques wrote:
> Xiubo Li <[email protected]> writes:
>
>> Hi Luis,
>>
>> There has another issue you need to handle at the same time.
>>
>> Currently only the empty directory could be enabled the file encryption, such as
>> for the following command:
>>
>> $ fscrypt encrypt mydir/
>>
>> But should we also make sure that the mydir/.snap/ is empty ?
>>
>> Here the 'empty' is not totally empty, which allows it should allow long snap
>> names exist.
>>
>> Make sense ?
> Right, actually I had came across that question in the past but completely
> forgot about it.
>
> Right now we simply check the dir stats to ensure a directory is empty.
> We could add an extra check in ceph_crypt_empty_dir() to ensure that there
> are no snapshots _above_ that directory (i.e. that there are no
> "mydir/.snap/_name_xxxxx").
Check this only in ceph_crypt_empty_dir() seems not enough, at least not
graceful.
Please see
https://github.com/google/fscrypt/blob/master/cmd/fscrypt/commands.go#L305.
The google/fscrypt will read that directory to check where it's empty or
not. So eventually we may need to fix it there. But for a workaround you
may could fix it in ceph_crypt_empty_dir() and ceph_ioctl() when setting
the key or policy ?
-- Xiubo
>
> Unfortunately, I don't know enough of snapshots implementation details to
> understand if it's a problem to consider a directory as being empty (in
> the fscrypt context) when there are these '_name_xxx' directories. My
> feeling is that this is not a problem but I really don't know.
>
> Do you (or anyone) have any ideas/suggestions?
>
> Cheers,
On Thu, 2022-03-17 at 20:31 +0800, Xiubo Li wrote:
> On 3/17/22 8:01 PM, Jeff Layton wrote:
> > On Thu, 2022-03-17 at 11:11 +0000, Lu?s Henriques wrote:
> > > Xiubo Li <[email protected]> writes:
> > >
> > > > On 3/17/22 6:01 PM, Jeff Layton wrote:
> > > > > I'm not sure we want to worry about .snap directories here since they
> > > > > aren't "real". IIRC, snaps are inherited from parents too, so you could
> > > > > do something like
> > > > >
> > > > > mkdir dir1
> > > > > mkdir dir1/.snap/snap1
> > > > > mkdir dir1/dir2
> > > > > fscrypt encrypt dir1/dir2
> > > > >
> > > > > There should be nothing to prevent encrypting dir2, but I'm pretty sure
> > > > > dir2/.snap will not be empty at that point.
> > > > If we don't take care of this. Then we don't know which snapshots should do
> > > > encrypt/dencrypt and which shouldn't when building the path in lookup and when
> > > > reading the snapdir ?
> > > In my patchset (which I plan to send a new revision later today, I think I
> > > still need to rebase it) this is handled by using the *real* snapshot
> > > parent inode. If we're decrypting/encrypting a name for a snapshot that
> > > starts with a '_' character, we first find the parent inode for that
> > > snapshot and only do the operation if that parent is encrypted.
> > >
> > > In the other email I suggested that we could prevent enabling encryption
> > > in a directory when there are snapshots above in the hierarchy. But now
> > > that I think more about it, it won't solve any problem because you could
> > > create those snapshots later and then you would still need to handle these
> > > (non-encrypted) "_name_xxxx" snapshots anyway.
> > >
> > Yeah, that sounds about right.
> >
> > What happens if you don't have the snapshot parent's inode in cache?
> > That can happen if you (e.g.) are running NFS over ceph, or if you get
> > crafty with name_to_handle_at() and open_by_handle_at().
> >
> > Do we have to do a LOOKUPINO in that case or does the trace contain that
> > info? If it doesn't then that could really suck in a big hierarchy if
> > there are a lot of different snapshot parent inodes to hunt down.
> >
> > I think this is a case where the client just doesn't have complete
> > control over the dentry name. It may be better to just not encrypt them
> > if it's too ugly.
> >
> > Another idea might be to just use the same parent inode (maybe the
> > root?) for all snapshot names. It's not as secure, but it's probably
> > better than nothing.
>
> Does it allow to have different keys for the subdirs in the hierarchy ?
> From my test it doesn't.
>
No. Once you set a key on directory you can't set another on a subtree
of it.
> If so we can always use the same oldest ancestor in the hierarchy, who
> has encryption key, to encyrpt/decrypt all the .snap in the hierarchy,
> then just need to lookup oldest ancestor inode only once.
>
That's a possibility.
--
Jeff Layton <[email protected]>
On 3/17/22 6:01 PM, Jeff Layton wrote:
> I'm not sure we want to worry about .snap directories here since they
> aren't "real". IIRC, snaps are inherited from parents too, so you could
> do something like
>
> mkdir dir1
> mkdir dir1/.snap/snap1
> mkdir dir1/dir2
> fscrypt encrypt dir1/dir2
>
> There should be nothing to prevent encrypting dir2, but I'm pretty sure
> dir2/.snap will not be empty at that point.
If we don't take care of this. Then we don't know which snapshots should
do encrypt/dencrypt and which shouldn't when building the path in lookup
and when reading the snapdir ?
-- Xiubo
>
> -- Jeff
>
> On Thu, 2022-03-17 at 13:27 +0800, Xiubo Li wrote:
>> Hi Luis,
>>
>> There has another issue you need to handle at the same time.
>>
>> Currently only the empty directory could be enabled the file encryption,
>> such as for the following command:
>>
>> $ fscrypt encrypt mydir/
>>
>> But should we also make sure that the mydir/.snap/ is empty ?
>>
>> Here the 'empty' is not totally empty, which allows it should allow long
>> snap names exist.
>>
>> Make sense ?
>>
>> - Xiubo
>>
>>
>> On 3/16/22 12:19 AM, Lu?s Henriques wrote:
>>> Hi!
>>>
>>> A couple of changes since v1:
>>>
>>> - Dropped the dentry->d_flags change in ceph_mkdir(). Thanks to Xiubo
>>> suggestion, patch 0001 now skips calling ceph_fscrypt_prepare_context()
>>> if we're handling a snapshot.
>>>
>>> - Added error handling to ceph_get_snapdir() in patch 0001 (Jeff had
>>> already pointed that out but I forgot to include that change in previous
>>> revision).
>>>
>>> - Rebased patch 0002 to the latest wip-fscrypt branch.
>>>
>>> - Added some documentation regarding snapshots naming restrictions.
>>>
>>> As before, in order to test this code the following PRs are required:
>>>
>>> mds: add protection from clients without fscrypt support #45073
>>> mds: use the whole string as the snapshot long name #45192
>>> mds: support alternate names for snapshots #45224
>>> mds: limit the snapshot names to 240 characters #45312
>>>
>>> Lu?s Henriques (3):
>>> ceph: add support for encrypted snapshot names
>>> ceph: add support for handling encrypted snapshot names
>>> ceph: update documentation regarding snapshot naming limitations
>>>
>>> Documentation/filesystems/ceph.rst | 10 ++
>>> fs/ceph/crypto.c | 158 +++++++++++++++++++++++++----
>>> fs/ceph/crypto.h | 11 +-
>>> fs/ceph/inode.c | 31 +++++-
>>> 4 files changed, 182 insertions(+), 28 deletions(-)
>>>
Jeff Layton <[email protected]> writes:
> On Thu, 2022-03-17 at 11:11 +0000, Luís Henriques wrote:
>> Xiubo Li <[email protected]> writes:
>>
>> > On 3/17/22 6:01 PM, Jeff Layton wrote:
>> > > I'm not sure we want to worry about .snap directories here since they
>> > > aren't "real". IIRC, snaps are inherited from parents too, so you could
>> > > do something like
>> > >
>> > > mkdir dir1
>> > > mkdir dir1/.snap/snap1
>> > > mkdir dir1/dir2
>> > > fscrypt encrypt dir1/dir2
>> > >
>> > > There should be nothing to prevent encrypting dir2, but I'm pretty sure
>> > > dir2/.snap will not be empty at that point.
>> >
>> > If we don't take care of this. Then we don't know which snapshots should do
>> > encrypt/dencrypt and which shouldn't when building the path in lookup and when
>> > reading the snapdir ?
>>
>> In my patchset (which I plan to send a new revision later today, I think I
>> still need to rebase it) this is handled by using the *real* snapshot
>> parent inode. If we're decrypting/encrypting a name for a snapshot that
>> starts with a '_' character, we first find the parent inode for that
>> snapshot and only do the operation if that parent is encrypted.
>>
>> In the other email I suggested that we could prevent enabling encryption
>> in a directory when there are snapshots above in the hierarchy. But now
>> that I think more about it, it won't solve any problem because you could
>> create those snapshots later and then you would still need to handle these
>> (non-encrypted) "_name_xxxx" snapshots anyway.
>>
>
> Yeah, that sounds about right.
>
> What happens if you don't have the snapshot parent's inode in cache?
> That can happen if you (e.g.) are running NFS over ceph, or if you get
> crafty with name_to_handle_at() and open_by_handle_at().
>
> Do we have to do a LOOKUPINO in that case or does the trace contain that
> info? If it doesn't then that could really suck in a big hierarchy if
> there are a lot of different snapshot parent inodes to hunt down.
>
> I think this is a case where the client just doesn't have complete
> control over the dentry name. It may be better to just not encrypt them
> if it's too ugly.
I *think* this is covered by my last revision. I didn't really tested
NFS, but this was why the patches are using ceph_get_inode() and falling
back to ceph_find_inode(). I tested this by directly mounting an
encrypted directory that had snapshots from a realm that wasn't in the
mount root.
(Obviously, these snapshot names are *not* encrypted because they belong
to snapshots that are not encrypted either.)
Cheers,
--
Luís
> Another idea might be to just use the same parent inode (maybe the
> root?) for all snapshot names. It's not as secure, but it's probably
> better than nothing.
> --
> Jeff Layton <[email protected]>
On 3/17/22 8:41 PM, Jeff Layton wrote:
> On Thu, 2022-03-17 at 20:31 +0800, Xiubo Li wrote:
>> On 3/17/22 8:01 PM, Jeff Layton wrote:
>>> On Thu, 2022-03-17 at 11:11 +0000, Lu?s Henriques wrote:
>>>> Xiubo Li <[email protected]> writes:
>>>>
>>>>> On 3/17/22 6:01 PM, Jeff Layton wrote:
>>>>>> I'm not sure we want to worry about .snap directories here since they
>>>>>> aren't "real". IIRC, snaps are inherited from parents too, so you could
>>>>>> do something like
>>>>>>
>>>>>> mkdir dir1
>>>>>> mkdir dir1/.snap/snap1
>>>>>> mkdir dir1/dir2
>>>>>> fscrypt encrypt dir1/dir2
>>>>>>
>>>>>> There should be nothing to prevent encrypting dir2, but I'm pretty sure
>>>>>> dir2/.snap will not be empty at that point.
>>>>> If we don't take care of this. Then we don't know which snapshots should do
>>>>> encrypt/dencrypt and which shouldn't when building the path in lookup and when
>>>>> reading the snapdir ?
>>>> In my patchset (which I plan to send a new revision later today, I think I
>>>> still need to rebase it) this is handled by using the *real* snapshot
>>>> parent inode. If we're decrypting/encrypting a name for a snapshot that
>>>> starts with a '_' character, we first find the parent inode for that
>>>> snapshot and only do the operation if that parent is encrypted.
>>>>
>>>> In the other email I suggested that we could prevent enabling encryption
>>>> in a directory when there are snapshots above in the hierarchy. But now
>>>> that I think more about it, it won't solve any problem because you could
>>>> create those snapshots later and then you would still need to handle these
>>>> (non-encrypted) "_name_xxxx" snapshots anyway.
>>>>
>>> Yeah, that sounds about right.
>>>
>>> What happens if you don't have the snapshot parent's inode in cache?
>>> That can happen if you (e.g.) are running NFS over ceph, or if you get
>>> crafty with name_to_handle_at() and open_by_handle_at().
>>>
>>> Do we have to do a LOOKUPINO in that case or does the trace contain that
>>> info? If it doesn't then that could really suck in a big hierarchy if
>>> there are a lot of different snapshot parent inodes to hunt down.
>>>
>>> I think this is a case where the client just doesn't have complete
>>> control over the dentry name. It may be better to just not encrypt them
>>> if it's too ugly.
>>>
>>> Another idea might be to just use the same parent inode (maybe the
>>> root?) for all snapshot names. It's not as secure, but it's probably
>>> better than nothing.
>> Does it allow to have different keys for the subdirs in the hierarchy ?
>> From my test it doesn't.
>>
> No. Once you set a key on directory you can't set another on a subtree
> of it.
If so. Yeah, I think your approach mentioned above is the best.
>> If so we can always use the same oldest ancestor in the hierarchy, who
>> has encryption key, to encyrpt/decrypt all the .snap in the hierarchy,
>> then just need to lookup oldest ancestor inode only once.
>>
Just like this.
-- Xiubo
> That's a possibility.
Xiubo Li <[email protected]> writes:
> On 3/17/22 6:01 PM, Jeff Layton wrote:
>> I'm not sure we want to worry about .snap directories here since they
>> aren't "real". IIRC, snaps are inherited from parents too, so you could
>> do something like
>>
>> mkdir dir1
>> mkdir dir1/.snap/snap1
>> mkdir dir1/dir2
>> fscrypt encrypt dir1/dir2
>>
>> There should be nothing to prevent encrypting dir2, but I'm pretty sure
>> dir2/.snap will not be empty at that point.
>
> If we don't take care of this. Then we don't know which snapshots should do
> encrypt/dencrypt and which shouldn't when building the path in lookup and when
> reading the snapdir ?
In my patchset (which I plan to send a new revision later today, I think I
still need to rebase it) this is handled by using the *real* snapshot
parent inode. If we're decrypting/encrypting a name for a snapshot that
starts with a '_' character, we first find the parent inode for that
snapshot and only do the operation if that parent is encrypted.
In the other email I suggested that we could prevent enabling encryption
in a directory when there are snapshots above in the hierarchy. But now
that I think more about it, it won't solve any problem because you could
create those snapshots later and then you would still need to handle these
(non-encrypted) "_name_xxxx" snapshots anyway.
Cheers,
--
Luís
>
> -- Xiubo
>
>>
>> -- Jeff
>>
>> On Thu, 2022-03-17 at 13:27 +0800, Xiubo Li wrote:
>>> Hi Luis,
>>>
>>> There has another issue you need to handle at the same time.
>>>
>>> Currently only the empty directory could be enabled the file encryption,
>>> such as for the following command:
>>>
>>> $ fscrypt encrypt mydir/
>>>
>>> But should we also make sure that the mydir/.snap/ is empty ?
>>>
>>> Here the 'empty' is not totally empty, which allows it should allow long
>>> snap names exist.
>>>
>>> Make sense ?
>>>
>>> - Xiubo
>>>
>>>
>>> On 3/16/22 12:19 AM, Luís Henriques wrote:
>>>> Hi!
>>>>
>>>> A couple of changes since v1:
>>>>
>>>> - Dropped the dentry->d_flags change in ceph_mkdir(). Thanks to Xiubo
>>>> suggestion, patch 0001 now skips calling ceph_fscrypt_prepare_context()
>>>> if we're handling a snapshot.
>>>>
>>>> - Added error handling to ceph_get_snapdir() in patch 0001 (Jeff had
>>>> already pointed that out but I forgot to include that change in previous
>>>> revision).
>>>>
>>>> - Rebased patch 0002 to the latest wip-fscrypt branch.
>>>>
>>>> - Added some documentation regarding snapshots naming restrictions.
>>>>
>>>> As before, in order to test this code the following PRs are required:
>>>>
>>>> mds: add protection from clients without fscrypt support #45073
>>>> mds: use the whole string as the snapshot long name #45192
>>>> mds: support alternate names for snapshots #45224
>>>> mds: limit the snapshot names to 240 characters #45312
>>>>
>>>> Luís Henriques (3):
>>>> ceph: add support for encrypted snapshot names
>>>> ceph: add support for handling encrypted snapshot names
>>>> ceph: update documentation regarding snapshot naming limitations
>>>>
>>>> Documentation/filesystems/ceph.rst | 10 ++
>>>> fs/ceph/crypto.c | 158 +++++++++++++++++++++++++----
>>>> fs/ceph/crypto.h | 11 +-
>>>> fs/ceph/inode.c | 31 +++++-
>>>> 4 files changed, 182 insertions(+), 28 deletions(-)
>>>>
>