Re: How to get my krb5 crypto lib upstream?

[Chuck Lever III]

> On May 31, 2023, at 1:02 PM, David Howells <dhowells@xxxxxxxxxx> wrote:
> 
> Hi Herbert, Chuck,
> 
> I'm wondering how to make progress on getting my krb5 crypto lib upstream.
> 
> Can I push it as it stands and then we try and build it into the crypto API
> for it later?  That would allow me to push my rxgk implementation for AF_RXRPC
> and at least allow userspace to use that.

Sharing our private conversation during LSF here:

My feeling is crypto/krb5 needs at least one consumer, and
it makes sense for the first one to be AFS.

Once crypto/krb5 is in the tree, I'll be glad to look at
starting to replace the internals of the SunRPC GSS Kerberos
mechanism with what is provided in crypto/krb5.

However, if there is going to be significant API churn after
crypto/krb5 lands, I'd like to wait until that stabilizes
before adopting major pieces of crypto/krb5 in SunRPC.


> As far as building a crypto API around it goes, we need four interfaces:
> 
> (1) Key generation.  We may need to generate a {cipher,hash} key pair {Ke,Ki}
>     or just a hash key Kc.  We might conceivably want both.
> 
>     At the moment, I return a prepared cipher or a prepared hash.  I don't
>     deal with the key pairing here as it makes testing a bit more awkward.
> 
> int crypto_krb5_get_Kc(const struct krb5_enctype *krb5,
>       const struct krb5_buffer *TK,
>       u32 usage,
>       struct krb5_buffer *key,
>       struct crypto_shash **_shash,
>       gfp_t gfp);
> int crypto_krb5_get_Ke(const struct krb5_enctype *krb5,
>       const struct krb5_buffer *TK,
>       u32 usage,
>       struct krb5_buffer *key,
>       struct crypto_sync_skcipher **_ci,
>       gfp_t gfp);
> int crypto_krb5_get_Ki(const struct krb5_enctype *krb5,
>       const struct krb5_buffer *TK,
>       u32 usage,
>       struct krb5_buffer *key,
>       struct crypto_shash **_shash,
>       gfp_t gfp);
> 
> (2) PRF+ generation.  This takes some a key and a metadata blob and generates
>     a new blob that then gets used as a key.
> 
> int crypto_krb5_calc_PRFplus(const struct krb5_enctype *krb5,
>     const struct krb5_buffer *K,
>     unsigned int L,
>     const struct krb5_buffer *S,
>     struct krb5_buffer *result,
>     gfp_t gfp);
> 
> (3) Encrypt and Decrypt.
> 
>     Encrypt has to be parameterised to take a specific confounder for testing
>     and generate a random one for normal operation.  The IV is fixed all
>     zeroes in the cases I've implemented.  When testing, decrypt should
>     perhaps be passed the confounder to check it.
> 
>     When encrypting, the output buffer will be larger than the input buffer
>     (or, at least, room must be set aside) so that a confounder, padding and
>     a checksum can be inserted.
> 
>     When decrypting, we either want to provide a separate output buffer so
>     that the confounder and checksum can be stripped off, or we need to be
>     able to find out where the decrypted payload plus the padding will be (we
>     can't work out how much padding there is - that's left to the caller).
> 
>     At the moment, I pass a single buffer descriptor, providing encrypt with
>     extra space front and back and providing decrypt with somewhere to save
>     offset and length:
> 
> ssize_t crypto_krb5_encrypt(const struct krb5_enctype *krb5,
>    struct krb5_enc_keys *keys,
>    struct scatterlist *sg, unsigned int nr_sg,
>    size_t sg_len,
>    size_t data_offset, size_t data_len,
>    bool preconfounded);
> int crypto_krb5_decrypt(const struct krb5_enctype *krb5,
> struct krb5_enc_keys *keys,
> struct scatterlist *sg, unsigned int nr_sg,

So are we going to stick with struct scatterlist here,
or should it be rather an iterator of some kind?


> size_t *_offset, size_t *_len,
> int *_error_code);
> 
>     I also allow a krb5/gssapi error code to be returned so that it can be
>     used in the generation of abort messages.  This needs sorting out
>     better.  It may be that only one code is actually relevant to this and
>     that the caller generates all the rest as it checks the metadata.
> 
>     The AEAD interface might suffice as it stands if we pass in the keys
>     already generated and passed in as a single key blob.  We don't want an
>     IV generator as the protocol defines the IV to use.
> 
> (4) GetMIC and VerifyMIC.
> 
>     Both of these need parameterising with extra metadata that will get
>     inserted into the hash before the data is hashed.  GetMIC will insert the
>     checksum into the buffer and VerifyMIC will check it and strip it off.
> 
>     I'm not sure that the hash API is suitable for this.  AEAD might suit for
>     GetMIC at least, but using AEAD for VerifyMIC would lead to an extraneous
>     copy I'd prefer to avoid.
> 
> 
> ssize_t crypto_krb5_get_mic(const struct krb5_enctype *krb5,
>    struct crypto_shash *shash,
>    const struct krb5_buffer *metadata,
>    struct scatterlist *sg, unsigned int nr_sg,
>    size_t sg_len,
>    size_t data_offset, size_t data_len);
> int crypto_krb5_verify_mic(const struct krb5_enctype *krb5,
>   struct crypto_shash *shash,
>   const struct krb5_buffer *metadata,
>   struct scatterlist *sg, unsigned int nr_sg,
>   size_t *_offset, size_t *_len,
>   int *_error_code);
> 
> There's a lot to be said in using an asynchronous overrideable interface for
> encrypt and decrypt.  The problem is that we want to do simultaneous hash and
> crypt if we can.  I think the Intel AES/SHA instructions permit this to be
> done and there is sufficient register space to do it - and I *think* that it
> may be possible to offload this to the Intel QAT or the Intel IAA on the
> latest 4th Gen Xeons - and maybe NICs that can handle NFS/SMB offload.

I agree that a hardware-based AES/SHA implementation of
encrypt/decrypt seems like a good step forward for storage
consumers like NFS and AFS. That could be a significant
benefit for switching SunRPC GSS to crypto/krb5.


> I think we'll perhaps need a "krb5 encoding type" API that can provide key
> derivation, PRF+ and information - something along the following lines:
> 
> struct krb5_enctype {
> int etype; // Encryption (key) type
> int ctype; // Checksum type
> const char *name; // "Friendly" name
> const char *encrypt_name; // Crypto encrypt name
> const char *cksum_name; // Crypto checksum name
> const char *hash_name; // Crypto hash name
> u16 block_len; // Length of encryption block
> u16 conf_len; // Length of confounder
> u16 cksum_len; // Length of checksum
> u16 key_bytes; // Length of raw key
> u16 key_len; // Length of final key
> u16 hash_len; // Length of hash
> u16 prf_len; // Length of PRF() result
> u16 Kc_len; // Length of Kc
> u16 Ke_len; // Length of Ke
> u16 Ki_len; // Length of Ki
> bool keyed_cksum; // T if a keyed cksum
> bool pad; // T if should pad
> };
> 
> We need to be able to look the encoding up by ID, not by name.

It's not clear why something like this would need to be
exposed to crypto/krb5 consumers. There are a few items
in here that XDR needs to know about (lengths and such)
but that kind of thing can be provided by a function
call rather than by having direct access to a structure.


--
Chuck Lever