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

Reduce required API of source streams. #103

Closed
3 tasks done
mattsb42-aws opened this issue Nov 13, 2018 · 2 comments
Closed
3 tasks done

Reduce required API of source streams. #103

mattsb42-aws opened this issue Nov 13, 2018 · 2 comments

Comments

@mattsb42-aws
Copy link
Member

mattsb42-aws commented Nov 13, 2018

Currently, the source stream used by streaming_client streams must implement four APIs:

  • read()
  • tell()
  • close()
  • closed

As raised in #80, there are several cases where it would be useful for us to only require read().

This leaves the following to be considered for elimination:

  • tell()
  • close()
  • closed
@mattsb42-aws
Copy link
Member Author

mattsb42-aws commented Nov 13, 2018

Current uses:

tell()

  • _EncryptionStream.stream_length() : This just tells us that the stream is not seekable and we cannot determine the length of the source. We already have logic to handle that most places, but we do have a dependency in StreamEncryptor._prep_non_framed()on preparing the AAD for non-framed bodies on encrypt.
  • StreamDecryptor._prep_non_framed() : Used to set the body_start value. We do not use this value anywhere except to set body_end, which we only use in _read_bytes_from_non_framed_body, as discussed below. These are completely unnecessary values, as we already have the body length from parsing the body intro. They are likely relics of early pre-release logic before we required that the full body be decrypted before any plaintext is released. Unfortunately, when I originally wrote this class I made them public.
    • After discussing, we determined that what we will do here is make this change, including logic to set these values by counting bytes read rather than from tell(). We will then introduce this change in a Z release and mark these attributes as deprecated. We will then later remove these attributes in a Y release.
  • StreamDecryptor._read_bytes_from_non_framed_body() : Used to determine how many more bytes to read. What we should do here instead is to use the body length that we already read to determine how many bytes we need to read.
  • deserialize.deserialize_non_framed_values() : Used to retrieve the tag before reading the body. We should simply read all ciphertext into memory, then read the tag, since we have to read everything into memory anyway before we can release any plaintext.

close()

If we were implementing this new, I would say that we should not try to close the source stream at all. All of these are simply to communicate to other parts of these classes through their use of closed.

  • StreamEncryptor._read_bytes_to_non_framed_body() : Closes the source stream once we determine that we have as many bytes from it as we can expect.
  • StreamEncryptor._read_bytes_to_framed_body() : Closes the source stream after we write the message footer.
  • StreamDecryptor._read_bytes_from_non_framed_body() : Closes the source stream after we read the message footer.
  • StreamDecryptor._read_bytes_from_framed_body() : Closes the source stream after we read the message footer.

closed

  • _EncryptionStream.read() : Used to keep reading until the source is closed. We can rework this instead just keep reading until we don't get anything back.
  • _EncryptionStream.next() : Used to determine if we should StopIteration. I think that a better approach here would be to instead simply try reading and raise StopIteration if we either get nothing back or hit a ValueError.
  • StreamDecryptor._read_bytes() : Used to short-circuit read if the source is closed. We should just check for the footer instead..
  • StreamEncryptor._read_bytes() : Used to short-circuit read if the source is closed. We need this check to avoid simplify avoiding trying to use the signer once it is finalized.

@mattsb42-aws
Copy link
Member Author

This is now complete and will be available in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant