-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add aes cipher multicodecs #202
base: master
Are you sure you want to change the base?
Conversation
So you're making a generic "cipher" type with entries that could be used in a variety of ways, but you're specifically using them as IPLD codecs to encode and decode specific forms, |
I wouldn’t consider them If someone wants to refer to these ciphers in some other context, outside of CID’s, and needs an int, hex or varint, then they should be able to reuse this code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 I guess I don't have any solid objections. I'm not super comfortable with the distinction between cipher
and ipld
since the purpose of adding them here is to enable development of an ipld
codec and there's currently no other purpose proposed for them. But I'm also not a big fan of the way we're using the tag in here either so one could read cipher
as "IPLD encrypted codec" if you want to stretch it.
Tag in the codec tableIt seems bizarre to me to encourage the reuse of I understand the claim that this codec is so simple that it in a sense "must" be correct, but why bother taking the risk and dealing with the associated fallout when we could just call it an IPLD (or serialization since there are no links) codec and if in the future anyone finds a use for an OverallI've already expressed my concern for adding these codecs at all offline (and summarized in https://github.com/ipld/specs/pull/349/files#r559963156). It's a 🤷 from me as my understanding of the general policy of the code table is to not object if someone makes a request for a code as long as the number of codes being requested isn't large and the code size is sufficiently large for the request. Instead we just try and discuss the options and guide people down paths we think will be helpful to them. I feel like I've done that and so the rest is up to you 😄. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I give it a "request changes" so that it doesn't get merged for now.
I don't have new objections, but I agree with @aschmahmann that there should be a single codec only and not three. I really like his "alternatives" outlined here: ipld/specs#349 (comment)
Ok, I’ve updated this PR to be in line with my most recent comment. This should address the comments here as it collapses the use case into a single block format. There are still multicodec entries for each cipher but thats for general use (and use within the block format itself) meaning that the table entries for AES variants no longer correspond to a block format and do not have a meaning when used as a the codec identifier in a CID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's much more cozy for this table, and I'm fine with the format, although suspect that some experimentation with extending the list of ciphers might extend the format, but that doesn't preclude getting an identifier registered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should have aes-gcm
, aes-cdc
and aes-ctr
or if it should be split into aes
, gcm
, cdc
, ctr
and then the encrypted block
would need to specify two multicodecs, one for the block cipher and one for the block operation mode.
But I'm not deep enough into the crypto space, so I don't know how many possible combinations there are to expect and whether that will explode or practically it's a small list of combination between ciphers and operation modes (which is what I suspect). So I don't want to block this.
One might be able to make the argument that The way it's been implemented at the moment @ https://github.com/multiformats/js-multiformats/pull/59/files#diff-d971711275e038fa06255a059bdaa381debc468ec6473d31cba5d230beef949aR50 leaves it up to the user, whether they supply a 16-byte or 32-byte
|
This is really a rabbit hole. I'm not deep enough (and hopefully won't get any deeper) to have a strong opinion about it. Though having three independent items that can be combined together in several different ways sounds a bit like different Multicodec codes/size information. But that may well be overkill/bloat things. Another option could be to define them as bit flags and then use the resulting values as Multicodec code. I haven't really thought this idea through, hence I'm unsure if this would be viable. |
TL;DRIn case this is helpful, I think that this PR as it stands is good to go 👍 CiphertextsAs an end user, the main thing that I'm interested in knowing is that a ciphertext was encrypted with AES in a particular mode (CTR, GCM, etc). I don't really care about the key length; presumably I know that if I have the key. It would be nice if this format also defined where the IV sat, and possibly its length (usually 96-bits, but I've seen 128 in the wild). KeysIf you want to describe the keys themselves (sounds dangerous so not sure why you'd want to), I don't need to know what mode it will be used in. I only care about the key type, and I suppose the length (though it's usually so short that I could just take the length) |
How do you propose that's done here? If the length, and key is entirely a user concern, shouldn't the IV be too? |
Followup from today’s IPLD call.
Working on an implementation of block encryption and I need to assign multicodecs for the main AES ciphers.
I think this is a good range to start adding ciphers in but if anyone has a better idea I’m open to it.