Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 24 Mar 2022 17:46:21 +0000
From:   Eric Biggers <ebiggers@kernel.org>
To:     =?iso-8859-1?Q?Lu=EDs?= Henriques <lhenriques@suse.de>
Cc:     Jeff Layton <jlayton@kernel.org>, idryomov@gmail.com,
        xiubli@redhat.com, ceph-devel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-fscrypt@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v11 02/51] fscrypt: export fscrypt_base64url_encode
 and fscrypt_base64url_decode
Message-ID: <YjyubQgfbQbUn4Ct@gmail.com>
References: <20220322141316.41325-1-jlayton@kernel.org>
 <20220322141316.41325-3-jlayton@kernel.org>
 <87zglgoi1e.fsf@brahms.olymp>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87zglgoi1e.fsf@brahms.olymp>
Precedence: bulk

On Wed, Mar 23, 2022 at 02:33:17PM +0000, Lu?s Henriques wrote:
> Hi Eric,
> 
> Jeff Layton <jlayton@kernel.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@google.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.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...)

- Eric