Split lambda handler to support generating Host certs #79

pecigonzalo · 2018-08-03T16:26:40Z

Clean PR implementing #77

coveralls · 2018-08-03T16:30:07Z

Coverage decreased (-0.4%) to 94.587% when pulling ed85a7f on pecigonzalo:feature/split_host_provider into f82e2a9 on Netflix:master.

pecigonzalo · 2018-08-03T16:31:46Z

@russell-lewis sorry for the delay, I believe this implements most of what we discussed. I sorted the code so its cleaner for the PR as well.

Added arg kwargs to the wrapper, to keep it simple, but I can add all the args if that is preferred, additionally copied/renamed the tests for the user to the wrapper, as to ensure it works.

The only thing left to do is to allow validity of host certs to be configurable, while in most cases I believe something like 1yr will be OK, but I did not know what is the preferred format for that in your config file. If you have a preference I can implement it.

pecigonzalo · 2018-08-03T18:11:29Z

FYI: I have a working WIP agent to run on the hosts, based on your example agent and using click.

russell-lewis · 2018-08-03T17:49:06Z

bless/aws_lambda/bless_lambda.py

+def lambda_handler_user(
+        event, context=None, ca_private_key_password=None,
+        entropy_check=True,
+        config_file=os.path.join(os.path.dirname(__file__), 'bless_deploy.cfg')):


config_file=None
That way we can use the global_bless_cache when running in a lambda, which won't set any of the optional arguments. The right default gets set at:
https://github.com/Netflix/bless/pull/79/files#diff-339b232e633aae1e41dec2a7824d5ac8R347

russell-lewis · 2018-08-03T17:57:49Z

tests/aws_lambda/test_bless_lambda_wrapper.py

+
+
+def test_basic_local_request():
+    output = lambda_handler(VALID_TEST_REQUEST, context=Context,


This duplication of the lambda_handler_user tests seems like overkill. How about just adding one basic test of lambda_handler() to test_bless_lambda_user.py ?

russell-lewis · 2018-08-03T22:22:56Z

bless/aws_lambda/bless_lambda.py

+def lambda_handler_host(
+        event, context=None, ca_private_key_password=None,
+        entropy_check=True,
+        config_file=os.path.join(os.path.dirname(__file__), 'bless_deploy.cfg')):


config_file=None

russell-lewis · 2018-08-03T22:44:32Z

tests/aws_lambda/test_bless_lambda_host.py

+    invoked_function_arn = 'bogus invoked_function_arn'
+
+
+VALID_TEST_REQUEST = {


I'd also add a test for multiple hostnames and invalid hostnames.

russell-lewis · 2018-08-03T22:57:59Z

tests/request/test_bless_request_user.py

@@ -1,7 +1,7 @@
 import pytest


We should add a test suite for the BlessHostSchema.

russell-lewis · 2018-08-03T23:03:57Z

bless/request/bless_request.py

+
+
+class BlessHostSchema(Schema):
+    hostnames = fields.Str(required=True)


We can use marshmallow to validate the individual hostnames, leveraging their URL validator https://marshmallow.readthedocs.io/en/3.0/api_reference.html#marshmallow.validate.URL

russell-lewis · 2018-08-04T00:26:02Z

bless/aws_lambda/bless_lambda.py

+        time.strftime("%Y/%m/%d %H:%M:%S", time.gmtime(valid_before))
+    )
+
+    # todo: verify multiple hostnames are supported in server certs


you can remove that #todo comment. multiple hostnames are supported.

Yep, tested and working on the client linked.

russell-lewis · 2018-08-04T00:30:21Z

bless/aws_lambda/bless_lambda.py

+
+    # cert values determined only by lambda and its configs
+    current_time = int(time.time())
+    # todo: config server cert validity range


How about two new config vars like the following?

[Bless Options]
server_certificate_validity_after_seconds = 31536000
server_certificate_validity_before_seconds = 120

russell-lewis · 2018-08-04T00:42:35Z

This looks like a great starting point.
If you'd like me to make some changes to address the comments, I can do that.

Ultimately, I think would be good to allow for things like the additional verification found at https://github.com/lyft/bless/blob/lyft-host-certificate-lambda/bless/aws_lambda/bless_lambda.py
although theirs is very specific to their environment, and not generalized or configurable.

The issue with server certs issued like this is you can obtain a server cert for any hostname. That may or may not be a concern in your environment.

pecigonzalo · 2018-08-04T07:37:37Z

@russell-lewis Ill work on the comments, I believe they are all valid points. I did not know Lyft had a branch with this implemented, ill check for the additional verification as it can be valuable.

As for the last concern, I believe that is correct, Ill check what sort of "locking" we can do around that, but for the most part I think it could be a hard thing to validate. I personally do not believe its a concern to be honest as this is mostly about not getting fingerprint issues when sshing to servers.

pecigonzalo · 2018-08-04T11:06:22Z

@russell-lewis implemented most changes, let me know if it LG and ill squash the fixups or let me know if there is a test case I'm missing here.

Regarding "allowed" hostnames, I saw the code from Lyft and I believe its quite tailored to their infrastructure. The only way I can think about how would be more generic, is using an instance tag EG: bless:AllowedHostnames and then BLESS could query the tag in the instance or similar, but still will make it more complex for deployments across several accounts. The only change of compromise would be someone/or the instance getting write access to its own tags.
The thing that might be worth implementing is the kmsauth on the host handler, as that is not there right now, and I can see more value on that.
IMHO a host that can kmsauth and managed to spoof your instance or network would be quite rare and at that point, you probably have other problems 😄

pecigonzalo · 2018-08-17T18:48:03Z

@russell-lewis have you had a chance to look at my comment?

pecigonzalo · 2018-09-20T08:06:27Z

@russell-lewis I have squashed the commits, let me know what else is missing.

pecigonzalo · 2019-02-18T18:09:31Z

@russell-lewis any updates on this?

… request schemas. You can now use bless_lambda_user.lambda_handler_user for user cert requests and bless_lambda_host.lambda_handler_host for host cert requests. Please note that as implemented, anyone who can call the host lambda can obtain host certs for any hostname.

russell-lewis · 2019-05-22T00:21:00Z

@pecigonzalo I'm so sorry about letting this slip through the cracks. I've spent some time this week giving BLESS some TLC. In order to keep the build workflow in line with our other tools I've backed out the pipenv changes. I took the host cert lambda, and split things out a bit more in https://github.com/Netflix/bless/pull/94/files .

pecigonzalo · 2019-05-24T17:07:32Z

@russell-lewis No worries, it was good timing as I was about to do a hard-fork for my use. Thanks a lot for the review/refactor and merge, it looks great!

Ill take it for a spin ASAP.

Move development to pipenv

0b97ba2

pecigonzalo force-pushed the feature/split_host_provider branch 2 times, most recently from e510d52 to d769e5f Compare August 3, 2018 16:39

russell-lewis requested changes Aug 4, 2018

View reviewed changes

pecigonzalo added 2 commits September 20, 2018 10:05

Add host cert issue hanlder

242a586

Add validations for hostnames and tests

ed85a7f

pecigonzalo force-pushed the feature/split_host_provider branch from 2265913 to ed85a7f Compare September 20, 2018 08:06

russell-lewis closed this May 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split lambda handler to support generating Host certs #79

Split lambda handler to support generating Host certs #79

pecigonzalo commented Aug 3, 2018

coveralls commented Aug 3, 2018 •

edited

Loading

pecigonzalo commented Aug 3, 2018

pecigonzalo commented Aug 3, 2018 •

edited

Loading

russell-lewis Aug 3, 2018

russell-lewis Aug 3, 2018

russell-lewis Aug 3, 2018

russell-lewis Aug 3, 2018

russell-lewis Aug 3, 2018

russell-lewis Aug 3, 2018

russell-lewis Aug 4, 2018

pecigonzalo Aug 4, 2018

russell-lewis Aug 4, 2018

russell-lewis commented Aug 4, 2018 •

edited

Loading

pecigonzalo commented Aug 4, 2018

pecigonzalo commented Aug 4, 2018 •

edited

Loading

pecigonzalo commented Aug 17, 2018

pecigonzalo commented Sep 20, 2018

pecigonzalo commented Feb 18, 2019

russell-lewis commented May 22, 2019

pecigonzalo commented May 24, 2019



		def test_basic_local_request():
		output = lambda_handler(VALID_TEST_REQUEST, context=Context,

		invoked_function_arn = 'bogus invoked_function_arn'


		VALID_TEST_REQUEST = {



		class BlessHostSchema(Schema):
		hostnames = fields.Str(required=True)

Split lambda handler to support generating Host certs #79

Split lambda handler to support generating Host certs #79

Conversation

pecigonzalo commented Aug 3, 2018

coveralls commented Aug 3, 2018 • edited Loading

pecigonzalo commented Aug 3, 2018

pecigonzalo commented Aug 3, 2018 • edited Loading

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

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

russell-lewis commented Aug 4, 2018 • edited Loading

pecigonzalo commented Aug 4, 2018

pecigonzalo commented Aug 4, 2018 • edited Loading

pecigonzalo commented Aug 17, 2018

pecigonzalo commented Sep 20, 2018

pecigonzalo commented Feb 18, 2019

russell-lewis commented May 22, 2019

pecigonzalo commented May 24, 2019

coveralls commented Aug 3, 2018 •

edited

Loading

pecigonzalo commented Aug 3, 2018 •

edited

Loading

russell-lewis commented Aug 4, 2018 •

edited

Loading

pecigonzalo commented Aug 4, 2018 •

edited

Loading