Message-ID: <87sfr6nyj7.fsf@brahms.olymp>
Date:   Fri, 25 Mar 2022 09:59:08 +0000
From:   Luís Henriques <lhenriques@...e.de>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Jeff Layton <jlayton@...nel.org>, idryomov@...il.com,
        xiubli@...hat.com, ceph-devel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-fscrypt@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v11 02/51] fscrypt: export fscrypt_base64url_encode
 and fscrypt_base64url_decode

Eric Biggers <ebiggers@...nel.org> writes:

> On Wed, Mar 23, 2022 at 02:33:17PM +0000, Luís Henriques wrote:
>> Hi Eric,
>> 
>> Jeff Layton <jlayton@...nel.org> writes:
>> 
>> > Ceph is going to add fscrypt support, but we still want encrypted
>> > filenames to be composed of printable characters, so we can maintain
>> > compatibility with clients that don't support fscrypt.
>> >
>> > We could just adopt fscrypt's current nokey name format, but that is
>> > subject to change in the future, and it also contains dirhash fields
>> > that we don't need for cephfs. Because of this, we're going to concoct
>> > our own scheme for encoding encrypted filenames. It's very similar to
>> > fscrypt's current scheme, but doesn't bother with the dirhash fields.
>> >
>> > The ceph encoding scheme will use base64 encoding as well, and we also
>> > want it to avoid characters that are illegal in filenames. Export the
>> > fscrypt base64 encoding/decoding routines so we can use them in ceph's
>> > fscrypt implementation.
>> >
>> > Acked-by: Eric Biggers <ebiggers@...gle.com>
>> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
>> > ---
>> >  fs/crypto/fname.c       | 8 ++++----
>> >  include/linux/fscrypt.h | 5 +++++
>> >  2 files changed, 9 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
>> > index a9be4bc74a94..1e4233c95005 100644
>> > --- a/fs/crypto/fname.c
>> > +++ b/fs/crypto/fname.c
>> > @@ -182,8 +182,6 @@ static int fname_decrypt(const struct inode *inode,
>> >  static const char base64url_table[65] =
>> >  	"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
>> >  
>> > -#define FSCRYPT_BASE64URL_CHARS(nbytes)	DIV_ROUND_UP((nbytes) * 4, 3)
>> > -
>> >  /**
>> >   * fscrypt_base64url_encode() - base64url-encode some binary data
>> >   * @src: the binary data to encode
>> > @@ -198,7 +196,7 @@ static const char base64url_table[65] =
>> >   * Return: the length of the resulting base64url-encoded string in bytes.
>> >   *	   This will be equal to FSCRYPT_BASE64URL_CHARS(srclen).
>> >   */
>> > -static int fscrypt_base64url_encode(const u8 *src, int srclen, char *dst)
>> > +int fscrypt_base64url_encode(const u8 *src, int srclen, char *dst)
>> 
>> I know you've ACK'ed this patch already, but I was wondering if you'd be
>> open to change these encode/decode interfaces so that they could be used
>> for non-url base64 too.
>> 
>> My motivation is that ceph has this odd limitation where snapshot names
>> can not start with the '_' character.  And I've an RFC that adds snapshot
>> names encryption support which, unfortunately, can end up starting with
>> this char after base64 encoding.
>> 
>> So, my current proposal is to use a different encoding table.  I was
>> thinking about the IMAP mailboxes naming which uses '+' and ',' instead of
>> the '-' and '_', but any other charset would be OK (except those that
>> include '/' of course).  So, instead of adding yet another base64
>> implementation to the kernel, I was wondering if you'd be OK accepting a
>> patch to add an optional arg to these encoding/decoding functions to pass
>> an alternative table.  Or, if you'd prefer, keep the existing interface
>> but turning these functions into wrappers to more generic functions.
>> 
>> Obviously, Jeff, please feel free to comment too if you have any reserves
>> regarding this approach.
>> 
>> Cheers,
>> -- 
>> Luís
>> 
>
> Base64 encoding/decoding is trivial enough that I think you should just add your
> own functions to fs/ceph/ for now if you need yet another Base64 variant.  If we
> were to add general functions that allow "building your own" Base64 variant, I
> think they'd belong in lib/, not fs/crypto/.  (I objected to lib/ in the first
> version of Jeff's patchset because that patchset proposed adding just the old,
> idiosyncratic fscrypt Base64 variant to lib/ and just calling it "base64", which
> was misleading.  But, if there were to be properly documented functions to
> "build your own" Base64 variant, allowing control over both the character set
> and whether padding is done, lib/ would be the place...)

OK, that makes sense.  I agree that the right place for a generic
implementation would be somewhere out of the fs/crypto/ directory.  I
guess that, for now, I'll follow your advice and keep a local
implementation (in fact, the libceph *has* already an implementation!).

But adding a generic implementation and clean-up all the different
implementations in the kernel tree is probably a nice project.  For the
future.  Maybe.  *sigh*

Cheers,
-- 
Luís

Powered by blists - more mailing lists