Provide a way to create and configure AWSGuard #33

justinvp · 2019-11-25T21:55:53Z

This provides a nicer way to create and configure the AWS Guard policies:

const awsGuard = new AwsGuard();

The above is equivalent to:

const awsGuard = new AwsGuard({ all: "advisory" });

To make all policies mandatory rather than advisory:

const awsGuard = new AwsGuard({ all: "mandatory" });

To make all policies mandatory, but change a couple to be advisory:

const awsGuard = new AwsGuard({
    all: "mandatory",
    ec2InstanceNoPublicIP: "advisory",
    elbAccessLoggingEnabled: "advisory",
});

To disable a particular policy:

const awsGuard = new AwsGuard({
    ec2InstanceNoPublicIP: "disabled",
});

To disable all policies except ones explicitly enabled:

const awsGuard = new AwsGuard({
    all: "disabled",
    ec2InstanceNoPublicIP: "mandatory",
    elbAccessLoggingEnabled: "mandatory",
});

To specify configuration for policies that have it:

const awsGuard = new AwsGuard({
    ec2VolumeInUseCheck: { checkDeletion: false },
    encryptedVolumes: { enforcementLevel: "mandatory", kmsId: "id" },
    redshiftClusterMaintenanceSettingsCheck: { preferredMaintenanceWindow: "Mon:09:30-Mon:10:00" },
    acmCheckCertificateExpiration: { maxDaysUntilExpiration: 10 },
});

Use the packages from NPM rather than requiring global installs of typescript, tslint, and mocha.

justinvp · 2019-11-25T22:03:44Z

src/awsGuard.ts

+ *     ec2InstanceNoPublicIP: "mandatory",
+ *     elbAccessLoggingEnabled: "mandatory",


I considered using the policy names themselves instead of the names like this. However, the policy names include hyphens which would necessitate specifying as strings:

const awsGuard = new AwsGuard({ "ec2-instance-no-public-ip": "advisory", "elb-logging-enabled": "advisory", });

I discussed with Erin and doing it this way doesn't feel as idiomatic as is typical with TS/JS where properties are camelCased. I'd prefer to keep these camelCase for TS/JS similar to the args passed to Pulumi resources.

When we eventually add a more general (lower-level) way of configuring policy packs (that the service could use to side-load configure policy packs), which would probably use the policy names (with hyphens) as the keys, we could still use this more idiomatic camelCasing scheme as I've proposed here for the TS API, but under the covers map the camelCase names to hyphen names for use with the built-in configuration mechanism.

justinvp · 2019-11-25T22:07:53Z

src/awsGuard.ts

+ * });
+ * ```
+ */
+export class AwsGuard extends PolicyPack {


Originally, I had named this AWSGuardPolicyPack, but then opted for simply AWSGuard, and finally changed the casing to match how we case Aws in API signatures in @pulumi/aws resulting in AwsGuard.

justinvp · 2019-11-25T22:07:56Z

src/awsGuard.ts

+    constructor(args?: AwsGuardArgs);
+    constructor(name: string, args?: AwsGuardArgs);
+    constructor(nameOrArgs?: string | AwsGuardArgs, args?: AwsGuardArgs) {


This may look odd, if you're unfamiliar with overloads in TypeScript. The first two signatures are the two available overloads, and the third is the implementation (the latter not being shown in intellisense).

This allows the following possible ways of constructing:

new AwsGuard(); new AwsGuard({ all: "advisory"}); new AwsGuard("my-custom-name", { all: "advisory" });

justinvp · 2019-11-25T22:15:55Z

src/compute.ts

+declare module "./awsGuard" {
+    interface AwsGuardArgs {
+        ec2InstanceDetailedMonitoringEnabled?: EnforcementLevel;
+        ec2InstanceNoPublicIP?: EnforcementLevel;
+        ec2VolumeInUseCheck?: EnforcementLevel | Ec2VolumeInUseCheckArgs;
+        elbAccessLoggingEnabled?: EnforcementLevel;
+        encryptedVolumes?: EnforcementLevel | EncryptedVolumesArgs;
+    }
+}


I considered having separate interfaces for each policy, e.g.:

export interface Ec2InstanceDetailedMonitoringEnabledPolicyPackArgs { ec2InstanceDetailedMonitoringEnabled?: EnforcementLevel; } export interface Ec2InstanceNoPublicIPPolicyPackArgs { ec2InstanceNoPublicIP?: EnforcementLevel; } // etc.

And then defining AwsGuardArgs as:

export interface AwsGuardArgs extends Ec2InstanceDetailedMonitoringEnabledPolicyPackArgs, Ec2InstanceNoPublicIPPolicyPackArgs, // etc. { all?: EnforcementLevel; }

Or:

export type AwsGuardArgs = Ec2InstanceDetailedMonitoringEnabledPolicyPackArgs & Ec2InstanceNoPublicIPPolicyPackArgs & // etc.

But then it adds a bunch of unnecessary types and forces you to add the definitions where AwsGuardArgs is declared.

Doing it the way I've done it is consistent with how we apply "mixins" elsewhere, for example: https://github.com/pulumi/pulumi-aws/blob/daa4d0d4ea9ca5c785f2673f2c87c1865473a506/sdk/nodejs/s3/s3Mixins.ts#L215-L246

src/compute.ts

justinvp · 2019-11-25T22:25:19Z

src/index.ts

+// If we're running our integration tests, create a new instance of AwsGuard.
+if (process.env["PULUMI_AWSGUARD_TESTING"] === "true") {
+    const awsGuard = new AwsGuard();
+}


I've been thinking about whether we want to provide a built-in way that news-up the policy pack from within this library (and I saw what Chris did here), and the more I think about it, the more I think we shouldn't do it. Doing it would be the equivalent of having a resource library like @pulumi/eks that had some way of newing up an eks.Cluster within the library itself, rather than requiring you to create your own Pulumi program that does it.

So my feeling is, we should just encourage the creation of your own Policy Pack (it'll be really easy to do because I'll be adding a template for pulumi policy new).

However, given the way we've currently created our integration tests, we just pass the src directory to --policy-pack when running the preview, so to keep the integration tests working in the simplest way, for now, I've used this temporary approach. But I'd like to follow-up and remove this from the library and fix-up the integration tests to not do it this way. I can open an issue and add a TODO if you agree.

I personally want to disagree. Because needing users to create their own, new, Node module that does nothing but instantiate a new AwsGuard() is kinda lame. (Since I need to get the files on disk, run npm install, etc.)

But even in a world where you can use a "built-in" or "default" version of the AWS Guard policy pack, you still need to have the source code on disk anyways. So even then we aren't really saving you any trouble.

So I'm inclined to agree with you. And that we should encourage the creation of custom Policy Packs, and just make that easy via pulumi policy new.

chrsmith

Some nits, but this looks WAY WAY WAY better than the quick hack I put together. 👍

chrsmith · 2019-11-25T22:35:14Z

src/awsGuard.ts

+/**
+ * Argument bag for configuring AwsGuard policies.
+ */
+// Mixins will be applied to this interface in each module that adds a property to


Nit: Mixing the /** */ and // comment styles here.

I actually did this deliberately. The /** */ is for the JSDoc comment for the API, the // comment below is an "implementation" comment.

src/awsGuard.ts

chrsmith · 2019-11-25T22:39:58Z

src/compute.ts

+        ec2InstanceNoPublicIP?: EnforcementLevel;
+        ec2VolumeInUseCheck?: EnforcementLevel | Ec2VolumeInUseCheckArgs;
+        elbAccessLoggingEnabled?: EnforcementLevel;
+        encryptedVolumes?: EnforcementLevel | EncryptedVolumesArgs;


I like the conciseness of how you are declaring the AwsGuardArgs args here using mix-ins. But it has the problem of being really confusing as more and more policies are added to the policy pack. A value like encryptedVolumes not not be unique across all args to all policies.... and the alternative of prefixing the arg with the policy name is kinda lame too.

All this to say: I like the pattern, but I am not sure if it is going to work out for AWS Guard given the large number of rules we plan on adding.

Every alternative I can think of is strictly worse, so let's keep this as-is. Just want calling it out in case this does end up causing a lot of friction, that we should be willing to switch to one of the alternative interfaces you considered if it becomes a big problem. (But it could be I'm being paranoid, and so let's try it this way as it makes policies much easier to author.)

src/compute.ts

chrsmith · 2019-11-25T22:48:25Z

src/index.ts

+// If we're running our integration tests, create a new instance of AwsGuard.
+if (process.env["PULUMI_AWSGUARD_TESTING"] === "true") {
+    const awsGuard = new AwsGuard();
+}


I personally want to disagree. Because needing users to create their own, new, Node module that does nothing but instantiate a new AwsGuard() is kinda lame. (Since I need to get the files on disk, run npm install, etc.)

But even in a world where you can use a "built-in" or "default" version of the AWS Guard policy pack, you still need to have the source code on disk anyways. So even then we aren't really saving you any trouble.

So I'm inclined to agree with you. And that we should encourage the creation of custom Policy Packs, and just make that easy via pulumi policy new.

chrsmith · 2019-11-25T22:50:01Z

src/security.ts

+    const { enforcementLevel, maxDaysUntilExpiration } = getValueOrDefault(args, {
+        enforcementLevel: defaultEnforcementLevel,
+    });
+    const maxDays = maxDaysUntilExpiration === undefined


Can we have getValueOrDefault be smart enough to take this into account? e.g.

const { enforcementLevel, maxDaysUntilExpiration } = getValueOrDefault(args, { enforcementLevel: defaultEnforcementLevel, maxDaysUntilExpiration: 14, });

src/database.ts

ekrengel · 2019-11-25T23:04:25Z

src/enforcementLevel.ts

+import { EnforcementLevel } from "@pulumi/policy";
+
+/** @internal */
+export const defaultEnforcementLevel: EnforcementLevel = "mandatory";


I believe we talked about making them "advisory" by default"... im cool with either way just want to know why we changed

I just matched how we were exporting these currently, but happy to make "advisory" the default. Chris had mentioned that in his PR as well.

justinvp added 5 commits November 25, 2019 01:46

Avoid global dependencies

5578d94

Use the packages from NPM rather than requiring global installs of typescript, tslint, and mocha.

Provide a way to create and configure AWSGuard

ceaa597

Make integration tests work

fb1b97b

Delete Pulumi.yaml

7df216f

Temporarily pin to latest dev release of @pulumi/policy

53a7b6c

justinvp commented Nov 25, 2019

View reviewed changes

src/compute.ts Show resolved Hide resolved

justinvp commented Nov 25, 2019

View reviewed changes

justinvp requested review from chrsmith and ekrengel November 25, 2019 22:26

chrsmith approved these changes Nov 25, 2019

View reviewed changes

ekrengel reviewed Nov 25, 2019

View reviewed changes

src/database.ts Show resolved Hide resolved

ekrengel reviewed Nov 25, 2019

View reviewed changes

ekrengel self-requested a review November 26, 2019 15:55

ekrengel approved these changes Nov 26, 2019

View reviewed changes

justinvp added 2 commits November 26, 2019 12:24

Address feedback

6d2d3b7

Integration tests expect mandatory

b91f3bf

justinvp merged commit 612bf9e into master Nov 26, 2019

pulumi-bot deleted the justin/use branch November 26, 2019 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to create and configure AWSGuard #33

Provide a way to create and configure AWSGuard #33

justinvp commented Nov 25, 2019 •

edited

Loading

justinvp Nov 25, 2019 •

edited

Loading

justinvp Nov 25, 2019

justinvp Nov 25, 2019 •

edited

Loading

justinvp Nov 25, 2019

justinvp Nov 25, 2019

chrsmith Nov 25, 2019

chrsmith left a comment

chrsmith Nov 25, 2019

justinvp Nov 25, 2019

chrsmith Nov 25, 2019

chrsmith Nov 25, 2019

chrsmith Nov 25, 2019

ekrengel Nov 25, 2019

justinvp Nov 25, 2019

		* ec2InstanceNoPublicIP: "mandatory",
		* elbAccessLoggingEnabled: "mandatory",

Provide a way to create and configure AWSGuard #33

Provide a way to create and configure AWSGuard #33

Conversation

justinvp commented Nov 25, 2019 • edited Loading

justinvp Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinvp Nov 25, 2019 • 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

chrsmith 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

justinvp commented Nov 25, 2019 •

edited

Loading

justinvp Nov 25, 2019 •

edited

Loading

justinvp Nov 25, 2019 •

edited

Loading