Hi!
Time for another iteration on the encrypted snapshots names, which is
mostly a rebase to the wip-fscrypt branch. To test this, I've used ceph
with the following PRs:
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
Changes since v3:
- Fixed WARN_ON() in ceph_encode_encrypted_dname()
- Updated documentation and copyright notice for the base64
encoding/decoding implementaiton which was taken from the fscrypt base.
Changes since v2:
- Use ceph_find_inode() instead of ceph_get_inode() for finding a snapshot
parent in function parse_longname(). I've also added a fallback to
ceph_get_inode() in case we fail to find the inode. This may happen if,
for example, the mount root doesn't include that inode. The iput() was
also complemented by a discard_new_inode() if the inode is in the I_NEW
state. (patch 0002)
- Move the check for '_' snapshots further up in the ceph_fname_to_usr()
and ceph_encode_encrypted_dname(). This fixes the case pointed out by
Xiubo in v2. (patch 0002)
- Use NAME_MAX for tmp arrays (patch 0002)
- Added an extra patch for replacing the base64url encoding by a different
encoding standard, the one used for IMAP mailboxes (which uses '+' and
',' instead of '-' and '_'). This should fix the issue with snapshot
names starting with '_'. (patch 0003)
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.
Luís Henriques (4):
ceph: add support for encrypted snapshot names
ceph: add support for handling encrypted snapshot names
ceph: update documentation regarding snapshot naming limitations
ceph: replace base64url by the encoding used for mailbox names
Documentation/filesystems/ceph.rst | 10 ++
fs/ceph/crypto.c | 252 +++++++++++++++++++++++++----
fs/ceph/crypto.h | 14 +-
fs/ceph/dir.c | 2 +-
fs/ceph/inode.c | 33 +++-
5 files changed, 273 insertions(+), 38 deletions(-)
On 4/14/22 9:51 PM, Luís Henriques wrote:
> Hi!
>
> Time for another iteration on the encrypted snapshots names, which is
> mostly a rebase to the wip-fscrypt branch. To test this, I've used ceph
> with the following PRs:
>
> 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
>
> Changes since v3:
>
> - Fixed WARN_ON() in ceph_encode_encrypted_dname()
>
> - Updated documentation and copyright notice for the base64
> encoding/decoding implementaiton which was taken from the fscrypt base.
>
> Changes since v2:
>
> - Use ceph_find_inode() instead of ceph_get_inode() for finding a snapshot
> parent in function parse_longname(). I've also added a fallback to
> ceph_get_inode() in case we fail to find the inode. This may happen if,
> for example, the mount root doesn't include that inode. The iput() was
> also complemented by a discard_new_inode() if the inode is in the I_NEW
> state. (patch 0002)
>
> - Move the check for '_' snapshots further up in the ceph_fname_to_usr()
> and ceph_encode_encrypted_dname(). This fixes the case pointed out by
> Xiubo in v2. (patch 0002)
>
> - Use NAME_MAX for tmp arrays (patch 0002)
>
> - Added an extra patch for replacing the base64url encoding by a different
> encoding standard, the one used for IMAP mailboxes (which uses '+' and
> ',' instead of '-' and '_'). This should fix the issue with snapshot
> names starting with '_'. (patch 0003)
>
> 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.
>
>
> Luís Henriques (4):
> ceph: add support for encrypted snapshot names
> ceph: add support for handling encrypted snapshot names
> ceph: update documentation regarding snapshot naming limitations
> ceph: replace base64url by the encoding used for mailbox names
>
> Documentation/filesystems/ceph.rst | 10 ++
> fs/ceph/crypto.c | 252 +++++++++++++++++++++++++----
> fs/ceph/crypto.h | 14 +-
> fs/ceph/dir.c | 2 +-
> fs/ceph/inode.c | 33 +++-
> 5 files changed, 273 insertions(+), 38 deletions(-)
>
This patch series LGTM. Thanks Luis !
Reviewed-by: Xiubo Li <[email protected]>
With snapshot names encryption we can not allow snapshots to be created in
locked directories because the names wouldn't be encrypted. This patch
forces the directory to be unlocked to allow a snapshot to be created.
Signed-off-by: Luís Henriques <[email protected]>
---
fs/ceph/dir.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index f48f1ff20927..93e2f08102a1 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1071,6 +1071,10 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
err = -EDQUOT;
goto out;
}
+ if ((op == CEPH_MDS_OP_MKSNAP) && !fscrypt_has_encryption_key(dir)) {
+ err = -ENOKEY;
+ goto out;
+ }
req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
On Mon, 2022-04-18 at 14:08 +0100, Lu?s Henriques wrote:
> With snapshot names encryption we can not allow snapshots to be created in
> locked directories because the names wouldn't be encrypted. This patch
> forces the directory to be unlocked to allow a snapshot to be created.
>
> Signed-off-by: Lu?s Henriques <[email protected]>
> ---
> fs/ceph/dir.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index f48f1ff20927..93e2f08102a1 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1071,6 +1071,10 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> err = -EDQUOT;
> goto out;
> }
> + if ((op == CEPH_MDS_OP_MKSNAP) && !fscrypt_has_encryption_key(dir)) {
> + err = -ENOKEY;
> + goto out;
> + }
>
>
> req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
Looks good. I'll pull this and the v4 series into the wip-fscrypt later
todat.
--
Jeff Layton <[email protected]>
On Mon, 2022-04-18 at 09:17 -0400, Jeff Layton wrote:
> On Mon, 2022-04-18 at 14:08 +0100, Lu?s Henriques wrote:
> > With snapshot names encryption we can not allow snapshots to be created in
> > locked directories because the names wouldn't be encrypted. This patch
> > forces the directory to be unlocked to allow a snapshot to be created.
> >
> > Signed-off-by: Lu?s Henriques <[email protected]>
> > ---
> > fs/ceph/dir.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index f48f1ff20927..93e2f08102a1 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -1071,6 +1071,10 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> > err = -EDQUOT;
> > goto out;
> > }
> > + if ((op == CEPH_MDS_OP_MKSNAP) && !fscrypt_has_encryption_key(dir)) {
> > + err = -ENOKEY;
> > + goto out;
> > + }
> >
> >
> > req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
>
> Looks good. I'll pull this and the v4 series into the wip-fscrypt later
> todat.
Actually, I take it back...
This check doesn't test whether the parent is encrypted. Doesn't it need
to do that too before checking for the key?
--
Jeff Layton <[email protected]>