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

Support Github OIDC authentication #13

Merged
merged 3 commits into from
May 14, 2024

Conversation

SemiConscious
Copy link
Contributor

@SemiConscious SemiConscious commented May 9, 2024

Hi There. First - thanks for creating this great action - it delivers very nearly everything we need right now, apart from this one little fix to allow us to stick to AWS best practice - auth onto AWS using the github OIDC provider (instead of IAM keys), and that's what this pull request is about.

I have made some minimal changes to the way AWS credentials are handled. There is a new input: aws-session-token which is added to the credentials objects. If it's a non-zero length string, the credentials object is cloned and returned as-is, instead of getting a new credentials object by using ID and Secret to authenticate the user, or assume the role. If there is nothing in aws-session-token the original flow is maintained. I believe this change will not harm existing configurations using v1.2.

I am using my forked and modified tree (this one) and it's working.

I hope this is useful and can eventually be merged. Let me know your thoughts.

A possible usability upgrade would be to detect if someone is using aws-actions/configure-aws-credentials and use the output or environment variables it generates directly, but I can't see a clean way to do that due to variables/env needing to be explicitly passed between workflow steps. Maybe you know how to do that.

All the best - Jim

@SemiConscious
Copy link
Contributor Author

SemiConscious commented May 9, 2024

Well - I'm surprised that there are merge conflicts here, but I get the same when I simulate a merge from my OIDC branch to main after merging your v2->v3 changes. I'm theorising there's some subtle line-ending or other whitespace conflict. dist/index is obviously something you can fix by npm run build, but the other two look like this:

image

image

In all cases, choose the one that isn't empty.

@SemiConscious
Copy link
Contributor Author

SemiConscious commented May 9, 2024

Perhaps I need to add a little more background here. I understand (from a contractor who is managing my company's migration from bitbucket to github) that Amazon best practice regarding authentication is to move away from using IAM keys to using temporary assumed roles. AWS have supplied the aws-actions/configure-aws-credentials@v4 action which incorporates this best practice. The 'recommended' flow is to use OIDC integration with AWS which allows github itself to assume a role with the required permissions, a temporary auth, without needing an IAM user, or keys. This simplifies the setup on the AWS end as well. In fact, as I am using terraform, I was able to use an existing module to generate everything I need:

# create a role that the github OIDC provider can assume

data "aws_iam_policy_document" "runner-user-role-policy" {
  statement {
    effect = "Allow"
    actions = [
      "ec2:DescribeInstances",
      "ec2:DescribeInstanceStatus",
      "ec2:DescribeInstanceTypes",
      "ec2:DescribeSubnets",
      "ec2:describeSpotPriceHistory",
      "pricing:GetProducts",
      "pricing:GetAttributeValues",
      # in case the instance has encrypted disks
      "kms:Encrypt",
      "kms:Decrypt",
      "kms:ReEncrypt*",
      "kms:GenerateDataKey*",
      "kms:DescribeKey"
    ]
    resources = ["*"]
  }

  # lock down ec2::RunInstances to our subnet and security group
  # see https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_examples_ec2_instances-subnet.html
  statement {
    effect = "Allow"
    actions = [
      "ec2:RunInstances"
    ]
    resources = [
      "<SUBNET_ARN>",
      "arn:aws:ec2:<REGION>:<ACCOUNTID>:network-interface/*",
      "arn:aws:ec2:<REGION>:<ACCOUNTID>:instance/*",
      "arn:aws:ec2:<REGION>:<ACCOUNTID>:volume/*",
      "arn:aws:ec2:*::image/ami-*",
      "arn:aws:ec2:<REGION>:<ACCOUNTID>:key-pair/*",
      "<SECURITY_GROUP_ARN>"
    ]
  }

  # these commands will only execute on instances that
  # have a matching Owner tag
  statement {
    effect = "Allow"
    actions = [
      "ec2:StartInstances",
      "ec2:StopInstances",
      "ec2:RebootInstances",
      "ec2:TerminateInstances"
    ]
    condition {
      test     = "StringEquals"
      variable = "ec2:ResourceTag/Owner"
      values = [
        "<RUNNER_INSTANCE_OWNER_NAME>"
      ]
    }
    resources = ["*"]
  }

  statement {
    effect = "Allow"
    actions = [
      "ec2:CreateTags"
    ]
    condition {
      test     = "StringEquals"
      variable = "ec2:CreateAction"
      values = [
        "RunInstances"
      ]
    }
    resources = ["*"]
  }
}

module "oidc_github" {
  source                   = "unfunco/oidc-github/aws"
  version                  = "1.8.0"
  github_repositories      = var.repos
  iam_role_inline_policies = { runner : data.aws_iam_policy_document.runner-user-role-policy.json }
}

which replaced all the IAM user/key stuff I had before. If not using terraform, then the aws-actions/configure-aws-credentials README file has a lot (too much? :)) detail on how to do this. Broadly, you need to create an IAM role encapsulating the permissions you need (the same as the original assumed role you outlined), plus an IAM OIDCProvider which federates with github. Again, the configure-aws-credentials README helpfully provides a CloudFormation script which does this.

I have added an example OIDC workflow job in the new README here, but I haven't provided any info in particular on the detail of how to set this up at the AWS end, beyond pointing to the configure-aws-credentials link above. If you'd like me to add some detail on this, let me know, otherwise please go ahead and add whatever you see fit and you're welcome to use that terraform snippet if it's helpful.

I think what AWS are hoping for is to eventually remove any actual authentication from our own actions, and simply use the output of the configure-aws-credentials action. To tell the truth, if I were writing this from scratch now, that's what I'd probably do. But ... legacy :)

@SemiConscious
Copy link
Contributor Author

SemiConscious commented May 9, 2024

One other thing: testing the new code. I have run your typescript test suite via the new OIDC route, as my setup doesn't have any convenient IAM user I can leverage. When setting up .env, I added a new variable INPUT_AWS_SESSION_TOKEN, and that's where I put the token. We use aws-vault for access to our dev and production environments, and aws-vault export <profile> outputs the values I need.

Setting up .env how you always have should still work however, using your original flow. I'm just suggesting this in case you have something similar set up your end for AWS access as it makes it easy to test with the OIDC flow. To be clear you don't HAVE to have the whole OIDC shenanigans set up in your test environment in order to run your typescript test suite, you just need to have assumed a role that gives you adequate access to your environment, and use the ID/Secret/Token that the assumed role output. All this change does in fact is to assume in the code that someone else has already done an STS::AssumeRole and we're using the outputs from that.

I also had a go at modifying the test workflows to allow oidc to be tested. If you set oidc to true in aws-test-all.yaml, aws-test-on-demand.yaml will invoke configure-aws-credentials and attempt to auth that way. I couldn't figure out how to conveniently run it so it may need some tweaking to get it to work.

Regarding your AWS test environment for running the test workflows: you'll need to add the OIDCProvider and associated role, but you shouldn't need to touch what you already have, you should be able to set up for testing oidc and non-oidc with the same environment.

@SemiConscious
Copy link
Contributor Author

Please let me know if there's anything I can do to support getting this over the line.

@mahdi-torabi
Copy link
Contributor

Thanks. I am gonna be working on merging these changes hopefully later today so hopefully we can have another release later this week. I'll keep you posted.

@mahdi-torabi
Copy link
Contributor

Could you please grant us the permission to push to the OIDC branch on your repo so I can push changes to this PR ?

@SemiConscious
Copy link
Contributor Author

SemiConscious commented May 13, 2024 via email

@mahdi-torabi
Copy link
Contributor

Just saw this - sorry! I have added Mahdi-Torabi1997 as a collaborator on my repo. Let me know if you need anything else

On Mon, 13 May 2024 at 17:51, mahdi-torabi @.> wrote: Could you please grant us the permission to push to the OIDC branch on your repo so I can push changes to this PR ? — Reply to this email directly, view it on GitHub <#13 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANMOPV2C3VK2IDHS5FLJU3ZCDVRPAVCNFSM6AAAAABHOIREXKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBYGIYDMMRSGA . You are receiving this because you authored the thread.Message ID: @.>

I think that's a different user. My GH account is https://github.com/mahdi-torabi

@SemiConscious
Copy link
Contributor Author

Done! Sorry about that.

@mahdi-torabi
Copy link
Contributor

Ok I resolved the conflicts and will merge this into main. I tested the old functionality and it still works as expected but since we are not setup with OIDC I have no way of testing those components.

Is it possible for you to run a GH action build pointing to main branch uses: NextChapterSoftware/ec2-action-builder@main before I push a release ?

If you are too busy or this is taking too much of your time let me know and I'll figure some other way of testing it.

@mahdi-torabi mahdi-torabi merged commit b81e663 into NextChapterSoftware:main May 14, 2024
@SemiConscious
Copy link
Contributor Author

Hi There - no trouble at all. Sorry for the delay, I just don't see the messages sometimes. I ran with NextChapterSoftware/ec2-action-builder@main and it worked fine, here's a screenshot:

image

Copy link
Contributor

Awesome. Thank you very much.

I'll push out a release with these changes in a couple of hours.

@SemiConscious
Copy link
Contributor Author

FYI: Theres a good howto on installing github OIDC on AWS here

If you use terraform it's even easier.

If you want to install it and you're having issues, let me know.

@mahdi-torabi
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants