From 134cde210b1004353c82ec9b10565ee91e6e8e78 Mon Sep 17 00:00:00 2001 From: Shunsuke Suzuki Date: Sat, 25 May 2024 14:34:38 +0900 Subject: [PATCH] feat: support verifying if the commit hash of a semver comment is equivalent to a commit SHA of action version (#439) * feat: support verifying if the commit hash of a semver comment is equivalent to a commit SHA of action version * fix: improve the error log and add document * docs: update document --- README.md | 6 ++ docs/codes/001.md | 33 ++++++++ pkg/cli/run.go | 8 ++ pkg/controller/run/config.go | 1 + pkg/controller/run/parse_line.go | 130 +++++++++++++++++++++++-------- pkg/controller/run/run.go | 3 + testdata/bar.yaml | 15 ++++ 7 files changed, 164 insertions(+), 32 deletions(-) create mode 100644 docs/codes/001.md create mode 100644 testdata/bar.yaml diff --git a/README.md b/README.md index a8c3c15b..16b9051f 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,8 @@ index 84bd67a..5d92e44 100644 permissions: ``` +[pinact also supports verifying version annotations](docs/codes/001.md). + ## Motivation It is a good manner to pin GitHub Actions versions by commit hash. @@ -135,6 +137,10 @@ $ pinact init '.github/pinact.yaml' About the configuration, please see [Configuration](#Configuration). +## Verify version annotations + +Please see [the document](docs/codes/001.md). + ## GitHub Actions https://github.com/suzuki-shunsuke/pinact-action diff --git a/docs/codes/001.md b/docs/codes/001.md new file mode 100644 index 00000000..814916d6 --- /dev/null +++ b/docs/codes/001.md @@ -0,0 +1,33 @@ +# Verify version annotations + +pinact >= v0.1.3 + +Please see the following code. + +```yaml +- uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v3.5.1 +``` + +You would assume the version of the action is v3.5.1 because the version annotation is "v3.5.1". +But the actual version is v2.7.0 because "ee0669bd1cc54295c223e0bb666b733df41de1c5" is the commit hash of v2.7.0. +Please check releases. + +- https://github.com/actions/checkout/releases/tag/v3.5.1 +- https://github.com/actions/checkout/releases/tag/v2.7.0 + +This indicates version annotations aren't necessarily correct. +Especially, attackers can specify a full commit SHA including a malicious code while setting a safe tag to the version annotation. +If a pull request includes changes of GitHub Actions, you should verify version annotations. + +pinact v0.1.3 or newer can verify version annotations using `pinact run`'s `--verify` option. +This verification works only if the version annotation is semver and the version is full commit hash like the above example. +This option gets a full commit hash from a version annotation by GitHub API and compares it with the version. + +e.g. + +```console +$ pinact run --verify testdata/bar.yaml +ERRO[0000] parse a line action=actions/checkout action_version=ee0669bd1cc54295c223e0bb666b733df41de1c5 commit_hash_of_version_annotation=83b7061638ee4956cf7545a6f7efe594e5ad0247 error="verify the version annotation: action_version must be equal to commit_hash_of_version_annotation" help_docs="https://github.com/suzuki-shunsuke/pinact/blob/main/docs/codes/001.md" pinact_version= program=pinact version_annotation=v3.5.1 workflow_file=testdata/bar.yaml +``` + +Note that `--verify` option calls GitHub API to verify version annotations, which may cause API rate limiting. diff --git a/pkg/cli/run.go b/pkg/cli/run.go index ee8d4d12..1ca3a53d 100644 --- a/pkg/cli/run.go +++ b/pkg/cli/run.go @@ -24,6 +24,13 @@ e.g. $ pinact run .github/actions/foo/action.yaml .github/actions/bar/action.yaml `, Action: r.runAction, + Flags: []cli.Flag{ + &cli.BoolFlag{ + Name: "verify", + Aliases: []string{"v"}, + Usage: "verify if pairs of commit SHA and version are correct", + }, + }, } } @@ -38,6 +45,7 @@ func (r *Runner) runAction(c *cli.Context) error { WorkflowFilePaths: c.Args().Slice(), ConfigFilePath: c.String("config"), PWD: pwd, + IsVerify: c.Bool("verify"), } return ctrl.Run(c.Context, r.LogE, param) //nolint:wrapcheck } diff --git a/pkg/controller/run/config.go b/pkg/controller/run/config.go index 32573fa5..95ddd058 100644 --- a/pkg/controller/run/config.go +++ b/pkg/controller/run/config.go @@ -10,6 +10,7 @@ import ( type Config struct { Files []*File IgnoreActions []*IgnoreAction `yaml:"ignore_actions"` + IsVerify bool `yaml:"-"` } type File struct { diff --git a/pkg/controller/run/parse_line.go b/pkg/controller/run/parse_line.go index d17fc03c..474fd45e 100644 --- a/pkg/controller/run/parse_line.go +++ b/pkg/controller/run/parse_line.go @@ -2,6 +2,7 @@ package run import ( "context" + "errors" "fmt" "regexp" "strings" @@ -11,7 +12,12 @@ import ( "github.com/suzuki-shunsuke/pinact/pkg/github" ) -var usesPattern = regexp.MustCompile(`^ +(?:- )?uses: +(.*)@([^ ]+)(?: +# +(?:tag=)?(v\d+[^ ]*))?`) +var ( + usesPattern = regexp.MustCompile(`^ +(?:- )?uses: +(.*)@([^ ]+)(?: +# +(?:tag=)?(v?\d+[^ ]*))?`) + fullCommitSHAPattern = regexp.MustCompile(`\b[0-9a-f]{40}\b`) + semverPattern = regexp.MustCompile(`^v?\d+\.\d+\.\d+[^ ]*$`) + shortTagPattern = regexp.MustCompile(`^v\d+$`) +) type Action struct { Name string @@ -21,6 +27,32 @@ type Action struct { RepoName string } +type VersionType int + +const ( + Semver VersionType = iota + Shortsemver + FullCommitSHA + Empty + Other +) + +func getVersionType(v string) VersionType { + if v == "" { + return Empty + } + if fullCommitSHAPattern.MatchString(v) { + return FullCommitSHA + } + if semverPattern.MatchString(v) { + return Semver + } + if shortTagPattern.MatchString(v) { + return Shortsemver + } + return Other +} + func (c *Controller) parseLine(ctx context.Context, logE *logrus.Entry, line string, cfg *Config) (string, error) { //nolint:cyclop,funlen matches := usesPattern.FindStringSubmatch(line) if matches == nil { @@ -29,9 +61,9 @@ func (c *Controller) parseLine(ctx context.Context, logE *logrus.Entry, line str return line, nil } action := &Action{ - Name: matches[1], + Name: matches[1], // local action is excluded by the regular expression because local action doesn't have version @ Version: matches[2], // full commit hash, main, v3, v3.0.0 - Tag: matches[3], // empty, v1, v3.0.0, hoge + Tag: matches[3], // empty, v1, v3.0.0 } for _, ignoreAction := range cfg.IgnoreActions { @@ -44,11 +76,19 @@ func (c *Controller) parseLine(ctx context.Context, logE *logrus.Entry, line str } } - if f := c.parseAction(action); !f { + if f := c.parseActionName(action); !f { logE.WithField("line", line).Debug("ignore line") return line, nil } - if action.Tag == "" { //nolint:nestif + + switch getVersionType(action.Tag) { + case Empty: + typ := getVersionType(action.Version) + switch typ { + case Shortsemver, Semver: + default: + return line, nil + } // @xxx // Get commit hash from tag // https://docs.github.com/en/rest/git/refs?apiVersion=2022-11-28#get-a-reference @@ -59,7 +99,7 @@ func (c *Controller) parseLine(ctx context.Context, logE *logrus.Entry, line str return line, nil } longVersion := action.Version - if shortTagPattern.MatchString(action.Version) { + if typ == Shortsemver { v, err := c.getLongVersionFromSHA(ctx, action, sha) if err != nil { return "", err @@ -70,23 +110,39 @@ func (c *Controller) parseLine(ctx context.Context, logE *logrus.Entry, line str } // @yyy # longVersion return c.patchLine(line, action, sha, longVersion), nil - } - // @xxx # v3 - // list releases - // extract releases by commit hash - if !shortTagPattern.MatchString(action.Tag) { - logE.WithField("action_version", action.Version).Debug("ignore the line because the tag is not short") + case Semver: + // verify commit hash + if !cfg.IsVerify { + return line, nil + } + // @xxx # v3.0.0 + // @ # v3.0.0 + if FullCommitSHA != getVersionType(action.Version) { + return line, nil + } + if err := c.verify(ctx, action); err != nil { + return "", fmt.Errorf("verify the version annotation: %w", err) + } return line, nil - } - longVersion, err := c.getLongVersionFromSHA(ctx, action, action.Version) - if err != nil { - return "", err - } - if longVersion == "" { - logE.Debug("failed to get a long tag") + case Shortsemver: + // @xxx # v3 + // @ # v3 + if FullCommitSHA != getVersionType(action.Version) { + return line, nil + } + // replace Shortsemer to Semver + longVersion, err := c.getLongVersionFromSHA(ctx, action, action.Version) + if err != nil { + return "", err + } + if longVersion == "" { + logE.Debug("failed to get a long tag") + return line, nil + } + return c.patchLine(line, action, action.Version, longVersion), nil + default: return line, nil } - return c.patchLine(line, action, action.Version, longVersion), nil } func (c *Controller) patchLine(line string, action *Action, version, tag string) string { @@ -105,7 +161,7 @@ func (c *Controller) getLongVersionFromSHA(ctx context.Context, action *Action, } // Get long tag from commit hash for range 10 { - tags, _, err := c.repositoriesService.ListTags(ctx, action.RepoOwner, action.RepoName, opts) + tags, resp, err := c.repositoriesService.ListTags(ctx, action.RepoOwner, action.RepoName, opts) if err != nil { return "", fmt.Errorf("list tags: %w", err) } @@ -127,19 +183,17 @@ func (c *Controller) getLongVersionFromSHA(ctx context.Context, action *Action, return tagName, nil } } - if len(tags) < opts.PerPage { + if resp.NextPage == 0 { return "", nil } - opts.Page++ + opts.Page = resp.NextPage } return "", nil } -var shortTagPattern = regexp.MustCompile(`^v\d+$`) - -// parseAction returns true if the action is a target. +// parseActionName returns true if the action is a target. // Otherwise, it returns false. -func (c *Controller) parseAction(action *Action) bool { +func (c *Controller) parseActionName(action *Action) bool { a := strings.Split(action.Name, "/") if len(a) == 1 { // If it fails to extract the repository owner and name, ignore the action. @@ -147,10 +201,22 @@ func (c *Controller) parseAction(action *Action) bool { } action.RepoOwner = a[0] action.RepoName = a[1] - if action.Tag != "" && !shortTagPattern.MatchString(action.Tag) { - // Ignore if the tag is not a short tag. - // e.g. uses: actions/checkout@xxx # v2.0.0 - return false - } return true } + +func (c *Controller) verify(ctx context.Context, action *Action) error { + sha, _, err := c.repositoriesService.GetCommitSHA1(ctx, action.RepoOwner, action.RepoName, action.Tag, "") + if err != nil { + return fmt.Errorf("get a commit hash: %w", err) + } + if action.Version == sha { + return nil + } + return logerr.WithFields(errors.New("action_version must be equal to commit_hash_of_version_annotation"), logrus.Fields{ //nolint:wrapcheck + "action": action.Name, + "action_version": action.Version, + "version_annotation": action.Tag, + "commit_hash_of_version_annotation": sha, + "help_docs": "https://github.com/suzuki-shunsuke/pinact/blob/main/docs/codes/001.md", + }) +} diff --git a/pkg/controller/run/run.go b/pkg/controller/run/run.go index a2f56aca..d7962157 100644 --- a/pkg/controller/run/run.go +++ b/pkg/controller/run/run.go @@ -15,6 +15,7 @@ type ParamRun struct { WorkflowFilePaths []string ConfigFilePath string PWD string + IsVerify bool } func (c *Controller) Run(ctx context.Context, logE *logrus.Entry, param *ParamRun) error { @@ -22,10 +23,12 @@ func (c *Controller) Run(ctx context.Context, logE *logrus.Entry, param *ParamRu if err := c.readConfig(param.ConfigFilePath, cfg); err != nil { return err } + cfg.IsVerify = param.IsVerify workflowFilePaths, err := c.searchFiles(logE, param.WorkflowFilePaths, cfg, param.PWD) if err != nil { return fmt.Errorf("search target files: %w", err) } + for _, workflowFilePath := range workflowFilePaths { logE := logE.WithField("workflow_file", workflowFilePath) if err := c.runWorkflow(ctx, logE, workflowFilePath, cfg); err != nil { diff --git a/testdata/bar.yaml b/testdata/bar.yaml new file mode 100644 index 00000000..5ef994c9 --- /dev/null +++ b/testdata/bar.yaml @@ -0,0 +1,15 @@ +--- +name: bar +on: workflow_call +jobs: + integration-test: + runs-on: ubuntu-latest + permissions: {} + steps: + # The version annotation is "v3.5.1", so you would think the version of the action is v3.5.1. + # But the actual version is v2.7.0 because "ee0669bd1cc54295c223e0bb666b733df41de1c5" is the commit hash of v2.7.0. + # https://github.com/actions/checkout/releases/tag/v3.5.1 + # https://github.com/actions/checkout/releases/tag/v2.7.0 + # This means version annotations aren't necessarily correct. + # pinact run's --verify option verifies version annoations. + - uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v3.5.1