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.

Sign up for GitHub

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

feat: support passing multiple trustedRepositories #41

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: support passing multiple trustedRepositories #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3332392
feat: support passing multiple trustedRepositories
blimmer Nov 10, 2023
0f2e2c8
fix: pass ForAnyValue:StringLike
blimmer Nov 10, 2023
b473ee1
feat: add tests
blimmer Nov 10, 2023
051ccd2
docs: document new interface
blimmer Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions API.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,27 @@ const uploadRole = new GithubActionsRole(scope, "UploadRole", {
myBucket.grantWrite(uploadRole);
```

You can also pass multiple trusted repositories with the `trustedRepositories` property:

```ts
import { GithubActionsRole } from "aws-cdk-github-oidc";

const uploadRole = new GithubActionsRole(stack, 'TestRole', {
provider,
trustedRepositories: [
{
owner: 'octo-org',
repo: 'octo-repo1',
},
{
owner: 'octo-org',
repo: 'octo-repo2',
filter: 'ref:refs/tags/v*',
},
],
});
```

You may pass in any `iam.RoleProps` into the construct's props, except `assumedBy` which will be defined by this construct (CDK will fail if you do):

```ts
Expand Down
90 changes: 79 additions & 11 deletions src/role.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,52 @@ export interface GithubConfiguration {
*/
readonly provider: IGithubActionsIdentityProvider;

/**
* Repository owner (organization or username).
*
* Use `trustedRepositories` if you want to provide multiple repo configurations
*
* @example
* 'octo-org'
*/
readonly owner?: string;

/**
* Repository name (slug) without the owner.
*
* Use `trustedRepositories` if you want to provide multiple repo configurations
*
* @example
* 'octo-repo'
*/
readonly repo?: string;

/**
* Subject condition filter, appended after `repo:${owner}/${repo}:` string in IAM Role trust relationship.
*
* @default
* '*'
*
* You may use this value to only allow Github to assume the role on specific branches, tags, environments, pull requests etc.
* @example
* 'ref:refs/tags/v*'
* 'ref:refs/heads/demo-branch'
* 'pull_request'
* 'environment:Production'
*
* @see https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect#examples
*/
readonly filter?: string;

/**
* Provide multiple trusted repositories allowed to assume this role.
*
* @default - required if top-level owner/repo/filter not set
*/
readonly trustedRepositories?: TrustedRepository[];
Copy link
Author

Choose a reason for hiding this comment

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

Up for discussion - would it be clearer to deprecate / remove the top-level properties and force people to use trustedRepostories instead?

}

export interface TrustedRepository {
Copy link
Author

Choose a reason for hiding this comment

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

TrustedRepository takes the same top-level owner/repo and optionally filter props.

/**
* Repository owner (organization or username).
*
Expand Down Expand Up @@ -102,6 +148,7 @@ export class GithubActionsRole extends iam.Role {
delete extractProps.owner;
delete extractProps.repo;
delete extractProps.filter;
delete extractProps.trustedRepositories;
return extractProps;
}

Expand All @@ -112,6 +159,20 @@ export class GithubActionsRole extends iam.Role {
}
}

/** Validates conflicting props aren't set simultaneously */
private static validateProps(scope: Construct, props: GithubActionsRoleProps) {
const topLevelPropsSet = props.owner || props.repo || props.filter;
const trustedRepositoriesSet = props.trustedRepositories && props.trustedRepositories.length > 0 || false;

if (topLevelPropsSet && trustedRepositoriesSet) {
cdk.Annotations.of(scope).addError('Cannot set both top-level owner/repo/filter and trustedRepositories. Use one or the other.');
}

if (!trustedRepositoriesSet && (props.owner === undefined || props.repo === undefined)) {
cdk.Annotations.of(scope).addError("If you don't provide `trustedRepositories`, you must provide `owner` and `repo`.");
}
Copy link
Author

Choose a reason for hiding this comment

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

If we decided to deprecate the top-level properties, we could annotate the deprecated properties here.

}

/** Validates the Github repository name (without owner). */
private static validateRepo(scope: Construct, repo: string): void {
if (repo === '') {
Expand All @@ -120,8 +181,8 @@ export class GithubActionsRole extends iam.Role {
}

/** Formats the `sub` value used in trust policy. */
private static formatSubject(props: GithubConfiguration): string {
const { owner, repo, filter = '*' } = props;
private static formatSubject(trustedRepository: TrustedRepository): string {
const { owner, repo, filter = '*' } = trustedRepository;
return `repo:${owner}/${repo}:${filter}`;
}

Expand All @@ -146,25 +207,32 @@ export class GithubActionsRole extends iam.Role {
*/
constructor(scope: Construct, id: string, props: GithubActionsRoleProps) {

const { provider, owner, repo } = props;
const { provider, owner, repo, trustedRepositories } = props;

// Validate props
GithubActionsRole.validateProps(scope, props);

// Perform validations
GithubActionsRole.validateOwner(scope, owner);
GithubActionsRole.validateRepo(scope, repo);
// Unify the two ways of defining trusted repositories
const subjects = trustedRepositories || [{ owner: owner!, repo: repo!, filter: props.filter }];

// Perform validations on each trusted repository
subjects.forEach((subject) => {
GithubActionsRole.validateOwner(scope, subject.owner);
GithubActionsRole.validateRepo(scope, subject.repo);
});

// Prepare values
const subject = GithubActionsRole.formatSubject(props);
// Extract IAM Role props
const roleProps = GithubActionsRole.extractRoleProps(props);

// The actual IAM Role creation
super(scope, id, {
...roleProps,
assumedBy: new iam.WebIdentityPrincipal(provider.openIdConnectProviderArn, {
StringLike: {
'ForAnyValue:StringLike': {
Copy link
Author

Choose a reason for hiding this comment

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

This should be safe even if just one property is passed. From the docs:

ForAnyValue – This qualifier tests whether at least one member of the set of request context key values matches at least one member of the set of context key values in your policy condition. The context key returns true if any one of the context key values in the request matches any one of the context key values in the policy. For no matching context key or a null dataset, the condition returns false.

If we think people might be worried about seeing a cdk diff with this, we could optionally use ForAnyValue only when multiple repos are passed.

Copy link

Choose a reason for hiding this comment

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

When setting this up manually, StringLike also worked. As there is only one key in this map there should be no difference. The array of values is automatically evaluated as an OR

// Only allow specified subjects to assume this role
[`${GithubActionsIdentityProvider.issuer}:sub`]: subject,
[`${GithubActionsIdentityProvider.issuer}:sub`]: subjects.map((subject) => GithubActionsRole.formatSubject(subject)),
},
StringEquals: {
'StringEquals': {
// Audience is always sts.amazonaws.com with AWS official Github Action
// https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-amazon-web-services#adding-the-identity-provider-to-aws
[`${GithubActionsIdentityProvider.issuer}:aud`]: 'sts.amazonaws.com',
Expand Down
109 changes: 103 additions & 6 deletions test/role.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,69 @@ test('Role with defaults', () => {
Action: 'sts:AssumeRoleWithWebIdentity',
Effect: 'Allow',
Condition: {
StringLike: {
'token.actions.githubusercontent.com:sub': 'repo:octo-org/octo-repo:*',
'ForAnyValue:StringLike': {
'token.actions.githubusercontent.com:sub': ['repo:octo-org/octo-repo:*'],
},
StringEquals: {
'StringEquals': {
'token.actions.githubusercontent.com:aud': 'sts.amazonaws.com',
},
},
Principal: {
Federated: {
'Fn::Join': [
'',
[
'arn:aws:iam::',
{
Ref: 'AWS::AccountId',
},
':oidc-provider/token.actions.githubusercontent.com',
],
],
},
},
}),
]),
}),
});
});

test('Role with multiple trusted repositories', () => {
const app = new cdk.App();
const stack = new cdk.Stack(app);
const provider = GithubActionsIdentityProvider.fromAccount(stack, 'GithubProvider');

new GithubActionsRole(stack, 'TestRole', {
provider,
trustedRepositories: [
{
owner: 'octo-org',
repo: 'octo-repo1',
},
{
owner: 'octo-org',
repo: 'octo-repo2',
filter: 'ref:refs/tags/v*',
},
],
});

const template = Template.fromStack(stack);

template.hasResourceProperties('AWS::IAM::Role', {
AssumeRolePolicyDocument: Match.objectLike({
Statement: Match.arrayWith([
Match.objectLike({
Action: 'sts:AssumeRoleWithWebIdentity',
Effect: 'Allow',
Condition: {
'ForAnyValue:StringLike': {
'token.actions.githubusercontent.com:sub': [
'repo:octo-org/octo-repo1:*',
'repo:octo-org/octo-repo2:ref:refs/tags/v*',
],
},
'StringEquals': {
'token.actions.githubusercontent.com:aud': 'sts.amazonaws.com',
},
},
Expand Down Expand Up @@ -88,10 +147,10 @@ test('Role with custom props', () => {
Action: 'sts:AssumeRoleWithWebIdentity',
Effect: 'Allow',
Condition: {
StringLike: {
'token.actions.githubusercontent.com:sub': 'repo:octo-org/octo-repo:ref:refs/tags/v*',
'ForAnyValue:StringLike': {
'token.actions.githubusercontent.com:sub': ['repo:octo-org/octo-repo:ref:refs/tags/v*'],
},
StringEquals: {
'StringEquals': {
'token.actions.githubusercontent.com:aud': 'sts.amazonaws.com',
},
},
Expand Down Expand Up @@ -187,3 +246,41 @@ test('Role with invalid repo', () => {
'Invalid Github Repository Name "". May not be empty string.',
);
});

test('Role with top-level props and trustedRepositories set', () => {
const app = new cdk.App();
const stack = new cdk.Stack(app);
const provider = GithubActionsIdentityProvider.fromAccount(stack, 'GithubProvider');

new GithubActionsRole(stack, 'TestRole', {
provider,
owner: 'octo-org',
repo: 'octo-repo',
trustedRepositories: [
{
owner: 'octo-org',
repo: 'octo-repo',
},
],
});

expect(stack.node.metadata).toHaveLength(1);
expect(stack.node.metadata[0].data).toBe(
'Cannot set both top-level owner/repo/filter and trustedRepositories. Use one or the other.',
);
});

test('Role without trustedRepositories or top-level props set', () => {
const app = new cdk.App();
const stack = new cdk.Stack(app);
const provider = GithubActionsIdentityProvider.fromAccount(stack, 'GithubProvider');

new GithubActionsRole(stack, 'TestRole', {
provider,
});

expect(stack.node.metadata).toHaveLength(1);
expect(stack.node.metadata[0].data).toBe(
"If you don't provide `trustedRepositories`, you must provide `owner` and `repo`.",
);
});