Skip to content
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

[WIP] Started trying to document symmetric encryption #26

Merged
merged 21 commits into from
Aug 9, 2013

Conversation

alex
Copy link
Member

@alex alex commented Aug 8, 2013

No description provided.

>>> from cryptography.primitives import BlockCipher, CBC
>>> from cryptography.primitives.aes import AES
>>> cipher = BlockCipher(AES(key), CBC(iv))
>>> cipher.encrypt("my secret message") + cipher.finalize()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring a .finalize() call seems error-prone and adding strings together like this is inefficient (ignoring CPython optimizations). Wouldn't it be better, if BlockCipher would behave like a StringBuilder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no way to avoid a finalize call. Are you suggesting the API should be .encrypt() and then .getvalue() at the end to get the full string? We discussed this, and the conclusion was that buffering should be provided at a higher level.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I haven't seen that discussion. Ignore me.

[...]

Here ``key`` is the encryption key (which must be kept secret), and ``iv`` is
the initialization vector (which should be random). Exactly what form these
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not should; must


``encrypt()`` should be called repeatedly with additional plaintext, and it
will return the encrypted bytes, if there isn't enough data, it will buffer it
internally. ``finalize()`` should be called at the end, and will return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about buffering internally. I think that padding will take care of the difference but I could be wrong about that. (E.g. a block that is too small will be padded out to be the right size). We should make sure about that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They way it should work is encrypt(thing-half-as-long-as-block_size) returns b"", and then encrypt(another-thing-half-as-long), returns the encrypted block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should be a higher level thing. Padding should handle the difference so encypt(thing-half-as-long-as-block-size) returns an encrypted chunk exactly as long as block size which is the encrypted version of thing-half-as-long-as-block-size padded out using the padding scheme. Perhaps @lvh can clarify? :/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Padding handles the issue of your plaintext being potentially shorter than a multiple of the block size. So you add as much padding to the end of the entire plaintext as is necessary to bring it up to the desired length or an entire block and you include information about how to remove it. Imagine for a moment .encrypt() would add padding to the given piece of plain text, how would you, given the entire plain text, decide what is padding and what isn't?

:param bytes initialization_vector: Must be random bytes. They do not need
to be kept secret (they can be included
in a transmitted message). Should be
the same number of bytes as the ``key``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IV is the block size, not the key size.

@dstufft
Copy link
Member

dstufft commented Aug 8, 2013

I'm liking the way this is looking. I noticed the BlockCipher class is missing a decrypt() method, intentional?

.. class:: cryptography.primitives.block.BlockCipher(cipher, mode)

Block ciphers work by encrypting content in chunks, often 64- or 128-bits.
Theycombine an underlying algorithm (such as AES), with a mode (such as CBC,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Theycombine" -> "They combine"

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b12f76e on symmetric-encryption-docs into 80cc920 on master.

@alex
Copy link
Member Author

alex commented Aug 9, 2013

@dstufft Not so much intentional as I wanted to start with the minimal bits to be able to write code.

@dstufft
Copy link
Member

dstufft commented Aug 9, 2013

This is probably as good as place as any to start moving forward with. It looks like a reasonable API and seems to have everything we need for encryption and we can always iterate from here.

dstufft added a commit that referenced this pull request Aug 9, 2013
[WIP] Started trying to document symmetric encryption
@dstufft dstufft merged commit 4bc4519 into master Aug 9, 2013
@dstufft dstufft deleted the symmetric-encryption-docs branch August 9, 2013 02:15
joerichter-stash pushed a commit to kiwigrid/cryptography that referenced this pull request Nov 15, 2017
Pass a Connection instance to the info callback (instead of a Context instance).  Fixes pyca#16 (a regression introduced by the Python rewrite).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants