Improve IAM role statements, policies and principals internal handling and resolution

[medikoo]

Use case description

Goal is to pave path for #4313

Currently Framework creates one IAM role, in which all needed statements for all functions are located.

We need a clear understanding which statements, managed policies and eventual principal registrations are required for which function

Current handling of IAM roles

1. Before configuration of all functions is fully processed, IAM role resource is created at mergeIamTemplates function (assuming there's no provider.role set or all functions having role) where:
   • Policy statements needed for logs groups access are added to it
   • Eventual policy statements from provider.iamRoleStatements are added to it
   • Eventual managed policies from provider.iamManagedPolicies are added to it
   • Managed policies needed for VPC configuration are added to it

It is all happening here:

serverless/lib/plugins/aws/package/lib/mergeIamTemplates.js

Lines 41 to 173 in 4e49473

	// resolve early if provider level role is provided
	if ('role' in this.serverless.service.provider) {
	return BbPromise.resolve();
	}
	// resolve early if all functions contain a custom role
	const customRolesProvided = [];
	this.serverless.service.getAllFunctions().forEach(functionName => {
	const functionObject = this.serverless.service.getFunction(functionName);
	customRolesProvided.push('role' in functionObject);
	});
	if (_.isEqual(_.uniq(customRolesProvided), [true])) {
	return BbPromise.resolve();
	}
	// merge in the iamRoleLambdaTemplate
	const iamRoleLambdaExecutionTemplate = this.serverless.utils.readFileSync(
	path.join(
	this.serverless.config.serverlessPath,
	'plugins',
	'aws',
	'package',
	'lib',
	'iam-role-lambda-execution-template.json'
	)
	);
	iamRoleLambdaExecutionTemplate.Properties.Path = this.provider.naming.getRolePath();
	iamRoleLambdaExecutionTemplate.Properties.RoleName = this.provider.naming.getRoleName();
	if (this.serverless.service.provider.rolePermissionsBoundary) {
	iamRoleLambdaExecutionTemplate.Properties.PermissionsBoundary = this.serverless.service.provider.rolePermissionsBoundary;
	}
	iamRoleLambdaExecutionTemplate.Properties.Policies[0].PolicyName = this.provider.naming.getPolicyName();
	_.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Resources, {
	[this.provider.naming.getRoleLogicalId()]: iamRoleLambdaExecutionTemplate,
	});
	const canonicalFunctionNamePrefix = `${
	this.provider.serverless.service.service
	}-${this.provider.getStage()}`;
	const logGroupsPrefix = this.provider.naming.getLogGroupName(canonicalFunctionNamePrefix);
	const policyDocumentStatements = this.serverless.service.provider.compiledCloudFormationTemplate
	.Resources[this.provider.naming.getRoleLogicalId()].Properties.Policies[0].PolicyDocument
	.Statement;
	let hasOneOrMoreCanonicallyNamedFunctions = false;
	// Ensure policies for functions with custom name resolution
	this.serverless.service.getAllFunctions().forEach(functionName => {
	const { name: resolvedFunctionName } = this.serverless.service.getFunction(functionName);
	if (!resolvedFunctionName || resolvedFunctionName.startsWith(canonicalFunctionNamePrefix)) {
	hasOneOrMoreCanonicallyNamedFunctions = true;
	return;
	}
	const customFunctionNamelogGroupsPrefix = this.provider.naming.getLogGroupName(
	resolvedFunctionName
	);
	policyDocumentStatements[0].Resource.push({
	'Fn::Sub':
	'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
	`:log-group:${customFunctionNamelogGroupsPrefix}:*`,
	});
	policyDocumentStatements[1].Resource.push({
	'Fn::Sub':
	'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
	`:log-group:${customFunctionNamelogGroupsPrefix}:*:*`,
	});
	});
	if (hasOneOrMoreCanonicallyNamedFunctions) {
	// Ensure general policies for functions with default name resolution
	policyDocumentStatements[0].Resource.push({
	'Fn::Sub':
	'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
	`:log-group:${logGroupsPrefix}*:*`,
	});
	policyDocumentStatements[1].Resource.push({
	'Fn::Sub':
	'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
	`:log-group:${logGroupsPrefix}*:*:*`,
	});
	}
	if (this.serverless.service.provider.iamRoleStatements) {
	// add custom iam role statements
	this.serverless.service.provider.compiledCloudFormationTemplate.Resources[
	this.provider.naming.getRoleLogicalId()
	].Properties.Policies[0].PolicyDocument.Statement = policyDocumentStatements.concat(
	this.serverless.service.provider.iamRoleStatements
	);
	}
	if (this.serverless.service.provider.iamManagedPolicies) {
	// add iam managed policies
	const iamManagedPolicies = this.serverless.service.provider.iamManagedPolicies;
	if (iamManagedPolicies.length > 0) {
	this.mergeManagedPolicies(iamManagedPolicies);
	}
	}
	// check if one of the functions contains vpc configuration
	const vpcConfigProvided = [];
	this.serverless.service.getAllFunctions().forEach(functionName => {
	const functionObject = this.serverless.service.getFunction(functionName);
	if ('vpc' in functionObject) {
	vpcConfigProvided.push(true);
	}
	});
	if (vpcConfigProvided.includes(true) || this.serverless.service.provider.vpc) {
	// add managed iam policy to allow ENI management
	this.mergeManagedPolicies([
	{
	'Fn::Join': [
	'',
	[
	'arn:',
	{ Ref: 'AWS::Partition' },
	':iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole',
	],
	],
	},
	]);
	}
	return BbPromise.resolve();

2. In next turn, events are compiled, and then eventual extra statements, polices are added and principals are registered. This is achieved by retrieving IamRoleLambdaExecution resource from serverless.service.provider.compiledCloudFormationTemplate.Resource and adding needed configuration on pre-created object literally, as e.g. it's done here:

   serverless/lib/plugins/aws/package/compile/events/msk/index.js

   Lines 115 to 124 in 4e49473

   	if (cfTemplate.Resources.IamRoleLambdaExecution) {
   	const statement =
   	cfTemplate.Resources.IamRoleLambdaExecution.Properties.Policies[0].PolicyDocument
   	.Statement;
   	if (mskStatement.Resource.length) {
   	statement.push(mskStatement);
   	statement.push(ec2Statement);
   	}
   	}
   	});

Proposed solution

Ideally would be to refactor above into following:

1. At mergeIamTemplates:
   • Create IAM role resources , but with no statements, policies (setup collections, but leave them empty) - we still need to create it here, to not break things for plugins.
   • Create awsProvider.iamConfig and functions[].iamConfig, where each iamConfig is object as follows:

{
  principals: new Set(),
  policyStatements: [],
  managedPolicies: []
};

• Add eventual policy statements from provider.iamRoleStatements to awsProvider.iamConfig.policyStatements
• Add eventual managed policies from provider.iamManagedPolicies to awsProvider.iamConfig.managedPolicies
• If provider.vpc add VPC related polices to awsProvider.iamConfig.managedPolicies

2. In any place where we currently extend IamRoleLambdaExecution directly (look for getRoleLogicalId() and IamRoleLambdaExecution references) Refactor the code, to not extend IAM role resource directly, but instead add related policy statements, managed policies, principals to corresponding function's iamConfig (as those cases will be about specific function event configurations)

3. In lib/plugins/aws/package/compile/functions/index.js:

   • Add related log group policy statements to function's iamConfig.policyStatements (at this point reflect exactly statements as were added at mergeIamTemplates function)
   • If function has vpc configuration add VPC related policies to its iamConfig.managedPolicies

4. Create resolveIamRoles.js in lib/plugins/aws/package/lib which should export a method to be assigned at

   serverless/lib/plugins/aws/package/index.js

   Lines 23 to 31 in 4e49473

   	Object.assign(
   	this,
   	generateCoreTemplate,
   	mergeIamTemplates,
   	generateArtifactDirectoryName,
   	mergeCustomProviderResources,
   	saveServiceState,
   	saveCompiledTemplate
   	);

   invoked at

   serverless/lib/plugins/aws/package/index.js

   Line 69 in 4e49473

   BbPromise.bind(this).then(() =>

   and which should:
   • Register all awProvider.iamConfig.principals and functions[].iamConfig.principals on IamRoleLambdaExecution resource (ensuring no duplicates)
   • Add all awsProvider.iamConfig.managedPolicies and functions[].iamConfig.managedPolicies to IamRoleLambdaExecution resource (ensuring no duplicates)
   • Add all awsProvider.iamConfig.policyStatements and functions[].iamConfig.managedPolicies to IamRoleLambdaExecution resource (ensuring no duplicates)

Testing

This change will likely break a lot of tests, why at the same time we should end with same result CF template.

This signals that again our tests are wrong, and we should not put any effort in updating them in current shape.

Instead, any test file that will host broken tests should be refactored to be based on runServerless (https://github.com/serverless/serverless/tree/master/test#unit-tests), and PR's that refactor each file should be proposed separate (different PR per each test file).

Having those tests migrated to runServerless should keep same tests passing after given refactor is made.

[rzaldana]

Hi @medikoo . I can have a go at this

[medikoo]

@zaldanaraul great to hear that! Go for it.

I believe (as mentioned in description) it'll break some test files, and refactoring them can be challenging. I suggest once you have the initial implementation working, you post the list of test files that break, and we will open another dedicated issue to address their refactor (this will likely result with other members of community helping us).

[rzaldana]

Hi @medikoo . I've been working on this issue but I have a couple questions before I submit a first draft:

1. for step 3 of the proposed solution, I'm not sure where inside of lib/plugins/aws/package/compile/functions/index.js I should add the log group policy statements to the function's iamConfig object. My guess is somewhere inside the compileFunction() function but I wanted to confirm this.

2. for step 4 of the proposed solution, you say the resolveIamRoles method should be invoked at
   

   serverless/lib/plugins/aws/package/index.js

   Line 69 in 4e49473

   BbPromise.bind(this).then(() =>

Do you mean just adding the line like this:

      'package:finalize': () =>
        BbPromise.bind(this).then(() =>
          this.serverless.pluginManager.spawn('aws:package:finalize');
	  this.resolveIamRoles(); // adding line here
        ),

or is there something I'm missing? On that note, I'm not too familiar with how the lifecycle manager works with events and hooks so if there is some more specific doc that talks about that I'd be grateful!

3. For dealing with principals, I did some research and it looks like we cannot add a Principal in an IAM identity-based policy (which includes IAM role policies). (source). We would only really need a principal to define the trust policy that allows the Lambda service to assume the role we're defining so I think there is no need for a Principals set in the iamConfig objects.

Waiting for your input on this!

[medikoo]

@zaldanaraul sorry for late response! Great thanks for questions see my answers

I'm not sure where inside of lib/plugins/aws/package/compile/functions/index.js I should add the log group policy statements to the function's iamConfig object. My guess is somewhere inside the compileFunction() function but I wanted to confirm this.

Yes, exactly.

for step 4 of the proposed solution, you say the resolveIamRoles method should be invoked at... Do you mean just adding the line like this:

Yes, however I think we need to populate IAM role before 'aws:package:finalize' is spawned, so order should be:

this.resolveIamRoles();
return this.serverless.pluginManager.spawn('aws:package:finalize');

I did some research and it looks like we cannot add a Principal in an IAM identity-based policy (which includes IAM role policies)

Currently we're adding principals to AssumeRolePolicyDocument IAM role property e.g. in case of CloudFront event here:

serverless/lib/plugins/aws/package/compile/events/cloudFront/index.js

Line 598 in 989bd66

lambdaAssumeStatement.Principal.Service.push('edgelambda.amazonaws.com');

so we need some property in which we can gather principals that need to land there (probably principals property name could be made more specific for that ?)

[issea1015]

(cc: @zaldanaraul) Hello @medikoo 👋 - The description sounds straightforward. Could you let me in?

[medikoo]

@issea1015 Sure, that's a great task to cover, many users are awaiting this

[issea1015]

In the PR, we've found and listed broken test files after this change:

• test/unit/lib/plugins/aws/package/compile/events/cloudFront.test.js
  • *already issued: Tests: Refactor lib/plugins/aws/package/compile/events/cloudFront/index.test.js #8528
• test/unit/lib/plugins/aws/package/compile/events/sqs.test.js
• test/unit/lib/plugins/aws/package/compile/events/stream.test.js
• test/unit/lib/plugins/aws/package/compile/events/websockets/lib/api.test.js
• test/unit/lib/plugins/aws/package/compile/functions.test.js
  • already issued: Tests: Refactor lib/plugins/aws/package/compile/functions/index.test.js #8523

@medikoo It'd be appreciated if you can make issues for each of them, just like #8523

[medikoo]

Great thanks @issea1015 for gathering the list!

test/unit/lib/plugins/aws/package/compile/events/cloudFront.test.js

Has dedicated at issue at #8528

I'll try to create issues for others this week. I'll list them here, once done

[medikoo]

@issea1015 sorry for delay, but after all I've created a refactor map for test files which we should refactor.

I've created corresponding issues:

• Tests: Refactor test/unit/lib/plugins/aws/package/compile/events/sqs.test.js #10329
• Tests: Refactor test/unit/lib/plugins/aws/package/compile/events/stream.test.js #10330
• Tests: Refactor test/unit/lib/plugins/aws/package/compile/events/websockets/lib/api.test.js #10331

[issea1015]

@medikoo I'm glad to hear that! Thanks for cleaning them up. We got one more step closer to the end.

[theashishbhatt]

issea1015, are you working on this?