| Message ID | 20220322141316.41325-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph+fscrypt : full support | expand |
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,
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
On Wed, Mar 23, 2022, at 10:33 AM, Luís Henriques wrote:
> So, my current proposal is to use a different encoding table.
Another alternative is https://en.wikipedia.org/wiki/Base62
Eric Biggers <ebiggers@kernel.org> writes: > 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...) 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,
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) { u32 ac = 0; int bits = 0; @@ -217,6 +215,7 @@ static int fscrypt_base64url_encode(const u8 *src, int srclen, char *dst) *cp++ = base64url_table[(ac << (6 - bits)) & 0x3f]; return cp - dst; } +EXPORT_SYMBOL_GPL(fscrypt_base64url_encode); /** * fscrypt_base64url_decode() - base64url-decode a string @@ -233,7 +232,7 @@ static int fscrypt_base64url_encode(const u8 *src, int srclen, char *dst) * Return: the length of the resulting decoded binary data in bytes, * or -1 if the string isn't a valid base64url string. */ -static int fscrypt_base64url_decode(const char *src, int srclen, u8 *dst) +int fscrypt_base64url_decode(const char *src, int srclen, u8 *dst) { u32 ac = 0; int bits = 0; @@ -256,6 +255,7 @@ static int fscrypt_base64url_decode(const char *src, int srclen, u8 *dst) return -1; return bp - dst; } +EXPORT_SYMBOL_GPL(fscrypt_base64url_decode); bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy, u32 orig_len, u32 max_len, diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index 91ea9477e9bd..671181d196a8 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -46,6 +46,9 @@ struct fscrypt_name { /* Maximum value for the third parameter of fscrypt_operations.set_context(). */ #define FSCRYPT_SET_CONTEXT_MAX_SIZE 40 +/* len of resulting string (sans NUL terminator) after base64 encoding nbytes */ +#define FSCRYPT_BASE64URL_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) + #ifdef CONFIG_FS_ENCRYPTION /* @@ -305,6 +308,8 @@ void fscrypt_free_inode(struct inode *inode); int fscrypt_drop_inode(struct inode *inode); /* fname.c */ +int fscrypt_base64url_encode(const u8 *src, int len, char *dst); +int fscrypt_base64url_decode(const char *src, int len, u8 *dst); int fscrypt_setup_filename(struct inode *inode, const struct qstr *iname, int lookup, struct fscrypt_name *fname);