RFC502: L2 Constructs for vpclattice

[mrpackethead]

This is a request for comments about L2 Constructs for vpclattice . See #502 for
additional details.

APIs are signed off by @TheRealAmazonKendra

---

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

[mrpackethead]

@TheRealAmazonKendra... Can you have a look at this please.

[TheRealAmazonKendra]

Overall this is a good start but it's missing the core contract for each of these constructs. Please add in the proposed props for each construct.

[TheRealAmazonKendra]

This example doesn't seem to be formatted quite right. Can you take another look at this?

[mrpackethead]

New example provided, hopefuly the formating is correct

[TheRealAmazonKendra]

I'm not seeing what the proposed props are for any of the Constructs below so it's hard to understand what the user experience will be here. I like the convenience functions, but what is the contract if a user wants to define the various sets of functionality as props when they declare the construct initially?

Pull request has been modified.

[mrpackethead]

Overall this is a good start but it's missing the core contract for each of these constructs. Please add in the proposed props for each construct.

I've provided the Proposed Props, and interfaces for each Construct now, hopefully this is what is needed.

[TheRealAmazonKendra]

This is very much moving in the right direction! Please see my comments inline.

[TheRealAmazonKendra]

What other types are included in AuthType?

[mrpackethead]

export enum AuthType {
  NONE = 'NONE',
  AWS_IAM = 'AWS_IAM'
}

Its probably that in time, the service will extend to have other methods. Today its AWS_IAM or nothing.

[TheRealAmazonKendra]

Thanks for this context. If more options are likely to added, I'd prefer to see this as an enum like class instead of an actual enum so that it's more extensible if options or props need to be added to potential auth types.

[mrpackethead]

@TheRealAmazonKendra @corymhall We need to pick up on this item that Kendra picked up a month ago. I will need to have an offline chat with you about this.

Pull request has been modified.

[mrpackethead]

@TheRealAmazonKendra , hopefully addressed your questions.

[TheRealAmazonKendra]

Two very small comments here. I'm going to ask for a second set of eyes on this from my team just in case I'm missing something but I'm feeling quite good about this.

[TheRealAmazonKendra]

This was an enum elsewhere, which I assume was also the intention here? Could you please update this to make it consistent?

[mrpackethead]

Good Spotting. that authType should be refering to the enum as well.

[TheRealAmazonKendra]

Thanks for this context. If more options are likely to added, I'd prefer to see this as an enum like class instead of an actual enum so that it's more extensible if options or props need to be added to potential auth types.

Pull request has been modified.

[corymhall]

Can you also add a section explaining how the network access is configured? When you associate a VPC it looks like you can configure security groups and ACLs, but I'm not super clear on how that will work here.

[corymhall]

We shouldn't expose any L1 properties on an L2.

[mrpackethead]

yes, agreed, just need to take a r53.IHostedZone,

[mrpackethead]

Addtionallky this does not need to be included in the interface

[corymhall]

We shouldn't expose any L1 properties on an L2.

[mrpackethead]

replaced with r53.IHostedZone

[corymhall]

We shouldn't expose any L1 properties on an L2.

[mrpackethead]

Yes, have created an interface DefaultListenerAction, already had all the bits required.

/**
 * A default listener action.
 * one of fixed response or forward needs to be provided.
 */
export interface DefaultListenerAction {
  /**
   * Provide a fixed Response
   * @default none
   */
  fixedResponse?: FixedResponse;
  /**
   * Forward to a target group
   * @default none
   */
  forward?: WeightedTargetGroup;
}

[corymhall]

I think for this we should mimic some of the other libraries that follow this patter (i.e. elbv2)

It also looks like we might want to allow users to grantAccess to the target in addition to the service.

const target = myListener.addTargets('helloworld', {
  priority: 10,
  targets: [new targets.LambdaTarget(support.helloWorld)],
  path: '/hello',
  ...
});
target.grantAccess(anotherService);

[mrpackethead]

'targets' in lattice are targets of the 'lattice service' not targets for the users. (this is the lattice language too, which overlaps with the lanugage of Load balancers). Theres another level of abstraction here as well..

Listeners have rules.. rules have targets.

Adding a target to a listener doe'snt make sense.

[corymhall]

But from the users perspective why do I need to know that? I just want to add a listener and say send requests that match this to this target. I don't really care that underneath that there are rules, etc.

[mrpackethead]

You absolutely do need to care that there are rules. There is no mapping between targets and listeners. Its what the Rule does. A listener has multiple rules, which map to targets. They have prioritys and a default.

If you dont' provide the ability to map rules, you cant' create the the service.

[corymhall]

This is how I'm thinking about this, but let me know if I'm completely misunderstanding something.

Say I have a color service that where a user makes a request to the service and gets back the color.

green -> https://myapi.com/green (lambda function)
blue -> https://myapi.com/blue (ECS service)
yellow -> https://myapi.com/yellow (EC2 Instance)

I would create the service and add a listener.

const service = new Service(this, 'ColorService');

const listener = service.addListener();

Now I want to add the rules that say when a user requests https://myapi.com/green the request is routed to the Lambda function I've created.

I could do that explicitly

listener.addListenerRule({
  path: '/green',
  actions: [
    { target: new LambdaTarget(...) }
  ],
});

Or I could I could say allow it to create the rule implicitly

listener.addTarget({
  path: '/green',
  target: new LambdaTarget(...),
});

The reason I am suggesting going this route is that we already have this as the established pattern for Load Balancers which has a very similar experience.

For example the CloudFormation for an ELB setup would be

ListenerRule2:
    Type: 'AWS::ElasticLoadBalancingV2::ListenerRule'
    Properties:
      Actions:
        - Type: forward
          TargetGroupArn: !Ref TargetGroup2
          Weight: 10
      Conditions:
        - Field: http-header
          HttpHeaderConfig:
            HttpHeaderName: User-Agent
            Values:
              - Chrome
      ListenerArn: !Ref Listener
      Priority: 2

And for the VPC Lattice

Type: AWS::VpcLattice::Rule
Properties: 
  Action: 
    Forward:
      TargetGroups:
        - Weight: 10
          TargetGroupIdentifier: !Ref TargetGroup2
  ListenerIdentifier: !Ref Listener
  Match: 
   HttpMatch:
    HeaderMatch:
      - Name: 'User-Agent'
        HeaderMatchType:
          Contains: Chrome
  Name: String
  Priority: 2
  ServiceIdentifier: !Ref Service
  Tags: 
    - Tag

It is slightly different, but the overall intent is the same. Does this make sense?

[mrpackethead]

I see why you think this now. Its an established pattern, but even with Loadbalances your not adding a target.. your adding a rule. Its a pattern that probaby is too hard to change. I remember actually finding this quite confusing when i first went to use the LB constructs as they didtn use the same 'naming' and conventions as the service and service documentation. ( https://docs.aws.amazon.com/vpc-lattice/latest/ug/listener-rules.html )

THeres a whole section about adding a rule to the listener.

So, this seems to break the design and contributions guides. However that been broken by the LB, and if it a pattern, then what its called is largely unimportant to how it works. I'd argue it makes it less understandable..

Please note that this methods' propertie included the principals. That is what get used to create the auth policy on the service.... THis saves the user from having to manually create what becomes a complex set of iam policy.

[corymhall]

Suggested change

	addService(props: AddServiceProps): void;
	addService(service: IService, props?: AddServiceProps): void;

[mrpackethead]

Yes, That is a better approach. Other than the service, theres no other properties anyway..

this can just be addService(service: IService)

[corymhall]

Suggested change

	public static fromId(scope: constructs.Construct, id: string, serviceId: string): IService {
	public static fromServiceId(scope: constructs.Construct, id: string, serviceId: string): IService {

[corymhall]

Same question as above, is there ever a reason to not apply the auth policy?

[mrpackethead]

same answer as above.

[mrpackethead]

same fix as above.

[corymhall]

I think we need to follow the integrations pattern instead.

[mrpackethead]

This one took me a little bit thinking and learning about how this works. But I've got there.
The target types, alb, ec2, ip and lambda will get created in a seperate module 'aws-vpclattice-targets'.. they will import 'ITarget' from this module, and implement it. Now i've written the code, i get it..

[mrpackethead]

Can you also add a section explaining how the network access is configured? When you associate a VPC it looks like you can configure security groups and ACLs, but I'm not super clear on how that will work here.

When you 'associate' a vpc, the service does a couple of thigns;

(1) It adds a service interface into your vpc. (note its using a linklocal 169.254 address )
(2) it associates a dns zone with your vpc, which is how resources in the vpc are able to resolve the service interface.
(3) Optionally as you point out, you can apply a SG on the interface to limit what can connect to it..

Theres not much 'configurable' about (1) and (2). That happens and you have no choice.

on (3) there is an optional prop securityGroups in the AssociateVPC class.. Pass it an array of securityGroups and it will apply them.

How its configured,and operates is really a product document thing, more than an RFC requirment i would have thought?

Pull request has been modified.

[TheRealAmazonKendra]

Looks like this just needs a couple formatting fixes but I'm putting it into final commenting and approving the api.

[mrpackethead]

Minor thing. Not sure who needs to update the main table?

[mrpackethead]

@TheRealAmazonKendra @evgenyka @corymhall This is now passed the ~week for final comments, Is there anything else holding this up from progressing?

[TheRealAmazonKendra]

We shouldn't be making changes to our formatting requirements. Please remove the markdownlint file.

[mrpackethead]

We shouldn't be making changes to our formatting requirements. Please remove the markdownlint file.

i agree. And i did'nt really want to do this. I coud'nt find the lint file that matched the projects reuqirements. I've finally found it the tools. Its not immediately obvious.. vscode did'tn find it.. This has been quite painful, the MD09 and 13 errors are because those are not enabled by default for some reason...

right now, if i remove it, markdown linting will break, and then theres a good chance it won't pass the pipeline.. There's an opporutnity for improvement on this one, so i'll document it, so it doe'snt trip up anyone else.

Pull request has been modified.

Pull request has been modified.

[mrpackethead]

We are missing a .grantInvoke() method on a Lattice Service. This became apparent during creating Integ Tests..

[mohakkohli]

Here's the comments that we think we should address both from security and usability perspective.

[mohakkohli]

Service Construct

allowUnauthenticatedAccess → requireAuthentication

This setting could be the on/off switch for AuthN on the service

I would invert this setting to requireAuthentication just because if you set this option you would expect we’re running the AuthN step - and unauthenticated would no longer be possible. If you had set this setting, then any SIGv4 caller could call - unless restrictions were added with the above methods

allowExternalPrincipals → onlyOrganizationIds & onlyAccountIds

This setting could add conditions to AuthZ policy
We could strengthen what control can be expressed here by having this be a list of allowed Organization Ids rather than a true/false.

Setting this to false will trigger the CDK to call the SDK to get the current users Organization Id, that’s not always the correct Organization Id - which Organization someone would want to restrict things to can depend on how they’re structuring their AWS resources. There could even be multiple organizations which are in use.

The word External in the name of the option could be a little overloaded - so could rename to something like onlyOrganizationIds which will be less ambiguous. If unset or the empty list, we’d apply no organization restriction, so that’s why I suggested only vs allow as a prefix.

Along the same lines onlyAccountIds could be a useful complementary function, that takes a list of account ids to restrict access to. This will also help prevent the caller from being surprised they need to have permission to describe organizations.

(The semantics are a bit different from RAM because we have Anonymous identities)

allowAnonymousPrincipals → true / false (default false)

This setting can add conditions to allow or explicitly DENY Anonymous principal.

If false, then an explicit DENY AuthZ policy should be added to the Service which prevents the principal named “ANONYMOUS”. This will ensure AuthN is not skipped by sending an Authorization: ANONYMOUS header - even when the service owner has used requireAuthentication

This prefix I kept as Allow because this is an option - callers can still be authenticated, but they can also be ANONYMOUS when this is set.

Service Network Construct

This construct should have security methods and semantics that are identical to what we discuss above for the Service Construct.

There is a bug on line 240 -
https://github.com/raindancers/aws-cdk/blob/mrpackethead/aws-vpclattice-alpha/packages/%40aws-cdk/aws-vpclattice-alpha/lib/servicenetwork.ts#L240

const statement = new iam.PolicyStatement({
  effect: iam.Effect.ALLOW,
  actions: ['vpc-lattice-svcs:Invoke'],
  resources: ['*'],
  principals: [new iam.AnyPrincipal()],
});

Don’t add this implicit ALLOW * permission - it will allow all Orgs and it will allow the Anonymous principal to call by default. It is better to have a default DENY. Instead make caller explicitly tell us the permissions - that’s important for the Policy configuration so that there are no surprises.

For the share() method , would use only the principals field - you can put into that field the exact lists of principals and org ids that we would have collected above. So no need to set allowExternalPrincipals in the resource share.

https://github.com/raindancers/aws-cdk/blob/mrpackethead/aws-vpclattice-alpha/packages/%40aws-cdk/aws-vpclattice-alpha/lib/servicenetwork.ts#L345C1-L353C1

  public share(props: ShareServiceNetworkProps): void {
    new ram.CfnResourceShare(this, 'ServiceNetworkShare', {
      name: props.name,
      resourceArns: [this.serviceNetworkArn],
      allowExternalPrincipals: props.allowExternalPrincipals,
      principals: props.principals,
    });
  }

Similar comment for shareToAccounts() on service:

https://github.com/raindancers/aws-cdk/blob/mrpackethead/aws-vpclattice-alpha/packages/%40aws-cdk/aws-vpclattice-alpha/lib/service.ts#L299

Listener Construct

The listener construct has the ability to set Policy, but it can only set it to the rules that were set on the service.

I would remove this - the auth rules getting generated are granular - but they don’t produce an AuthZ result that is different from what you have achieved setting policy at the Service or Service Network level on Resource ‘*’. You’d need to have even more options to vary some of the policy statements from the API structure to achieve a different AuthZ result.

This could also lead to large policies and push against the upper bound for individual policies, since we auto add new large statements with each sort of match.

Listeners who need to have fine grained auto at the level of route should provide a full policy statement, rather than our trying to generate from the shape of their API. This will help users make explicit decisions about security, rather than our generating default rules for them - and it will be easier for them to manage these rules since they’d be thier own explicit artifact.

Associate VPC Construct

https://github.com/raindancers/aws-cdk/blob/mrpackethead/aws-vpclattice-alpha/packages/%40aws-cdk/aws-vpclattice-alpha/lib/servicenetwork.ts#L391C1-L394C9

Would make the port configurable - this only allows ingress via 443, but that may not be the port in use & protocol may not be HTTPS.

  securityGroup.addIngressRule(
    ec2.Peer.ipv4(props.vpc.vpcCidrBlock),
    ec2.Port.tcp(443),
  );

I’m not so sure I would try to create the SecurityGroup on behalf of customers at all - that’s a resource we’re taking as an input to Lattice - there’s lots of options and considerations when it comes to how users would use these.

Would the VPC L2 construct cover this sort of convenience?

HTTP Method Enum

Could include all the VERBs here like OPTIONS, PATCH, TRACE, ...
https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods

[mrpackethead]

Service Construct

allowUnauthenticatedAccess → requireAuthentication

This setting could be the on/off switch for AuthN on the service

I would invert this setting to requireAuthentication just because if you set this option you would expect we’re running the AuthN step - and unauthenticated would no longer be possible. If you had set this setting, then any SIGv4 caller could call - unless restrictions were added with the above methods

I would by default always pick the more 'secure' default. So you have to opt out of security. Its a trivial change to invert the logic, and I'm ok with it, it really does'tn change the functionality.

allowExternalPrincipals → onlyOrganizationIds & onlyAccountIds

This setting could add conditions to AuthZ policy We could strengthen what control can be expressed here by having this be a list of allowed Organization Ids rather than a true/false.

The grantAccess() method on the Network Class, provides the mechanism to add a list of principals, but its not setting a condition in an iam policy i'm going to have to go and look for an example if there is a grant like method for conditons. I dont' remember seeing one.
The idea of being able to provide a list of principals to create the condition is a good one. I' Need to think about how we should implement it in a cdk-esqe way.
``

Setting this to false will trigger the CDK to call the SDK to get the current users Organization Id, that’s not always the correct Organization Id - which Organization someone would want to restrict things to can depend on how they’re structuring their AWS resources. There could even be multiple organizations which are in use.

That was the intent. ( and its currently the default ). if you do want to allow externals, I think you shoudl be setting this to true, so you've explicity allowed it. THat would result in NO condition be added to the policy.

The word External in the name of the option could be a little overloaded - so could rename to something like onlyOrganizationIds which will be less ambiguous. If unset or the empty list, we’d apply no organization restriction, so that’s why I suggested only vs allow as a prefix.

Maybe 'External' is overloaded. but onlyOnlyOrganizationId is confusing. The intent here is to add a conditon that is restricted to the org that the account is deployed to.

Along the same lines onlyAccountIds could be a useful complementary function, that takes a list of account ids to restrict access to. This will also help prevent the caller from being surprised they need to have permission to describe organizations.

I'll go and find what the 'grant' like CDK method for a condition is.

(The semantics are a bit different from RAM because we have Anonymous identities)

allowAnonymousPrincipals → true / false (default false)

This setting can add conditions to allow or explicitly DENY Anonymous principal.

If false, then an explicit DENY AuthZ policy should be added to the Service which prevents the principal named “ANONYMOUS”. This will ensure AuthN is not skipped by sending an Authorization: ANONYMOUS header - even when the service owner has used requireAuthentication

This prefix I kept as Allow because this is an option - callers can still be authenticated, but they can also be ANONYMOUS when this is set.

Service Network Construct

This construct should have security methods and semantics that are identical to what we discuss above for the Service Construct.

There is a bug on line 240 - https://github.com/raindancers/aws-cdk/blob/mrpackethead/aws-vpclattice-alpha/packages/%40aws-cdk/aws-vpclattice-alpha/lib/servicenetwork.ts#L240

const statement = new iam.PolicyStatement({
  effect: iam.Effect.ALLOW,
  actions: ['vpc-lattice-svcs:Invoke'],
  resources: ['*'],
  principals: [new iam.AnyPrincipal()],
});

Don’t add this implicit ALLOW * permission - it will allow all Orgs and it will allow the Anonymous principal to call by default. It is better to have a default DENY. Instead make caller explicitly tell us the permissions - that’s important for the Policy configuration so that there are no surprises.

For the share() method , would use only the principals field - you can put into that field the exact lists of principals and org ids that we would have collected above. So no need to set allowExternalPrincipals in the resource share.

https://github.com/raindancers/aws-cdk/blob/mrpackethead/aws-vpclattice-alpha/packages/%40aws-cdk/aws-vpclattice-alpha/lib/servicenetwork.ts#L345C1-L353C1

  public share(props: ShareServiceNetworkProps): void {
    new ram.CfnResourceShare(this, 'ServiceNetworkShare', {
      name: props.name,
      resourceArns: [this.serviceNetworkArn],
      allowExternalPrincipals: props.allowExternalPrincipals,
      principals: props.principals,
    });
  }

Similar comment for shareToAccounts() on service:

https://github.com/raindancers/aws-cdk/blob/mrpackethead/aws-vpclattice-alpha/packages/%40aws-cdk/aws-vpclattice-alpha/lib/service.ts#L299

Listener Construct

The listener construct has the ability to set Policy, but it can only set it to the rules that were set on the service.

The principals in the listener Rule are optional. If you dont' supply them, it wont' add statemetns to the policy.. However if you do, it will.

You have the ability to add what ever policy statements you like with the addStatementToPolicy() method.

For the implmenetations i've done, this has been a very easy and quick way to get something working. As written its provided the ability to have an 'easy' way. and you've got a way to effectively hand build whatever you like..

I would remove this - the auth rules getting generated are granular - but they don’t produce an >AuthZ result that is different from what you have achieved setting policy at the Service or >Service Network level on Resource ‘*’.

Having best of both worlds here. is a good thing. CDK is about abstracting the complexity away for the 80%, and providing options for the 20% who dare to do things the hard way.

ou’d need to have even more options to vary some of the policy statements from the API structure ?to achieve a different AuthZ result.

Which we have.

This could also lead to large policies and push against the upper bound for individual policies, since we auto add new large statements with each sort of match

We can check for policy size at synthtime. What is the maximum policy size permitted?
.

Listeners who need to have fine grained auto at the level of route should provide a full policy statement, rather than our trying to generate from the shape of their API. This will help users make explicit decisions about security, rather than our generating default rules for them - and it will be easier for them to manage these rules since they’d be thier own explicit artifact.

Which you can. Again, this is the point of CDK. Abstractions to make it simple. If you want to have that degree of constrol, nothing stops you here, at the L2 Contruct, or even you can drop down to the L1 Cfn types.

Associate VPC Construct

https://github.com/raindancers/aws-cdk/blob/mrpackethead/aws-vpclattice-alpha/packages/%40aws-cdk/aws-vpclattice-alpha/lib/servicenetwork.ts#L391C1-L394C9

Would make the port configurable - this only allows ingress via 443, but that may not be the port in use & protocol may not be HTTPS.

  securityGroup.addIngressRule(
    ec2.Peer.ipv4(props.vpc.vpcCidrBlock),
    ec2.Port.tcp(443),
  );

This only gets added if a security Group if one is not supplied. As writen the props for AssociateVPC take a security group.

Options:
(a) Default to tcp/443 if you dont' supply one.
(b) Default to not providing a security group, ( this seems very bad idea ) if you dont' provide one
(c) make the security group a required prop.

Its just my experience here, but Id have thought that MOST people will be using tcp 443, so thats what you default to, and leave the option for someone to provide one.

We are also exposing securityGroups as an attribute, so theres another avenue open for people.

I’m not so sure I would try to create the SecurityGroup on behalf of customers at all - that’s >a resource we’re taking as an input to Lattice - there’s lots of options and considerations when >it comes to how users would use these.

If you were doing this in CF, that woudl be the approach.. CDK is all about making it easy.

Would the VPC L2 construct cover this sort of convenience?

yes. there are plenty of CDK examples of this.

HTTP Method Enum

Could include all the VERBs here like OPTIONS, PATCH, TRACE, ... https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods

Yes, that is definatly a good idea.

[mergify]

Thank you for contributing! Your pull request is now being automatically merged.

[MrArnoldPalmer]

@mrpackethead there is a conflict in the readme that needs to be resolved before this will auto-merge. The status table of the RFCs in the README is auto-generated, which we should make more clear in the docs 😄

I don't have perms to push to your fork so you'll have to do it.

Pull request has been modified.

[mrpackethead]

@MrArnoldPalmer, i think i've fixed it..

[mergify]

Thank you for contributing! Your pull request is now being automatically merged.