Skip to content

Commit

Permalink
KMS: Clean up base64 logic in the encrypt and decrypt functions. (#1074)
Browse files Browse the repository at this point in the history 
The use of base64 is essentially an implementation detail of the Cloud KMS REST
API: it is required only so that arbitrary binary data can be included in a JSON
string, which only allows Unicode characters. Therefore, the "encrypt" sample
function should decode the base64-encoded ciphertext before writing the
file. Similarly, "decrypt" should not assume that an input file is
base64-encoded, but should perform the base64-encoding itself before sending the
encrypted data to KMS.

This aligns with how the "gcloud kms encrypt" and "gcloud kms decrypt" commands
behave. See https://stackoverflow.com/q/45699472 for an example of user
confusion caused by the mismatch.
  • Loading branch information
Russ Amos authored and Jon Wayne Parrott committed Aug 16, 2017
1 parent 0a86e80 commit e0f957c
Showing 1 changed file with 9 additions and 8 deletions.
17 changes: 9 additions & 8 deletions kms/api-client/snippets.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,18 @@ def encrypt(project_id, location, keyring, cryptokey, plaintext_file_name,
# Read text from the input file.
with io.open(plaintext_file_name, 'rb') as plaintext_file:
plaintext = plaintext_file.read()
encoded_text = base64.b64encode(plaintext)

# Use the KMS API to encrypt the text.
cryptokeys = kms_client.projects().locations().keyRings().cryptoKeys()
request = cryptokeys.encrypt(
name=name, body={'plaintext': encoded_text.decode('utf-8')})
name=name,
body={'plaintext': base64.b64encode(plaintext).decode('ascii')})
response = request.execute()
ciphertext = base64.b64decode(response['ciphertext'].encode('ascii'))

# Write the encrypted text to a file.
with io.open(encrypted_file_name, 'wb') as encrypted_file:
encrypted_file.write(response['ciphertext'].encode('utf-8'))
encrypted_file.write(ciphertext)

print('Saved encrypted text to {}.'.format(encrypted_file_name))
# [END kms_encrypt]
Expand All @@ -109,19 +110,19 @@ def decrypt(project_id, location, keyring, cryptokey, encrypted_file_name,

# Read cipher text from the input file.
with io.open(encrypted_file_name, 'rb') as encrypted_file:
cipher_text = encrypted_file.read()
ciphertext = encrypted_file.read()

# Use the KMS API to decrypt the text.
cryptokeys = kms_client.projects().locations().keyRings().cryptoKeys()
request = cryptokeys.decrypt(
name=name, body={'ciphertext': cipher_text.decode('utf-8')})
name=name,
body={'ciphertext': base64.b64encode(ciphertext).decode('ascii')})
response = request.execute()
plaintext = base64.b64decode(response['plaintext'].encode('ascii'))

# Write the plain text to a file.
with io.open(decrypted_file_name, 'wb') as decrypted_file:
plaintext_encoded = response['plaintext']
plaintext_decoded = base64.b64decode(plaintext_encoded)
decrypted_file.write(plaintext_decoded)
decrypted_file.write(plaintext)

print('Saved decrypted text to {}.'.format(decrypted_file_name))
# [END kms_decrypt]
Expand Down

3 comments on commit e0f957c

@drewrothstein
Copy link

Choose a reason for hiding this comment

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

Just a heads-up that this breaks existing implementations that folks might have such as (basically the sample code):

def _decrypt(project_id, location, keyring, cryptokey, cipher_text):
    """Decrypts and returns string from given cipher text."""
    logging.info('Decrypting cryptokey: {}'.format(cryptokey))
    kms_client = googleapiclient.discovery.build('cloudkms', 'v1')
    name = 'projects/{}/locations/{}/keyRings/{}/cryptoKeys/{}'.format(
        project_id, location, keyring, cryptokey)
    cryptokeys = kms_client.projects().locations().keyRings().cryptoKeys()
    request = cryptokeys.decrypt(
        name=name, body={'ciphertext': cipher_text.decode('utf-8')})
    response = request.execute()
    return base64.b64decode(response['plaintext'])


def _download_output(output_bucket, filename):
    """Downloads the output file from GCS and returns it as a string."""
    logging.info('Downloading output file')
    client = storage.Client()
    bucket = client.get_bucket(output_bucket)
    output_blob = (
        'keys/{}'
        .format(filename))
    return bucket.blob(output_blob).download_as_string()

The _decrypt would need to change to (as done above):

def _decrypt(project_id, location, keyring, cryptokey, cipher_text):
    """Decrypts and returns string from given cipher text."""
    logging.info('Decrypting cryptokey: {}'.format(cryptokey))
    kms_client = googleapiclient.discovery.build('cloudkms', 'v1')
    name = 'projects/{}/locations/{}/keyRings/{}/cryptoKeys/{}'.format(
        project_id, location, keyring, cryptokey)
    cryptokeys = kms_client.projects().locations().keyRings().cryptoKeys()
    request = cryptokeys.decrypt(
        name=name,
        body={'ciphertext': base64.b64encode(cipher_text).decode('ascii')})
    response = request.execute()
    return base64.b64decode(response['plaintext'].encode('ascii'))

While I understand this change and the confusion it caused this particular user mentioned on SO, it actually left me in a loop for a period of time as I was re-reviewing what changed and attempting to find anything that had changed in my codebase and then eventually stumbled here.

I didn't expect to pull an update to this repository and have a breaking change to encrypting new key material when I had not changed any of my code.

Just a heads-up. To be honest, I would have preferred a compatibility mode for a bit with some messaging and eventual deprecation. There isn't a ton of documentation on KMS in the wild and this repository was my goto when first building a couple applications utilizing KMS. Given the lack of usage and documentation a little more hand-holding here would have been appreciated.

@theacodes
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't expect to pull an update to this repository and have a breaking change to encrypting new key material when I had not changed any of my code.

I don't understand, are you saying you're using our sample code repository directly in your app? We have no way of versioning the samples so it's difficult for us to communicate changes (and there are some weeks with upwards of 100 changes here). Generally our samples do not constitute production ready code and are for illustrative purposes only.

That said - this sample change didn't introduce any breaking changes at the service or client library level - you can easily just check out an older version of this sample and everything should work as before.

@drewrothstein
Copy link

@drewrothstein drewrothstein commented on e0f957c Aug 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand, are you saying you're using our sample code repository directly in your app?

Nope, that would be crazy! :)

I am using snippets.py (actually a wrapper around it to make invocation quick/easy) to encrypt secrets and upload them to GCS.

When I pulled this update in and ran snippets.py, the data that was produced, received a similar error to the author because my _decrypt function above had not been updated, thus the breaking change.

Let me know if that isn't clear and I can provide more details.

Cheers!

Please sign in to comment.