-
Notifications
You must be signed in to change notification settings - Fork 166
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
Using GCM Nonce pattern for CBC, CFB, and CTR #261
base: main
Are you sure you want to change the base?
Conversation
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.
This is a really nice change in terms of getting the API laid out! However, it'd be really nice to keep the Nonces and IVs stored directly, instead of using Data
objects. Can we return to that representation?
Yeah, I'll give it a shot |
changed from Data back to the Tuple style. I did give them typealiases |
Just a friendly bump: @Lukasa in case it gets lost in the shuffle |
@swift-server-bot add to allowlist |
} | ||
|
||
/// Returns an iterator over the elements of the nonce. | ||
public func makeIterator() -> Array<UInt8>.Iterator { |
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.
Let's use our own iterator type to hide the face that we're using Array. We can make this cheaper in future, and it would probably be nice to do so.
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.
Sounds good
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 gave it a shot, and made a generic Iterator that could be used by all three.
It might make sense to bring it into the GBC and ChaChaPoly nonces too
fileprivate struct ByteIterator<T>: IteratorProtocol { | ||
var currentOffset = 0 | ||
var pointer: UnsafeRawBufferPointer? = nil | ||
let length: Int |
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.
Great change! Let's drop the pointer though. In the fullness of time this may end up using Span
, but for now we can still use the Array iterator in here. I don't think we want a pointer, so we'd have to use some other spelling.
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.
OK. I wasn't sure if you were suggesting to also drop the offset and length. (I was trying to figure out what you meant by Span
)
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.
Oh sorry, span is here
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 don't think we need the length tracking any longer, array's iterator can do that.
fileprivate struct ByteIterator<T>: IteratorProtocol { | ||
var currentOffset = 0 | ||
var pointer: UnsafeRawBufferPointer? = nil | ||
let length: Int |
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 don't think we need the length tracking any longer, array's iterator can do that.
} | ||
|
||
/// Returns an iterator over the elements of the nonce. | ||
public func makeIterator() -> some IteratorProtocol<UInt8> { |
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.
Let's add @inlinable
annotations to every function on this type. It'll require @usableFromInline
on the typealias, var bytes
, and static var emptyBytes
but that should be fine too.
Following GCM use of gyb for Nonce generation, I converted IV and Nonce classes of the CBC, CFB, and CTR to allow conformance to ContinuousBytes and Sequence.
Checklist
If you've made changes to
gyb
files.script/generate_boilerplate_files_with_gyb
and included updated generated files in a commit of this pull requestMotivation:
The project I am working on uses a AES CBC algorithm to send encrypted data to exchange keys between a server and client. This means, that the client and server exchange the IV/Nonce with each other. Currently, we can only use CommonCrypto, because AES._CBC.IV doesn't facilitate an API to get the internal data bytes.
Modifications:
Removed the current IV and Nonce structs from their corresponding files. Made a Nonces.swift.gyb file that will generate the corresponding structs for each algorithm.
Result:
Allows us to integrate Crypto into our repository and fix an issue we have involving padding of our cipher data, when using CCCrypt
I'm happy to add unit tests for this PR, however I don't think they are needed. While the conformance to new Protocols is public, I believe their use is also internal and so they are being exercised by the current set of unit tests.