Skip to content

Commit

Permalink
fix: remove unused app client since SSH key is used to secure app aut…
Browse files Browse the repository at this point in the history
…horization
  • Loading branch information
npalm committed Oct 1, 2021
1 parent 3c3ef19 commit baefb53
Show file tree
Hide file tree
Showing 16 changed files with 89 additions and 126 deletions.
27 changes: 20 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,7 @@ Go to GitHub and [create a new app](https://docs.github.com/en/developers/apps/c
- `Self-hosted runners`: Read & write (to register runner)
8. Save the new app.
9. On the General page, make a note of the "App ID" and "Client ID" parameters.
10. Create a new client secret and also write it down.
11. Generate a new private key and save the `app.private-key.pem` file.
10. Generate a new private key and save the `app.private-key.pem` file.

### Setup terraform module

Expand Down Expand Up @@ -174,8 +173,6 @@ module "github-runner" {
github_app = {
key_base64 = "base64string"
id = "1"
client_id = "c-123"
client_secret = "client_secret"
webhook_secret = "webhook_secret"
}
Expand Down Expand Up @@ -343,6 +340,23 @@ No requirements.
| aws | n/a |
| random | n/a |

## Modules

| Name | Source | Version |
|------|--------|---------|
| runner_binaries | ./modules/runner-binaries-syncer | |
| runners | ./modules/runners | |
| ssm | ./modules/ssm | |
| webhook | ./modules/webhook | |

## Resources

| Name |
|------|
| [aws_resourcegroups_group](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/resourcegroups_group) |
| [aws_sqs_queue](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/sqs_queue) |
| [random_string](https://registry.terraform.io/providers/hashicorp/random/latest/docs/resources/string) |

## Inputs

| Name | Description | Type | Default | Required |
Expand All @@ -358,9 +372,9 @@ No requirements.
| enable\_organization\_runners | Register runners to organization, instead of repo level | `bool` | `false` | no |
| enable\_ssm\_on\_runners | Enable to allow access the runner instances for debugging purposes via SSM. Note that this adds additional permissions to the runner instances. | `bool` | `false` | no |
| environment | A name that identifies the environment, used as prefix and for tagging. | `string` | n/a | yes |
| ghes\_ssl\_verify | GitHub Enterprise SSL verification. Set to 'false' when custom certificate (chains) is used for GitHub Enterprise Server (insecure). | `bool` | `true` | no |
| ghes\_url | GitHub Enterprise Server URL. Example: https://github.internal.co - DO NOT SET IF USING PUBLIC GITHUB | `string` | `null` | no |
| ghes\_ssl\_verify | GitHub Enterprise SSL verification. Set to `false` when custom certificate (chains) is used for GitHub Enterprise Server (insecure). | `bool` | `true` | no |
| github\_app | GitHub app parameters, see your github app. Ensure the key is the base64-encoded `.pem` file (the output of `base64 app.private-key.pem`, not the content of `private-key.pem`). | <pre>object({<br> key_base64 = string<br> id = string<br> client_id = string<br> client_secret = string<br> webhook_secret = string<br> })</pre> | n/a | yes |
| github\_app | GitHub app parameters, see your github app. Ensure the key is the base64-encoded `.pem` file (the output of `base64 app.private-key.pem`, not the content of `private-key.pem`). | <pre>object({<br> key_base64 = string<br> id = string<br> webhook_secret = string<br> })</pre> | n/a | yes |
| idle\_config | List of time period that can be defined as cron expression to keep a minimum amount of runners active instead of scaling down to 0. By defining this list you can ensure that in time periods that match the cron expression within 5 seconds a runner is kept idle. | <pre>list(object({<br> cron = string<br> timeZone = string<br> idleCount = number<br> }))</pre> | `[]` | no |
| instance\_profile\_path | The path that will be added to the instance\_profile, if not set the environment name will be used. | `string` | `null` | no |
| instance\_type | [DEPRECATED] See instance\_types. | `string` | `"m5.large"` | no |
Expand Down Expand Up @@ -416,7 +430,6 @@ No requirements.
| runners | n/a |
| ssm\_parameters | n/a |
| webhook | n/a |

<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

## Contribution
Expand Down
13 changes: 8 additions & 5 deletions examples/default/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ module "runners" {
github_app = {
key_base64 = var.github_app_key_base64
id = var.github_app_id
client_id = var.github_app_client_id
client_secret = var.github_app_client_secret
webhook_secret = random_password.random.result
}

Expand All @@ -48,9 +46,14 @@ module "runners" {
# idleCount = 1
# }]

# disable KMS and encryption
# encrypt_secrets = false

# Let the module manage the service linked role
# create_service_linked_role_spot = true

instance_types = ["m5.large", "c5.large"]

# override delay of events in seconds
delay_webhook_event = 5

# override scaling down
scale_down_schedule_expression = "cron(* * * * ? *)"
}
2 changes: 0 additions & 2 deletions examples/ubuntu/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ module "runners" {
github_app = {
key_base64 = var.github_app_key_base64
id = var.github_app_id
client_id = var.github_app_client_id
client_secret = var.github_app_client_secret
webhook_secret = random_password.random.result
}

Expand Down
6 changes: 2 additions & 4 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@ locals {
ami_filter = length(var.ami_filter) > 0 ? var.ami_filter : local.runner_architecture == "arm64" ? { name = ["amzn2-ami-hvm-2*-arm64-gp2"] } : { name = ["amzn2-ami-hvm-2.*-x86_64-ebs"] }

github_app_parameters = {
client_id = module.ssm.parameters.github_app_client_id
client_secret = module.ssm.parameters.github_app_client_secret
id = module.ssm.parameters.github_app_id
key_base64 = module.ssm.parameters.github_app_key_base64
id = module.ssm.parameters.github_app_id
key_base64 = module.ssm.parameters.github_app_key_base64
}
}

Expand Down
29 changes: 27 additions & 2 deletions modules/runners/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,31 @@ No requirements.
|------|---------|
| aws | n/a |

## Modules

No Modules.

## Resources

| Name |
|------|
| [aws_ami](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/ami) |
| [aws_caller_identity](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/caller_identity) |
| [aws_cloudwatch_event_rule](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_event_rule) |
| [aws_cloudwatch_event_target](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_event_target) |
| [aws_cloudwatch_log_group](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_log_group) |
| [aws_iam_instance_profile](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_instance_profile) |
| [aws_iam_policy_document](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) |
| [aws_iam_role](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role) |
| [aws_iam_role_policy](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) |
| [aws_iam_role_policy_attachment](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) |
| [aws_lambda_event_source_mapping](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_event_source_mapping) |
| [aws_lambda_function](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_function) |
| [aws_lambda_permission](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_permission) |
| [aws_launch_template](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/launch_template) |
| [aws_security_group](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group) |
| [aws_ssm_parameter](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter) |

## Inputs

| Name | Description | Type | Default | Required |
Expand All @@ -73,8 +98,9 @@ No requirements.
| enable\_organization\_runners | n/a | `bool` | n/a | yes |
| enable\_ssm\_on\_runners | Enable to allow access to the runner instances for debugging purposes via SSM. Note that this adds additional permissions to the runner instances. | `bool` | n/a | yes |
| environment | A name that identifies the environment, used as prefix and for tagging. | `string` | n/a | yes |
| ghes\_ssl\_verify | GitHub Enterprise SSL verification. Set to 'false' when custom certificate (chains) is used for GitHub Enterprise Server (insecure). | `bool` | `true` | no |
| ghes\_url | GitHub Enterprise Server URL. DO NOT SET IF USING PUBLIC GITHUB | `string` | `null` | no |
| github\_app\_parameters | Parameter Store for GitHub App Parameters. | <pre>object({<br> key_base64 = map(string)<br> id = map(string)<br> client_id = map(string)<br> client_secret = map(string)<br> })</pre> | n/a | yes |
| github\_app\_parameters | Parameter Store for GitHub App Parameters. | <pre>object({<br> key_base64 = map(string)<br> id = map(string)<br> })</pre> | n/a | yes |
| idle\_config | List of time period that can be defined as cron expression to keep a minimum amount of runners active instead of scaling down to 0. By defining this list you can ensure that in time periods that match the cron expression within 5 seconds a runner is kept idle. | <pre>list(object({<br> cron = string<br> timeZone = string<br> idleCount = number<br> }))</pre> | `[]` | no |
| instance\_profile\_path | The path that will be added to the instance\_profile, if not set the environment name will be used. | `string` | `null` | no |
| instance\_type | [DEPRECATED] See instance\_types. | `string` | `"m5.large"` | no |
Expand Down Expand Up @@ -126,7 +152,6 @@ No requirements.
| role\_runner | n/a |
| role\_scale\_down | n/a |
| role\_scale\_up | n/a |

<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

## Philips Forest
Expand Down
36 changes: 3 additions & 33 deletions modules/runners/lambdas/runners/src/scale-runners/gh-auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,8 @@ jest.mock('@octokit/auth-app');
const cleanEnv = process.env;
const ENVIRONMENT = 'dev';
const GITHUB_APP_ID = '1';
const GITHUB_APP_CLIENT_ID = '1';
const GITHUB_APP_CLIENT_SECRET = 'client_secret';
const PARAMETER_GITHUB_APP_ID_NAME = `/actions-runner/${ENVIRONMENT}/github_app_id`;
const PARAMETER_GITHUB_APP_KEY_BASE64_NAME = `/actions-runner/${ENVIRONMENT}/github_app_key_base64`;
const PARAMETER_GITHUB_APP_CLIENT_ID_NAME = `/actions-runner/${ENVIRONMENT}/github_app_client_id`;
const PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME = `/actions-runner/${ENVIRONMENT}/github_app_client_secret`;

const mockedGet = mocked(getParameterValue);

Expand All @@ -31,8 +27,6 @@ beforeEach(() => {
process.env = { ...cleanEnv };
process.env.PARAMETER_GITHUB_APP_ID_NAME = PARAMETER_GITHUB_APP_ID_NAME;
process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME = PARAMETER_GITHUB_APP_KEY_BASE64_NAME;
process.env.PARAMETER_GITHUB_APP_CLIENT_ID_NAME = PARAMETER_GITHUB_APP_CLIENT_ID_NAME;
process.env.PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME = PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME;
nock.disableNetConnect();
});

Expand Down Expand Up @@ -83,14 +77,8 @@ describe('Test createGithubAppAuth', () => {
appId: parseInt(GITHUB_APP_ID),
privateKey: decryptedValue,
installationId,
clientId: GITHUB_APP_CLIENT_ID,
clientSecret: GITHUB_APP_CLIENT_SECRET,
};
mockedGet
.mockResolvedValueOnce(GITHUB_APP_ID)
.mockResolvedValueOnce(b64)
.mockResolvedValueOnce(GITHUB_APP_CLIENT_ID)
.mockResolvedValueOnce(GITHUB_APP_CLIENT_SECRET);
mockedGet.mockResolvedValueOnce(GITHUB_APP_ID).mockResolvedValueOnce(b64);

const mockedAuth = jest.fn();
mockedAuth.mockResolvedValue({ token });
Expand All @@ -104,8 +92,6 @@ describe('Test createGithubAppAuth', () => {
// Assert
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_ID_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_KEY_BASE64_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_CLIENT_ID_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME);

expect(mockedCreatAppAuth).toBeCalledTimes(1);
expect(mockedCreatAppAuth).toBeCalledWith(authOptions);
Expand All @@ -126,16 +112,10 @@ describe('Test createGithubAppAuth', () => {
appId: parseInt(GITHUB_APP_ID),
privateKey: decryptedValue,
installationId,
clientId: GITHUB_APP_CLIENT_ID,
clientSecret: GITHUB_APP_CLIENT_SECRET,
request: mockedRequestInterface.defaults({ baseUrl: githubServerUrl }),
};

mockedGet
.mockResolvedValueOnce(GITHUB_APP_ID)
.mockResolvedValueOnce(b64)
.mockResolvedValueOnce(GITHUB_APP_CLIENT_ID)
.mockResolvedValueOnce(GITHUB_APP_CLIENT_SECRET);
mockedGet.mockResolvedValueOnce(GITHUB_APP_ID).mockResolvedValueOnce(b64);
const mockedAuth = jest.fn();
mockedAuth.mockResolvedValue({ token });
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Expand All @@ -149,8 +129,6 @@ describe('Test createGithubAppAuth', () => {
// Assert
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_ID_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_KEY_BASE64_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_CLIENT_ID_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME);

expect(mockedCreatAppAuth).toBeCalledTimes(1);
expect(mockedCreatAppAuth).toBeCalledWith(authOptions);
Expand All @@ -172,16 +150,10 @@ describe('Test createGithubAppAuth', () => {
const authOptions = {
appId: parseInt(GITHUB_APP_ID),
privateKey: decryptedValue,
clientId: GITHUB_APP_CLIENT_ID,
clientSecret: GITHUB_APP_CLIENT_SECRET,
request: mockedRequestInterface.defaults({ baseUrl: githubServerUrl }),
};

mockedGet
.mockResolvedValueOnce(GITHUB_APP_ID)
.mockResolvedValueOnce(b64)
.mockResolvedValueOnce(GITHUB_APP_CLIENT_ID)
.mockResolvedValueOnce(GITHUB_APP_CLIENT_SECRET);
mockedGet.mockResolvedValueOnce(GITHUB_APP_ID).mockResolvedValueOnce(b64);
const mockedAuth = jest.fn();
mockedAuth.mockResolvedValue({ token });
mockedCreatAppAuth.mockImplementation(() => {
Expand All @@ -194,8 +166,6 @@ describe('Test createGithubAppAuth', () => {
// Assert
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_ID_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_KEY_BASE64_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_CLIENT_ID_NAME);
expect(getParameterValue).toBeCalledWith(PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME);

expect(mockedCreatAppAuth).toBeCalledTimes(1);
expect(mockedCreatAppAuth).toBeCalledWith(authOptions);
Expand Down
2 changes: 0 additions & 2 deletions modules/runners/lambdas/runners/src/scale-runners/gh-auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ async function createAuth(installationId: number | undefined, ghesApiUrl: string
await getParameterValue(process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME),
'base64',
).toString(),
clientId: await getParameterValue(process.env.PARAMETER_GITHUB_APP_CLIENT_ID_NAME),
clientSecret: await getParameterValue(process.env.PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME),
};
if (installationId) authOptions = { ...authOptions, installationId };

Expand Down
2 changes: 0 additions & 2 deletions modules/runners/policies/lambda-scale-down.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@
],
"Resource": [
"${github_app_key_base64_arn}",
"${github_app_client_secret_arn}",
"${github_app_client_id_arn}",
"${github_app_id_arn}"
]
%{ if kms_key_arn != "" ~}
Expand Down
2 changes: 0 additions & 2 deletions modules/runners/policies/lambda-scale-up.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@
],
"Resource": [
"${github_app_key_base64_arn}",
"${github_app_client_secret_arn}",
"${github_app_client_id_arn}",
"${github_app_id_arn}"
]
},
Expand Down
26 changes: 11 additions & 15 deletions modules/runners/scale-down.tf
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,14 @@ resource "aws_lambda_function" "scale_down" {

environment {
variables = {
ENVIRONMENT = var.environment
MINIMUM_RUNNING_TIME_IN_MINUTES = var.minimum_running_time_in_minutes
RUNNER_BOOT_TIME_IN_MINUTES = var.runner_boot_time_in_minutes
SCALE_DOWN_CONFIG = jsonencode(var.idle_config)
GHES_URL = var.ghes_url
NODE_TLS_REJECT_UNAUTHORIZED = var.ghes_url != null && ! var.ghes_ssl_verify ? 0 : 1
PARAMETER_GITHUB_APP_CLIENT_ID_NAME = var.github_app_parameters.client_id.name
PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME = var.github_app_parameters.client_secret.name
PARAMETER_GITHUB_APP_ID_NAME = var.github_app_parameters.id.name
PARAMETER_GITHUB_APP_KEY_BASE64_NAME = var.github_app_parameters.key_base64.name
ENVIRONMENT = var.environment
MINIMUM_RUNNING_TIME_IN_MINUTES = var.minimum_running_time_in_minutes
RUNNER_BOOT_TIME_IN_MINUTES = var.runner_boot_time_in_minutes
SCALE_DOWN_CONFIG = jsonencode(var.idle_config)
GHES_URL = var.ghes_url
NODE_TLS_REJECT_UNAUTHORIZED = var.ghes_url != null && ! var.ghes_ssl_verify ? 0 : 1
PARAMETER_GITHUB_APP_ID_NAME = var.github_app_parameters.id.name
PARAMETER_GITHUB_APP_KEY_BASE64_NAME = var.github_app_parameters.key_base64.name
}
}

Expand Down Expand Up @@ -72,11 +70,9 @@ resource "aws_iam_role_policy" "scale_down" {
name = "${var.environment}-lambda-scale-down-policy"
role = aws_iam_role.scale_down.name
policy = templatefile("${path.module}/policies/lambda-scale-down.json", {
github_app_client_id_arn = var.github_app_parameters.client_id.arn
github_app_client_secret_arn = var.github_app_parameters.client_secret.arn
github_app_id_arn = var.github_app_parameters.id.arn
github_app_key_base64_arn = var.github_app_parameters.key_base64.arn
kms_key_arn = local.kms_key_arn
github_app_id_arn = var.github_app_parameters.id.arn
github_app_key_base64_arn = var.github_app_parameters.key_base64.arn
kms_key_arn = local.kms_key_arn
})
}

Expand Down
Loading

0 comments on commit baefb53

Please sign in to comment.