feat(aws-lambda) Lambda authentication via EC2 IAM roles #2777

Mgutjahr · 2017-08-04T12:54:26Z

Summary

Adds support for querying temporary credentials from EC2 metadata service instead of fetching long-lived key-pairs from the database.

The credentials are fetched from the EC2 metadata service and are cached for 60 seconds. I assume that many users are running kong on EC2 instances. For these users this is the recommended way to authenticate requests for AWS services as key-rotation and credential provisioning are managed by AWS.

For further details refer to the AWS documentation about EC2 roles

Full changelog

Added kong.plugins.aws-lambda.iam-role-credentials for fetching and caching temporary credentials from the metadata service (default 169.254.169.254 port 80)
Added plugins config flag use_ec2_iam_role to kong.plugins.aws-lambda.schema and declared aws_key and aws_secret as optional
Added self_check function to check whether either use_ec2_iam_role is set to true or both aws_key and aws_secret as configured
Added if branch in kong.plugins.aws-lambda.handler to either fetch credentials from the metadata service or use credentials stored in database

p0pr0ck5

Thank you @Mgutjahr for the PR! Overall I think this will be a very useful addition. A handful of mostly minor style/nitpick comments attached. Additionally, we would like to see tests, both for the schema change, and an integration test for the plugin behavior change as well. Admittedly mocking out something for this will be challenging; perhaps draw on something like #2287 for past discussions of how we've handled tests for this plugin.

p0pr0ck5 · 2017-08-04T18:14:12Z

kong/plugins/aws-lambda/schema.lua

-  }
+  },
+  self_check =  function(schema, plugin_t, dao, is_update)
+    if not plugin_t["use_ec2_iam_role"] or plugin_t["use_ec2_iam_role"] == false then


prefer to use the dot notation to access table members in this case (e.g. plugin_t.use_ec2_iam_role)

p0pr0ck5 · 2017-08-04T18:15:13Z

kong/plugins/aws-lambda/handler.lua

+    local iam_role_credentials = get_credentials_from_iam_role()
+    opts.access_key = iam_role_credentials.access_key
+    opts.secret_key = iam_role_credentials.secret_key
+    opts.headers['X-Amz-Security-Token'] = iam_role_credentials.session_token


very minor style nitpick, prefer to use double quotes instead of single quotes.

p0pr0ck5 · 2017-08-04T18:16:30Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+
+local CACHE_IAM_INSTANCE_CREDS_DURATION = 60 -- seconds to cache credentials from metadata service
+
+local function get_iam_credentials_cache_key()


this could probably be a constant instead of a function

p0pr0ck5 · 2017-08-04T18:17:31Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+
+local function get_iam_credentials_from_instance_profile()
+    local iam_profile_credentials, err = cache.get_or_set(get_iam_credentials_cache_key(), CACHE_IAM_INSTANCE_CREDS_DURATION,
+                                       fetch_iam_credentials_from_metadata_service)


would like to see the fetch_iam_credentials_from_metadata_service call explicitly defining params, instead of relying on the defaults defined in the function itself. this would likely make testing easier.

p0pr0ck5 · 2017-08-04T18:17:58Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+                       iam_role_name .. "' with session token " .. iam_security_token_data['Token'])
+
+    return {
+        access_key = iam_security_token_data['AccessKeyId'],


style: prefer to use dot notation to access these field members

p0pr0ck5 · 2017-08-04T18:36:06Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+                       iam_role_name .. "' with session token " .. iam_security_token_data['Token'])
+
+    return {
+        access_key = iam_security_token_data['AccessKeyId'],


minor style: spacing seems off here, please use two spaces to indent.

p0pr0ck5 · 2017-08-04T20:49:12Z

kong/plugins/aws-lambda/iam-role-credentials.lua

@@ -0,0 +1,91 @@
+local http = require 'resty.http'
+local json = require 'cjson'
+local cache = require "kong.tools.database_cache"


minor style comment: please align assignment operators here

p0pr0ck5 · 2017-08-04T20:49:34Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+    metadata_service_port = metadata_service_port or 80
+
+    local client = http.new()
+    local ok, err = client:connect(metadata_service_host, metadata_service_port) 


style: please align assignment operators here

p0pr0ck5 · 2017-08-04T20:50:06Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+    end 
+
+    if role_name_request_res.status ~= 200 then
+      return nil, "[aws-lambda] Fetching role name from metadata service returned status code ".. role_name_request_res.status  .. 


style: can we try to adjust line breaks and such so as to avoid the line being much longer than 80 chars

p0pr0ck5 · 2017-08-04T20:50:37Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+
+    local iam_security_token_data = json.decode(iam_security_token_request:read_body())
+
+    ngx.log(ngx.INFO, "[aws-lambda] Received temporary IAM credential from metadata service for role '" ..


this also seems like it should be a debug level log entry

Mgutjahr · 2017-08-07T09:21:04Z

Hi @p0pr0ck5,
thanks for the detailed feedback and for pointing out references to possible solutions!

I addressed most of your recommendations and fixed the mentioned style issues. On the testing side I added a tests for the schema extension. In addition, I added an integration test which loads credentials from a mocked-out metadata service (using the same approach aws-lambda uses)

I decided not to add support for configuring the role the plugin should load as one EC2 instance can only have role configured at a time. This can be found from the AWS documentation:

An instance profile can contain only one IAM role. This limit cannot be increased.

This is also how almost all of the available AWS SDK's decided to implement support for instance roles.

Sorry again for the style issues and the missing tests.

p0pr0ck5

@Mgutjahr thanks for the refactors! a lot of these comments are ticky-tack style notes, just good to be consistent, certainly not a huge problem. we appreciate your feedback! at the moment we are in a merge freeze on master as the release of 0.11 stable is closing in (i believe @thibaultcha should have details on that), but barring that i think this is close to being merged. thanks again for the contribution!

p0pr0ck5 · 2017-08-07T22:58:58Z

kong/plugins/aws-lambda/schema.lua

@@ -81,4 +85,14 @@ return {
      func = check_status,
    },
  },
+  self_check =  function(schema, plugin_t, dao, is_update)
+    if not plugin_t.use_ec2_iam_role or plugin_t.use_ec2_iam_role == false then


i believe this second condition is redundant, if not should handle both nil and false

p0pr0ck5 · 2017-08-07T23:03:03Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+    metadata_service_host = metadata_service_host or '169.254.169.254'
+    metadata_service_port = metadata_service_port or 80
+
+    local iam_profile_credentials, err = cache.get_or_set(IAM_CREDENTIALS_CACHE_KEY, CACHE_IAM_INSTANCE_CREDS_DURATION,


A note here, this will require refactoring for the upcoming Kong 0.11, as the cache mechanism has changed and this function call is no longer valid. We should probably have PRs for 0.10.x and 0.11 and merge them simultaneously, with the latter being generated from rebasing this commit in its final form onto next and adjusting as needed. this might need coordination/communication with @thibaultcha prior to releasing a stable 0.11?

p0pr0ck5 · 2017-08-07T23:03:46Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+    end 
+
+    if role_name_request_res.status ~= 200 then
+      return nil, "[aws-lambda] Fetching role name from metadata service returned status code "..


minor style point: need a space between " and ..

p0pr0ck5 · 2017-08-07T23:04:46Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+    end
+
+    if iam_security_token_request.status == 404 then
+        return nil, '[aws-lambda] Unable to request IAM credentials for role' .. iam_role_name ..


minor style point: prefer double quotes " to single quote '

p0pr0ck5 · 2017-08-07T23:04:56Z

kong/plugins/aws-lambda/iam-role-credentials.lua

@@ -0,0 +1,88 @@
+local http  = require 'resty.http'
+local json  = require 'cjson'
+local cache = require 'kong.tools.database_cache'


minor style point: prefer double quotes " to single quote ' for the entries (generally we prefer double quotes everywhere)

p0pr0ck5 · 2017-08-07T23:07:03Z

kong/plugins/aws-lambda/schema.lua

-      required = true,
+    },
+    use_ec2_iam_role = {
+      type="boolean",


style: spacing around assignment operator

p0pr0ck5 · 2017-08-07T23:07:58Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+    return {
+        access_key = iam_security_token_data.AccessKeyId,
+        secret_key = iam_security_token_data.SecretAccessKey,
+        session_token = iam_security_token_data.Token


a few minor style comments:

align assignment operators

append a trailing comma to the end of array elements to minimize future diffs

p0pr0ck5 · 2017-08-07T23:11:35Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+
+    local iam_role_name = role_name_request_res:read_body() 
+
+    ngx.log(ngx.DEBUG, "[aws-lambda] IAM role '" .. iam_role_name ..


here (and a few other places) we can use variadic arguments instead of string concatenation to build the message

p0pr0ck5 · 2017-08-08T21:51:58Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+
+    local iam_security_token_request, err = client:request {
+      method = "GET",
+      path = "/latest/meta-data/iam/security-credentials/" .. iam_role_name


style: trailing comma here for consistency

p0pr0ck5 · 2017-08-08T21:53:56Z

spec/fixtures/custom_nginx.template

+                    AccessKeyId = "test_iam_access_key_id",
+                    SecretAccessKey ="test_iam_secret_access_key",
+                    Token = "test_session_token"
+                 }


minor style: i think we're one space too far indented here?

Mgutjahr · 2017-08-10T18:07:43Z

Hi @p0pr0ck5,
I fixed the requested style issues. Hope it looks better now. Good call on the testing front, we should definitely add a test for a non-existing metadata service as this could be a possible error scenario.

On creating the test I realised that the metadata service call had no connection timeout configured, as this could eat up resources I added a 500ms timeout for the metadata service calls (this should be sufficient as the metadata service runs locally on EC2 instances. Requests usually take ~1ms).

thibaultcha · 2017-08-10T19:33:14Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+
+local function fetch_iam_credentials_from_metadata_service(metadata_service_host, metadata_service_port)
+    local client = http.new()
+    client:set_timeout(500)


It's dangerous to make such assumptions considering Kong might be running in a variety of infrastructures with different limitations or requirements out there. Such timeout variables should ideally be made configurable.

Not quite sure about the validity of this comment, given the very narrow infrastructure scope of the context at play here. Adding a tunable for this seems like unnecessary cognitive load.

Our "mantra" of being opinionated (in order to not oversight any potential use-case or to be compatible with as many infrastructures as possible), hints that we should provide customizable timeouts. In fact there shouldn't be a single timeout (to the best of my knowledge) that isn't configurable in Kong already, including this module itself.

I think the idea here is that inter-infrastructure compatibility is meaningless here. Is it worth sticking to a hard-line mantra in such a case? IMO it's not, but this isn't a hill for me to die on, just chiming in that this is a very specific use case :)

I get your concerns, I tried to omit any unnecessary config properties to keep the plugin configuration relatively clean. Especially as the timeout would be another semi-optional config property (only applies if role based authentication is used). From my experience (using native AWS sdks on a variety of platforms) this is a property you would never need to touch. But maybe there are cases where you have too.

I checked other "official" SDK's from Amazon. Most set a default timeout which cannot be changed.

python cli --> 1 second

python boto --> 1 second (can be configured)

java sdk --> 2 seconds

The timeouts from these SDK's are higher than the one set by me, but most of timeouts mentioned above apply for all services offered by the metadata service (and not just the credentials service).

I'd suggest that I increase the timeout to 5 seconds by default and expose it as a parameter to get_iam_credentials_from_instance_profile(). - This way we can tweak it for the test and exposing it to the plugins config afterwards would be trivial if needed.

If you want me to add it to the config I can do this of course too. Just let me know what you'd prefer.

thibaultcha · 2017-08-10T19:33:25Z

kong/plugins/aws-lambda/iam-role-credentials.lua

+    }
+
+    if not role_name_request_res or role_name_request_res == "" then
+          ngx.log(ngx.ERR, "[aws-lambda] Could not fetch role name from metadata service: ", err)


(Some weird indentation here again)

thibaultcha · 2017-08-10T19:36:20Z

kong/plugins/aws-lambda/schema.lua

    },
    aws_secret = {
      type = "string",
-      required = true,
+    },
+    use_ec2_iam_role = {


Our policy is that no plugin config already in the database should be left in a previous version of the plugin's schema. This is because we want to avoid complicated code paths handling different versions of the same plugin's schema in the plugin business logic itself. This is a policy that has unfortunately not always been followed, and such oversight lead us to a few bugs/overhead maintenance issues recently.

So: new schema value, new migration for Cassandra/PostgreSQL :)

Hi @thibaultcha,
are you referring to the use_ec2_iam_role config property introduced in this PR? - The aws_secret and aws_key were changed to semi-optional, so whatever config was in the database is still correct after the PR (it will continue to use secrets & keys from the db config if use_ec2_iam_role is not set / set to false). In other words, as long as use_ec2_iam_role is not set to true the old schema is still enforced.

Do you mean I should configure use_ec2_iam_role to a required property and set it to false by default through a migration script? I'm unsure of the benefits of this. Wouldn't it be better to remove the default=false flag here?

Hmm indeed... I do agree with that reasoning. We kind of set this "unsaid" policy of enforcing migrations when introducing new values, but optional values would not benefit from it in any way, because they might be nil per definition already.

Alright then, this sounds good! Sorry for the noise!

p0pr0ck5 · 2017-08-23T17:00:08Z

@Mgutjahr can you rebase this commit to fix the conflict with the rockspec? After that I think we're good to go here.

update

Mgutjahr · 2017-09-10T16:58:00Z

Hi @p0pr0ck5 & @thibaultcha ,
sorry for my late reply. I rebased the branch to the current master branch. Note that I had to move the caching of IAM credentials to the handler due to the change of the caching in 0.11.

Mgutjahr · 2017-10-20T11:53:06Z

Hi @thibaultcha, do you have any updates/comments on this PR?

p0pr0ck5 · 2017-12-08T17:10:18Z

@Mgutjahr sorry for the delay on our end. Apologies, since we've had another release since the last change, can we update the rockspec here? I'd like to see this merged into 0.12. @thibaultcha thoughts?

fyntic · 2018-02-20T15:57:48Z

Hello @p0pr0ck5 , @thibaultcha and @Mgutjahr .

Sorry for disturb you, guys, but we really need this feature available in Kong for AWS.
When do you think it might be merged and released?

Thanks in advance.

aloisbarreras · 2018-05-04T13:35:40Z

Is all that needs to happen just rebasing the rockspec? I'm happy to do that real fast if that will help this get merged in.

vvondra · 2018-06-04T15:48:05Z

is it ok if i re-open this fixed?

farazs · 2018-07-16T19:09:41Z

@thibaultcha is all that's left for this to update the rockspec and resolve the handler.lua conflict? seems something trivial and we can merge this.

@Mgutjahr would you be able to do that or someone else can fork this and make the necessary updates. would be really nice to get this merged since it's so close and quite useful.

Mgutjahr · 2018-07-16T19:25:34Z

@farazs I can try to rebase it this weekend. I want to test it at least once also on an actual EC2 on AWS with the current version of kong which is why it is a bit more work than just rebasing it.

tarciosaraiva · 2018-10-01T05:38:25Z

Any chance of this getting reviewed soon?

Adds support for querying temporary credentials from EC2 metadata service instead of fetching long-lived key-pairs from the database. The credentials are fetched from the EC2 metadata service and are cached for 60 seconds. I assume that many users are running kong on EC2 instances. For these users this is the recommended way to authenticate requests for AWS services as key-rotation and credential provisioning are managed by AWS. For further details refer to the [AWS documentation about EC2 roles](http://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2.html)

…ling)

…C2 metadata service instance role

…ptional aws_secret and aws_key config properties

…r log messages

…request termination to handler on metadata service error

… as function params

…cation to be compatible with kong 0.11

Mgutjahr · 2018-10-21T16:17:20Z

Hey, I rebased the PR again to the current master branch. The branch could be merged if you want to. There are just two things I'd like to point out:

The IAM role authentication currently only works if kong is running on an EC2 instance. It would not work if Kong is running on ECS, Fargate or EKS. We can add support for ECS and Fargate but I would not do this in the same PR. Inspiration on how to do this can be found in the various official AWS SDK's out there (e.g. Java or Node ) .. there is unfortunately no real documentation out there from Amazon (at least I could not find it)
This PR is also covered in PR Iam roles with proxy #3639. I'm ok if you close this PR and continue on Iam roles with proxy #3639, the PR mentioned has however additional features & functionality we might want to discuss in more detail. Especially proxying of the metadata service could be a bit risky from a security perspective (as you get IAM credentials through this service and proxying could increase the attack surface for a man in the middle attack)

CLAassistant · 2019-07-01T06:26:32Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Manuel Gutjahr seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

p0pr0ck5 added the pr/status/please review label Aug 4, 2017

p0pr0ck5 previously requested changes Aug 4, 2017

View reviewed changes

p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Aug 4, 2017

p0pr0ck5 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Aug 7, 2017

p0pr0ck5 reviewed Aug 8, 2017

View reviewed changes

p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Aug 8, 2017

thibaultcha requested changes Aug 10, 2017

View reviewed changes

erran-r7 mentioned this pull request Sep 6, 2017

feat(aws-lambda) support using ec2 metadata credentials erran/kong#1

Closed

Mgutjahr force-pushed the feat/aws-lambda-instance-profile-roles branch from 19f2813 to 76184c0 Compare September 10, 2017 16:52

tecnobrat mentioned this pull request Sep 26, 2017

Instance Profiles MrSaints/kong-plugin-aws#2

Open

rkbasa mentioned this pull request Jul 23, 2018

Iam roles with proxy #3639

Closed

Manuel Gutjahr added 3 commits October 21, 2018 13:17

feat(aws-lambda) Fix lua style issues (Quotes, alignments, error-hand…

43d3401

…ling)

feat(awe-lambda) Add iam-role-credentials.lua to rockspec

2ee002d

Manuel Gutjahr added 14 commits October 21, 2018 13:17

feat(aws-lambda) Add integration test for fetching credentials from E…

30df3cb

…C2 metadata service instance role

feat(aws-lambda) Add schema tests for self_check function with semi-o…

809a078

…ptional aws_secret and aws_key config properties

style(aws-lambda) Remove unused variables / Add assertions

0e3f2bf

style(aws-lambda) Add trailing commas, remove string concatenation fo…

1444cf8

…r log messages

refactor(aws-lambda) Add timeout to metadata service calls. Move ngx …

436e980

…request termination to handler on metadata service error

tests(aws-lambda) Add test for non-existing metadata service

3fba98b

style(aws-lambda) replace single quotes with double quotes

36e5352

style(aws-lambda) fix indentation for method call

b8adc14

style(aws-lambda) fix indentations

9cc2559

style(aws-lambda) Reformat test for metadata service

8ae5ae0

refactor(aws-lambda) Expose request timeout and credentials cache_ttl…

155b23c

… as function params

refactor(aws-lambda) Upgrade aws-lambda IAM instance profile authenti…

ecd9f73

…cation to be compatible with kong 0.11

Remove duplicate require statement

90e0ceb

(spec) Adapt port for custom nginx template / mock

70c116f

Mgutjahr force-pushed the feat/aws-lambda-instance-profile-roles branch from 76184c0 to 70c116f Compare October 21, 2018 12:47

Tieske added pr/do not merge and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Mar 15, 2019

guanlan closed this Nov 12, 2020


		local CACHE_IAM_INSTANCE_CREDS_DURATION = 60 -- seconds to cache credentials from metadata service

		local function get_iam_credentials_cache_key()


		local iam_security_token_data = json.decode(iam_security_token_request:read_body())

		ngx.log(ngx.INFO, "[aws-lambda] Received temporary IAM credential from metadata service for role '" ..


		local iam_role_name = role_name_request_res:read_body()

		ngx.log(ngx.DEBUG, "[aws-lambda] IAM role '" .. iam_role_name ..

feat(aws-lambda) Lambda authentication via EC2 IAM roles #2777

feat(aws-lambda) Lambda authentication via EC2 IAM roles #2777

Conversation

Mgutjahr commented Aug 4, 2017

Summary

Full changelog

p0pr0ck5 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

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

Mgutjahr commented Aug 7, 2017

p0pr0ck5 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

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

Mgutjahr commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Aug 10, 2017 • 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

p0pr0ck5 commented Aug 23, 2017

Mgutjahr commented Sep 10, 2017 • edited Loading

Mgutjahr commented Oct 20, 2017

p0pr0ck5 commented Dec 8, 2017

fyntic commented Feb 20, 2018

aloisbarreras commented May 4, 2018

vvondra commented Jun 4, 2018

farazs commented Jul 16, 2018

Mgutjahr commented Jul 16, 2018

tarciosaraiva commented Oct 1, 2018

Mgutjahr commented Oct 21, 2018 • edited Loading

CLAassistant commented Jul 1, 2019 • edited Loading

thibaultcha Aug 10, 2017 •

edited

Loading

Mgutjahr commented Sep 10, 2017 •

edited

Loading

Mgutjahr commented Oct 21, 2018 •

edited

Loading

CLAassistant commented Jul 1, 2019 •

edited

Loading