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 read, write, destroy secrets #2

Merged
merged 1 commit into from Jul 1, 2019

Conversation

2 participants
@theomessin
Copy link
Member

commented Jun 28, 2019

This change is Reviewable

@theomessin theomessin requested a review from kkom Jun 28, 2019

@kkom

kkom approved these changes Jun 28, 2019

Copy link

left a comment

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


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

# @todo: add checks to ensure shush has been initialized in all methods.

hhhhmmmmm...

as in __init__() initialised?

or .initialize() initialised?

the first one can be assumed, since we're dealing with instance methods, right?

the second one – just write your code so that it fails at runtime if it's not initialized remotely, e.g. catch exceptions appropriately

the outcome to the programmer is the same, but it doesn't slow down execution in the standard case where everything is initialized

in short – these checks are harmful in my opinion


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

from google.cloud import kms_v1
from google.cloud import storage
from google.cloud.kms_v1 import enums

<3


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

        crypto_key = {'purpose': purpose}
        self.kms_client.create_crypto_key(keyring_name, "default", crypto_key)

what about the rotation period?


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

            self.keyring_location,
            "shush-secrets",
            "default",

make this into a constant please, so that we don't copy paste it


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

        )

        blob.upload_from_string(ciphertext)

lol, this is bytes right?


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

            self.keyring_location,
            "shush-secrets",
            "default",

constant


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

        blob = bucket.get_blob(
            f"{self.keyring_location}/"
            f"default/{secret_name}.encrypted",

constant again imo


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

        if blob is None:
            raise ValueError(f"Could not find secret with name {secret_name}.")

maybe let's print the full path to the blob instead?

this will help with debugging I think!


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

            raise ValueError(f"Could not find secret with name {secret_name}.")

        blob.delete()

sooo clean, wtf!

@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)


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

Previously, kkom (Konrad Komorowski) wrote…

hhhhmmmmm...

as in __init__() initialised?

or .initialize() initialised?

the first one can be assumed, since we're dealing with instance methods, right?

the second one – just write your code so that it fails at runtime if it's not initialized remotely, e.g. catch exceptions appropriately

the outcome to the programmer is the same, but it doesn't slow down execution in the standard case where everything is initialized

in short – these checks are harmful in my opinion

The second one, yeah basically need to handle exceptions properly.


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

Previously, kkom (Konrad Komorowski) wrote…

what about the rotation period?

I don't think this should be part of the library.
You can just set it up from the UI manually after initialisation...


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

Previously, kkom (Konrad Komorowski) wrote…

make this into a constant please, so that we don't copy paste it

Done in next PR.


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

Previously, kkom (Konrad Komorowski) wrote…

lol, this is bytes right?

Yeah, tell that to Google 😆


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

Previously, kkom (Konrad Komorowski) wrote…

constant

Done in next PR.


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

Previously, kkom (Konrad Komorowski) wrote…

constant again imo

Done in next PR.


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

Previously, kkom (Konrad Komorowski) wrote…

maybe let's print the full path to the blob instead?

this will help with debugging I think!

Done in next PR.

@theomessin theomessin merged commit 708d5bc into master Jul 1, 2019

1 check was pending

code-review/reviewable 7 discussions left (kkom)
Details

@theomessin theomessin deleted the read_write_destroy branch Jul 1, 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.