Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a ClientVPN connection handler request/response definition #343

Merged
merged 4 commits into from Dec 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions events/README.md
Expand Up @@ -12,6 +12,8 @@ This package provides input types for Lambda functions that process AWS events.

[AppSync](README_AppSync.md)

[ClientVPN Connection Handler](README_ClientVPN.md)

[CloudFormation Events](../cfn/README.md)

[CloudWatch Events](README_CloudWatch_Events.md)
Expand Down
56 changes: 56 additions & 0 deletions events/README_ClientVPN.md
@@ -0,0 +1,56 @@
# Sample Function

The following is a sample Lambda function that receives a Client VPN connection handler request as an input and then validates the IP address input and checks whether the connection source IP is on the allowed list defined as a map inside the function. If the source IP matches an allowed IP address it allows the access, otherwise an error message is presented to the user. Debug logs are generated to CloudWatch Logs. (Note that by default anything written to Console will be logged as CloudWatch Logs events.)
gpoul marked this conversation as resolved.
Show resolved Hide resolved

```go
import (
"fmt"
"log"
"net"

"encoding/json"

"github.com/aws/aws-lambda-go/events"
"github.com/aws/aws-lambda-go/lambda"
)

var (
AllowedIPs = map[string]bool{
"10.11.12.13": true,
}
)

func handler(request events.ClientVPNConnectionHandlerRequest) (events.ClientVPNConnectionHandlerResponse, error) {
requestJson, _ := json.MarshalIndent(request, "", " ")
log.Printf("REQUEST: %s", requestJson)

sourceIP := request.PublicIP
if net.ParseIP(sourceIP) == nil {
return events.ClientVPNConnectionHandlerResponse{}, fmt.Errorf("Invalid parameter PublicIP passed in request: %q", sourceIP)
}

log.Printf("SOURCE IP: %q", sourceIP)

if allowed, ok := AllowedIPs[sourceIP]; ok && allowed {
log.Printf("Allowing access from: %q", sourceIP)
return events.ClientVPNConnectionHandlerResponse{
Allow: true,
ErrorMsgOnFailedPostureCompliance: "",
PostureComplianceStatuses: []string{},
Comment on lines +38 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

small nitpick: do you need to set either of these? The nil value for a string is "" and it might be ok for PostureComplianceStatuses to be nil? I only ask because omitting them might be cleaner.

[edit] https://docs.aws.amazon.com/vpn/latest/clientvpn-admin/connection-authorization.html suggests these two fields are "required" so I definitely wouldn't omitempty them but it might still be ok for them to be "" and nil (their zero values).

Copy link
Contributor Author

@gpoul gpoul Dec 17, 2020

Choose a reason for hiding this comment

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

I've written a test like this to see how I can make this happen:

func TestClientVPNConnectionHandlerResponseConstructorValidation(t *testing.T) {
        // read json from file
        inputJSON, err := ioutil.ReadFile("./testdata/clientvpn-connectionhandler-response.json")
        if err != nil {
                t.Errorf("could not open test file. details: %v", err)
        }

        // construct response
        outputEvent := ClientVPNConnectionHandlerResponse{
                Allow: true,
                SchemaVersion: "v1",
        }

        // serialize to json
        outputJSON, err := json.Marshal(outputEvent)
        if err != nil {
                t.Errorf("could not marshal event. details: %v", err)
        }

        assert.JSONEq(t, string(inputJSON), string(outputJSON))
}

for the string value that is easy; I can just drop it and it will be "", but the array is not so easy to get marshalled as {}; if I understand golang/go#37711 correctly the choice is either a func NewClientVPNConnectionHandlerResponse or a custom marshaller that handles this; both feel like they depart from how the rest of the library is used a bit, although there seems to be functions for DynamoDB attributes as well.

Anything you'd suggest? I somehow feel it's easier to keep this in the sample and hopefully people will use the sample to then end up with the right data structure? The alternative that feels clean as well is the function that creates an "empty" object as it could also create the SchemaVersion as well and we can drop the version from the sample code; that would gain something at least.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

 ClientVPNConnectionHandlerResponse{
                Allow: true,
                SchemaVersion: "v1",
        }

should marshal as:

{
	"allow": true,
	"error-msg-on-failed-posture-compliance": "",
	"posture-compliance-statuses": null,
        "schema-version": "v1",
}

The question is: is this ok? Or does it cause failures. Does it need to be:

{
	"allow": true,
	"error-msg-on-failed-posture-compliance": "",
	"posture-compliance-statuses": [],
        "schema-version": "v1",
}

If it does then yea, we'll need to keep the:

PostureComplianceStatuses: []string{},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It tested with

                return events.ClientVPNConnectionHandlerResponse{
                        Allow: true,
                        ErrorMsgOnFailedPostureCompliance: "",
                        SchemaVersion: "v1",
                }, nil

and

                return events.ClientVPNConnectionHandlerResponse{
                        Allow: true,
                        SchemaVersion: "v1",
                }, nil

and they both fail with an "internal" error. I think the {} is indeed required to be returned to ClientVPN.

Would you think a func NewClientVPNConnectionHandlerResponse would be good or should we leave the example as-is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that stinks. It's [] not {} right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry; it's an empty array. Even in JSON. The behavior with ClientVPN I've tested is that both code snippets above in #343 (comment) don't result in a successful authentication; whereas if I use the original snippet that initializes an empty array instead of nil it works.

So I think we either need to include this in the sample or provide a function that correctly initializes the response struct.

SchemaVersion: "v1",
}, nil
}

log.Printf("Blocking access from: %q", sourceIP)
return events.ClientVPNConnectionHandlerResponse{
Allow: false,
ErrorMsgOnFailedPostureCompliance: "You're trying to connect from an IP address that is not allowed.",
PostureComplianceStatuses: []string{"BlockedSourceIP"},
SchemaVersion: "v1",
}, nil
}

func main() {
lambda.Start(handler)
}
```
20 changes: 20 additions & 0 deletions events/clientvpn.go
@@ -0,0 +1,20 @@
package events

type ClientVPNConnectionHandlerRequest struct {
ConnectionID string `json:"connection-id"`
EndpointID string `json:"endpoint-id"`
CommonName string `json:"common-name"`
Username string `json:"username"`
OSPlatform string `json:"platform"`
OSPlatformVersion string `json:"platform-version"`
PublicIP string `json:"public-ip"`
ClientOpenVPNVersion string `json:"client-openvpn-version"`
SchemaVersion string `json:"schema-version"`
}

type ClientVPNConnectionHandlerResponse struct {
Allow bool `json:"allow"`
ErrorMsgOnFailedPostureCompliance string `json:"error-msg-on-failed-posture-compliance"`
PostureComplianceStatuses []string `json:"posture-compliance-statuses"`
SchemaVersion string `json:"schema-version"`
}
36 changes: 36 additions & 0 deletions events/clientvpn_test.go
@@ -0,0 +1,36 @@
package events

import (
"encoding/json"
"io/ioutil"
"testing"

"github.com/aws/aws-lambda-go/events/test"
"github.com/stretchr/testify/assert"
)

func TestClientVPNConnectionHandlerRequestMarshaling(t *testing.T) {
// read json from file
inputJSON, err := ioutil.ReadFile("./testdata/clientvpn-connectionhandler-request.json")
if err != nil {
t.Errorf("could not open test file. details: %v", err)
}

// de-serialize into ClientVPNConnectionHandlerRequest
var inputEvent ClientVPNConnectionHandlerRequest
if err := json.Unmarshal(inputJSON, &inputEvent); err != nil {
t.Errorf("could not unmarshal event. details: %v", err)
}

// serialize to json
outputJSON, err := json.Marshal(inputEvent)
if err != nil {
t.Errorf("could not marshal event. details: %v", err)
}

assert.JSONEq(t, string(inputJSON), string(outputJSON))
}

func TestClientVPNConnectionHandlerRequestMarshalingMalformedJson(t *testing.T) {
test.TestMalformedJson(t, ClientVPNConnectionHandlerRequest{})
}
11 changes: 11 additions & 0 deletions events/testdata/clientvpn-connectionhandler-request.json
@@ -0,0 +1,11 @@
{
"connection-id": "cvpn-connection-04e7e1b2f0daf9460",
"endpoint-id": "cvpn-endpoint-0f13eab7f860433cc",
"common-name": "",
"username": "username",
"platform": "",
"platform-version": "",
"public-ip": "10.11.12.13",
"client-openvpn-version": "",
"schema-version": "v1"
}