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

fix: remove unused app client id and secret since SSH key is used for authorization #1223

Merged
merged 2 commits into from
Oct 11, 2021
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 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 @@ -377,7 +374,7 @@ No requirements.
| 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 |
| 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
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
36 changes: 16 additions & 20 deletions modules/runners/scale-up.tf
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,17 @@ resource "aws_lambda_function" "scale_up" {

environment {
variables = {
ENABLE_ORGANIZATION_RUNNERS = var.enable_organization_runners
ENVIRONMENT = var.environment
GHES_URL = var.ghes_url
NODE_TLS_REJECT_UNAUTHORIZED = var.ghes_url != null && ! var.ghes_ssl_verify ? 0 : 1
RUNNER_EXTRA_LABELS = var.runner_extra_labels
RUNNER_GROUP_NAME = var.runner_group_name
RUNNERS_MAXIMUM_COUNT = var.runners_maximum_count
LAUNCH_TEMPLATE_NAME = join(",", [for template in aws_launch_template.runner : template.name])
SUBNET_IDS = join(",", var.subnet_ids)
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
ENABLE_ORGANIZATION_RUNNERS = var.enable_organization_runners
ENVIRONMENT = var.environment
GHES_URL = var.ghes_url
NODE_TLS_REJECT_UNAUTHORIZED = var.ghes_url != null && ! var.ghes_ssl_verify ? 0 : 1
RUNNER_EXTRA_LABELS = var.runner_extra_labels
RUNNER_GROUP_NAME = var.runner_group_name
RUNNERS_MAXIMUM_COUNT = var.runners_maximum_count
LAUNCH_TEMPLATE_NAME = join(",", [for template in aws_launch_template.runner : template.name])
SUBNET_IDS = join(",", var.subnet_ids)
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 @@ -70,13 +68,11 @@ resource "aws_iam_role_policy" "scale_up" {
name = "${var.environment}-lambda-scale-up-policy"
role = aws_iam_role.scale_up.name
policy = templatefile("${path.module}/policies/lambda-scale-up.json", {
arn_runner_instance_role = aws_iam_role.runner.arn
sqs_arn = var.sqs_build_queue.arn
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
arn_runner_instance_role = aws_iam_role.runner.arn
sqs_arn = var.sqs_build_queue.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
6 changes: 2 additions & 4 deletions modules/runners/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,8 @@ variable "enable_organization_runners" {
variable "github_app_parameters" {
description = "Parameter Store for GitHub App Parameters."
type = object({
key_base64 = map(string)
id = map(string)
client_id = map(string)
client_secret = map(string)
key_base64 = map(string)
id = map(string)
})
}

Expand Down
Loading