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

Code list_secrets and fix dependencies. #3

Merged
merged 1 commit into from Jul 11, 2019

Conversation

2 participants
@theomessin
Copy link
Member

commented Jul 1, 2019

Also code write_secret_from_file()


This change is Reviewable

@theomessin theomessin requested a review from kkom Jul 1, 2019

@kkom
Copy link

left a comment

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @theomessin)


Pipfile, line 11 at r1 (raw file):


[packages]
google-cloud-kms = ">=1.1"

do you think it makes sense to say it has to be less than 2.x? any idea on how google plans to upgrade things?


Pipfile, line 12 at r1 (raw file):

[packages]
google-cloud-kms = ">=1.1"
google-cloud-storage = ">=1.16"

do you think it makes sense to say it has to be less than 2.x? any idea on how google plans to upgrade things?


examples/helloworld.py, line 9 at r1 (raw file):

)

# client.initialize()

say why you're commenting it out :)


examples/helloworld.py, line 13 at r1 (raw file):

client.write_secret("abc_api_key", b"s5TmXcIS9L41hkRdOmg4SpDcd3m5l6Yg")
client.write_secret("xkcd_password", b"correct horse battery staple")
# client.write_secret("top_secret", open("my.secret", "rb"))

why is this commented out?


shush/client.py, line 90 at r1 (raw file):

    def _get_secret_path(self, name: str) -> str:
        """Get the bucket relative GCS path to a secret"""
        return f"{self.keyring_location}/{self.KEY_NAME}/{name}"

wouldn't this be a good place to add the .encrypted suffix?


shush/client.py, line 105 at r1 (raw file):

        blobs = self.bucket.list_blobs(prefix=self._get_secret_path(""))
        filenames = map(lambda b: path.basename(b.name), blobs)
        return list(map(lambda f: path.splitext(f)[0], filenames))

what does path.splitext do? is it guaranteed to return always a 2-element list?


shush/client.py, line 118 at r1 (raw file):

    def write_secret_from_file(self, name: str, plainfile: BinaryIO) -> None:
        """Usage: write_secret_from_file("secret", open("private", "rb"))"""
        plaintext: bytes = plainfile.read(64 * 1024 + 1)

please put 64 * 1024 in a constant! this will serve as auto-documentation as well


shush/client.py, line 120 at r1 (raw file):

        plaintext: bytes = plainfile.read(64 * 1024 + 1)
        if len(plaintext) > (64 * 1024):
            raise ValueError(f"Secret size must be less than 64KiB.")

nit: 64 KiB


shush/client.py, line 123 at r1 (raw file):

        return self.write_secret(name, plaintext)

    def destroy_secret(self, secret_name: str) -> None:

soooo clean, love it!

@theomessin theomessin force-pushed the code_list_fix_deps branch from 9186962 to d9ca54f Jul 1, 2019

@theomessin theomessin requested a review from kkom Jul 1, 2019

@theomessin
Copy link
Member Author

left a comment

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @kkom)


Pipfile, line 11 at r1 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

do you think it makes sense to say it has to be less than 2.x? any idea on how google plans to upgrade things?

I like this idea. Done!


Pipfile, line 12 at r1 (raw file):

I like this idea. Done!


examples/helloworld.py, line 9 at r1 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

say why you're commenting it out :)

Fixed.


examples/helloworld.py, line 13 at r1 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

why is this commented out?

Fixed.


shush/client.py, line 90 at r1 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

wouldn't this be a good place to add the .encrypted suffix?

I decided to get rid of the extension, don't think it's necessary.


shush/client.py, line 105 at r1 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

what does path.splitext do? is it guaranteed to return always a 2-element list?

Yes 😃 https://docs.python.org/3/library/os.path.html#os.path.splitext


shush/client.py, line 118 at r1 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

please put 64 * 1024 in a constant! this will serve as auto-documentation as well

Done.


shush/client.py, line 120 at r1 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

nit: 64 KiB

Google doesn't have a space but sure 😛

@theomessin theomessin force-pushed the code_list_fix_deps branch from d9ca54f to 0488872 Jul 1, 2019

@kkom

kkom approved these changes Jul 1, 2019

Copy link

left a comment

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @theomessin)


Pipfile, line 8 at r2 (raw file):

[dev-packages]
mypy = "==0.711"
flake8 = "==3.7"

this can be 3.7.7 then –> look at the lock file diff


shush/client.py, line 105 at r1 (raw file):

Previously, theomessin (Theodore Messinezis) wrote…

Yes 😃 https://docs.python.org/3/library/os.path.html#os.path.splitext

why are we doing it though?

we no longer have the .encrypted suffix, right?


shush/client.py, line 116 at r2 (raw file):

        blob.upload_from_string(kms_response.ciphertext)

    def write_secret_from_file(self, name: str, plainfile: BinaryIO) -> None:

can we add something that will verify at runtime that we're getting the right type?

maybe we can use typeguard and call the check_argument_types function?

I am worried that people will not open the file in the right way


shush/client.py, line 119 at r2 (raw file):

        """Usage: write_secret_from_file("secret", open("private", "rb"))"""
        kms_max_plaintext_size = 64 * 1024  # Must be up to 64 kibibytes.
        plaintext: bytes = plainfile.read(kms_max_plaintext_size + 1)

Python isn't able to automatically infer that?

well, that's too bad

@theomessin theomessin force-pushed the code_list_fix_deps branch from 0488872 to b6e99ae Jul 11, 2019

@theomessin theomessin requested a review from kkom Jul 11, 2019

@theomessin
Copy link
Member Author

left a comment

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @kkom)


Pipfile, line 8 at r2 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

this can be 3.7.7 then –> look at the lock file diff

Done.


shush/client.py, line 105 at r1 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

why are we doing it though?

we no longer have the .encrypted suffix, right?

Shit, you're right. Fixed.


shush/client.py, line 116 at r2 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

can we add something that will verify at runtime that we're getting the right type?

maybe we can use typeguard and call the check_argument_types function?

I am worried that people will not open the file in the right way

Done.


shush/client.py, line 119 at r2 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

Python isn't able to automatically infer that?

well, that's too bad

I just put that for my own sake. Now with the new type checking it's not needed.

@theomessin
Copy link
Member Author

left a comment

>>> client.write_secret_from_file("secret", open("secret", "r"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/theodore/Workspace/hackpartners/shush/shush/client.py", line 119, in write_secret_from_file
    raise ValueError(f"Type {type(plainfile)} is not BufferedReader.")
ValueError: Type <class '_io.TextIOWrapper'> is not BufferedReader.

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @kkom)

@kkom

kkom approved these changes Jul 11, 2019

Copy link

left a comment

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @theomessin)


shush/client.py, line 118 at r3 (raw file):

    def write_secret_from_file(self, name: str, plainfile: BinaryIO) -> None:
        """Usage: write_secret_from_file("secret", open("private", "rb"))"""
        if not isinstance(plainfile, BufferedReader):

can you just add a comment saying that BufferedReader is the Bytes thing? (as this was confusing for me)

@kkom
Copy link

left a comment

client.write_secret_from_file("secret", open("secret", "r"))
Traceback (most recent call last):
File "", line 1, in
File "/home/theodore/Workspace/hackpartners/shush/shush/client.py", line 119, in write_secret_from_file
raise ValueError(f"Type {type(plainfile)} is not BufferedReader.")
ValueError: Type <class '_io.TextIOWrapper'> is not BufferedReader.

i would actually put that in a test!

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @theomessin)

@theomessin theomessin force-pushed the code_list_fix_deps branch from b6e99ae to 7b3913a Jul 11, 2019

@theomessin theomessin requested a review from kkom Jul 11, 2019

@theomessin
Copy link
Member Author

left a comment

Done!

Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @kkom)


shush/client.py, line 118 at r3 (raw file):

Previously, kkom (Konrad Komorowski) wrote…

can you just add a comment saying that BufferedReader is the Bytes thing? (as this was confusing for me)

Done.

@theomessin theomessin merged commit 50b22b0 into master Jul 11, 2019

1 check was pending

code-review/reviewable 2 files, 1 discussion left (kkom)
Details

@theomessin theomessin deleted the code_list_fix_deps branch Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.