feat: improve the interface for defining rule details and pack extensibility

[JeremyJonas]

Description

Currently Rules only define a name and the Pack must define the info and explanation, but this seems redundant if multiple Packs share the same rules. I see some minor differences in these details between packs, but seems trivial (assuming HIPAA for instance has specific info/explanation that differs for compliance terminology maybe).

It would be great to have the Rule file contain a default info and explanation so pack creators don't copy paste from existing packs. And make it so packs can overwrite this default. This would also provide better context in the rule directly rather than relying on the pack.

Not sure what the reasoning behind current abstraction is, would love to find out?

Additionally, following a pattern/interface were Packs expose a dictionary of Rules would be very beneficial to creating subsets of an existing Pack that retains the same details, or creating suppression utils with explicit rule references rather than loosely couple literal names.

The additional benefit of this would be that the RULES.md markdown file could also be auto-generated.

https://github.com/cdklabs/cdk-nag/blob/bd4bb47501203c3052502945451b9168d5c0c3f0/src/packs/aws-solutions.ts#L877-L885

https://github.com/cdklabs/cdk-nag/blob/c43361f58717a5dee1e94ac84318ccf32f6d57e3/src/rules/apigw/APIGWAccessLogging.ts#L10-L46

Use Case

1. Reuse the info/explanation of rules across packs
2. Create a sub-set pack from existing pack
3. Support creation of suppression utils that can utilize strict typing of pack rules
4. Auto-generate RULES.md

Proposed Solution

Rules

Rule Interface

export interface IRule {
  validate(node: CfnResource): NagRuleResult;
  name: string;
  info: string;
  explanation: string;
}

Rule Implementation

export default {
  /**
   * APIs have access logging enabled
   * @param node the CfnResource to check
   */
  validate(node: CfnResource): NagRuleCompliance {
    // ...
  },
  name: parse(__filename).name,
  info: 'The API does not have access logging enabled.',
  explanation: 'Enabling access logs helps operators view who accessed an API and how the caller accessed the API.',
} as IRule;

Pack

Pack Rule Interface

adds level

interface IPackRule extends IRule {
  level: NagMessageLevel;
}

NagPack (base)

export abstract class NagPack implements IAspect {
  // ...

  // mapping of "RuleSuffix=>IRule"
  public abstract rules: Record<string, IPackRule>;
  /**
   * All aspects can visit an IConstruct.
   */
  public visit(node: IConstruct): void {
    Object.entries(this.rules).forEach(([ruleSuffixOverride, rule]) => {
      if (node instanceof CfnResource) {
        this.applyRule({
          node,
          ruleSuffixOverride,
          info: rule.info,
          explanation: rule.explanation,
          level: rule.level,
          rule: rule.validate,
        });
      }
    })
  }
 // ...

NagPack (implementation)

export class AwsSolutionsChecks extends NagPack {
  //...

  public rules: Record<string, IPackRule> = {
    // COMPUTE
    'EB1': {
      ...ElasticBeanstalkVPCSpecified,
      level: NagMessageLevel.ERROR,
    },
    'EB3': {
      ...ElasticBeanstalkManagedUpdatesEnabled,
      level: NagMessageLevel.ERROR,
      info: 'Add custom pack specific info to override default - blah blah',
      explanation: 'blah blah specific for pack'
    },
    // STORAGE
    'S1': {
      ...S3BucketLoggingEnabled,
      level: NagMessageLevel.ERROR,
    },
    'S2': {
      ...S3BucketLevelPublicAccessProhibited,
      level: NagMessageLevel.ERROR,
      info: 'Add custom pack specific info to override default - blah blah',
      explanation: 'blah blah specific for pack'
    },
  };
 
 //...
};

Other information

No response

Acknowledge

• [X] I may be able to implement this feature request
• [ ] This feature might incur a breaking change

[dontirun]

@JeremyJonas , I really like this idea and I think we should implement it! I have a few suggestions that I think we can work out over the course of the PR.

I think we should mark this is a breaking change, since it will break any user created packs that are currently using any of the exported rules

Not sure what the reasoning behind current abstraction is, would love to find out?

The shortest answer is that this was my first project in TypeScript and there was originally only one pack. 😅

[dontirun]

I think we should mark this is a breaking change, since it will break any user created packs that are currently using any of the exported rules

On second thought we can break this into separate PRs. This PR can be focused on changing the internals (as outlined).

I can work on seperate PR afterwards to rewrite the existing rules. That PR can be marked as breaking

[JeremyJonas]

@dontirun Yes I agree, the internals could be written to support backward compatibility (support both ways of writing rules and the optionally providing a dictionary on packs to expose them) and keep in v1. It would be a bit extra code, but definitely possible.

You could actually export the new IRule format suggested as "named export" and the old way as "default". That way even developers of rules/packs would not have a breaking change as well :)

export const APIGWAccessLogging: IRule = {
  /**
   * APIs have access logging enabled
   * @param node the CfnResource to check
   */
  validate(node: CfnResource): NagRuleCompliance {
    // ...
  },
  name: parse(__filename).name,
  info: 'The API does not have access logging enabled.',
  explanation: 'Enabling access logs helps operators view who accessed an API and how the caller accessed the API.',
};

// backward compatibility
export default Object.defineProperty(
  (node: CfnResource): NagRuleCompliance => APIGWAccessLogging.validate(node),
  'name',
  { value: APIGWAccessLogging.name }
);

[mrpackethead]

This definately is a +1