add azure KMS as another option in refs #410

anirban1c · 2019-10-24T12:20:48Z

Fixes issue #
Add azure kms as another backend as described here
https://github.com/Azure/azure-sdk-for-python/tree/master/sdk/keyvault/azure-keyvault-keys#cryptographic-operations

this requires a key vault with a key and a service principal with - GET, ENCRYPT and DECRYPT added to it

Proposed Changes

add another module azkms
update cli with --vault option
add in azure pip dependencies

TODO: add a test module for azkms

googlebot · 2019-10-24T12:20:51Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

.gitignore

kapitan/cli.py

uberspot · 2019-10-24T12:44:25Z

kapitan/refs/secrets/azkms.py

+
+logger = logging.getLogger(__name__)
+
+# We are using default credentials as defined in the link:


This is a good candidate have in documentation instead of comments. I'd suggest adding this and any other needed docs for azkms to docs/secrets.md :) That way they are also searchable/indexed in the website https://kapitan.dev

kapitan/refs/secrets/azkms.py

googlebot · 2019-10-25T00:19:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

anirban1c · 2019-10-25T10:15:13Z

@googlebot I signed it!

uberspot · 2019-10-25T14:49:37Z

kapitan/cli.py

-        secret_obj = AzureKMSSecret(data, vault, key, encode_base64=args.base64)
+            raise KapitanError("No KMS vault specified. Use --azure-vault or specify it in parameters.kapitan.secrets.azkms.vault and use --target-name")
+        if not encryption_algorithm:
+            raise KapitanError("No KMS vault specified. Use --azure-key-algo or specify it in parameters.kapitan.secrets.azkms.vault and use --target-name")


No KMS Key algorithm specified* The msg here should be updated.

uberspot

In general looks good! 👍
Don't forget to add some docs under docs/secrets.md.

kapitan/cli.py

uberspot · 2019-10-25T14:50:52Z

kapitan/cli.py

-        #
-        # Azure KMS marker -- remove it
-        #
+        # TODO: same key but in a different vault (check ..)


This TODO should either be fixed before merging or be more detailed so that if needed someone else can also take over. :)

uberspot · 2019-10-25T14:52:19Z

kapitan/refs/secrets/azkms.py

+    rsa_oaep_256 = "RSA-OAEP-256"
+    rsa1_5 = "RSA1_5"
+
+    # an old classic


In comments, we need an explanation of what the code below does or other edge cases.
This comment seems unclear.

uberspot · 2019-10-25T14:53:55Z

kapitan/refs/secrets/azkms.py

+    # an old classic
+    @staticmethod
+    def value_of(value):
+        for m, mm in EncryptionAlgorithm.__members__.items():


What is m and what is mm in this context?
One of them is algorithm members probably. But it helps readability a lot if we use longer-named variables here.

for _attribute_name, _ in EncryptionAlgorithm.members.items():

uberspot · 2019-10-25T15:02:28Z

----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_binary
Traceback (most recent call last):
  File "/opt/python/3.7.1/lib/python3.7/unittest/loader.py", line 434, in _find_test_path
    module = self._get_module_from_name(name)
  File "/opt/python/3.7.1/lib/python3.7/unittest/loader.py", line 375, in _get_module_from_name
    __import__(name)
  File "/home/travis/build/deepmind/kapitan/tests/test_binary.py", line 29, in <module>
    from kapitan.cli import main
  File "/home/travis/build/deepmind/kapitan/kapitan/cli.py", line 37, in <module>
    from kapitan.refs.secrets.azkms import AzureKMSSecret
  File "/home/travis/build/deepmind/kapitan/kapitan/refs/secrets/azkms.py", line 29, in <module>
    from azure.keyvault.keys import KeyClient
ModuleNotFoundError: No module named 'azure.keyvault.keys'

Travis is failing because No module named 'azure.keyvault.keys'
Is there a pip dependency missing from requirements.txt?
I might be wrong but https://docs.microsoft.com/en-us/python/api/overview/azure/key-vault?view=azure-python mentions using from azure.keyvault import KeyVaultClient instead?

ramaro · 2019-11-04T18:19:20Z

kapitan/refs/secrets/azkms.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"gkms secrets module"


change to azkms

ramaro · 2019-11-04T18:22:37Z

kapitan/refs/secrets/azkms.py

+    """
+    #TODO : import the module so that it depends on
+
+    rsa_oaep = "RSA-OAEP"


why not turn this into a dict?

ramaro · 2019-11-04T18:23:42Z

kapitan/refs/secrets/azkms.py

+    # an old recepie using java style attribuet access for Enum's
+    @staticmethod
+    def value_of(value):
+        for _attribute_name, _ in EncryptionAlgorithm.__members__.items():


use dict instead?

pievalentin · 2019-12-02T19:32:53Z

Would be interested in seeing this merged. Is forking and doing a PR good practice?

uberspot · 2019-12-02T20:55:00Z

If you maintain the other authors' commits I don't see why not. :)

anirban1c-maersk added 2 commits October 24, 2019 12:49

.

36c2897

update az loading

835ec38

googlebot added the cla: no label Oct 24, 2019

uberspot requested review from ramaro, uberspot and adrianchifor October 24, 2019 12:37

uberspot requested changes Oct 24, 2019

View reviewed changes

code changes on PR comments

81b2bde

uberspot reviewed Oct 25, 2019

View reviewed changes

fix requirements.txt, cleanup as per comments

d316b8c

ramaro reviewed Nov 4, 2019

View reviewed changes

kapitan/refs/secrets/azkms.py

"""

#TODO : import the module so that it depends on

rsa_oaep = "RSA-OAEP"

Copy link

Member

ramaro Nov 4, 2019

Choose a reason for hiding this comment

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

why not turn this into a dict?

ramaro reviewed Nov 4, 2019

View reviewed changes

ramaro closed this Nov 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add azure KMS as another option in refs #410

add azure KMS as another option in refs #410

anirban1c commented Oct 24, 2019

googlebot commented Oct 24, 2019

uberspot Oct 24, 2019

anirban1c Oct 24, 2019

googlebot commented Oct 25, 2019

anirban1c commented Oct 25, 2019

uberspot Oct 25, 2019

uberspot left a comment

uberspot Oct 25, 2019

uberspot Oct 25, 2019

uberspot Oct 25, 2019

anirban1c Jan 7, 2020

uberspot commented Oct 25, 2019

ramaro Nov 4, 2019

ramaro Nov 4, 2019

ramaro Nov 4, 2019

pievalentin commented Dec 2, 2019

uberspot commented Dec 2, 2019


		logger = logging.getLogger(__name__)

		# We are using default credentials as defined in the link:

add azure KMS as another option in refs #410

add azure KMS as another option in refs #410

Conversation

anirban1c commented Oct 24, 2019

Proposed Changes

googlebot commented Oct 24, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Oct 25, 2019

anirban1c commented Oct 25, 2019

Choose a reason for hiding this comment

uberspot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uberspot commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pievalentin commented Dec 2, 2019

uberspot commented Dec 2, 2019