Skip to content

Conversation

@eregon
Copy link
Contributor

@eregon eregon commented Apr 1, 2020

This PR adds a starter workflow for ruby/setup-ruby, the most complete action to setup Ruby. This action is hosted in the official GitHub Ruby organization.

I'm doing this PR based on this comment by @ethomson : actions/setup-ruby#49 (comment)

  • Include a good description of the workflow.
  • Links to the language or tool will be nice (unless its really obvious)

In the workflow and properties files:

  • Includes a matching ci/properties/*.properties.json file.
  • Use title case for the names of workflows and steps, for example "Run tests".
  • The name of CI workflows should only be the name of the language or platform: for example "Go" (not "Go CI" or "Go Build")

Should it just be Ruby here?
But then we would have two starter workflows with the title Ruby which seems confusing.

I followed a similar structure to ci/node.js.yml, and used the matrix form to show how to test on multiple Rubies, including alternative implementations, which actions/setup-ruby cannot do currently.

  • Include comments in the workflow for any parts that are not obvious or could use clarification.
    I think the commands are obvious and need no comment.
  • CI workflows should run push.

Some general notes:

  • Does not use an Action that isn't in the actions organization.

Since it was suggested in actions/setup-ruby#49 (comment) this was OK, I suppose it doesn't apply anymore.

  • Does not send data to any 3rd party service except for the purposes of installing dependencies.
    The action only downloads prebuilt Ruby hosted as GitHub Releases.
  • Does not use a paid service or product.

@eregon
Copy link
Contributor Author

eregon commented Apr 1, 2020

I would be interested to see a rendered version of this before merging if that's possible.

@ethomson
Copy link
Contributor

ethomson commented Apr 3, 2020

This adds a new starter workflow template - and I feel like if we have two very similar ruby templates then we need to provide a lot of indication about when to use which.

Instead of adding a second one, perhaps we should just make the existing ruby template(s) use the ruby/setup-ruby action?

/cc @chrispat to sanity check me.

@eregon
Copy link
Contributor Author

eregon commented Apr 3, 2020

Instead of adding a second one, perhaps we should just make the existing ruby template(s) use the ruby/setup-ruby action?

That would be fine by me and sounds even better.

@chrispat
Copy link
Member

chrispat commented Apr 3, 2020

Yes I think that makes sense. I don't see a reason to have two different workflows and I also expect that we will deprecate actions/setup-ruby.

@eregon eregon force-pushed the add-ruby-setup-ruby branch from 1dcfb2a to 391f084 Compare April 3, 2020 18:28
Copy link
Contributor Author

@eregon eregon left a comment

Choose a reason for hiding this comment

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

I updated the PR to change the existing Ruby workflow to use ruby/setup-ruby, please review.

I could use a matrix-style workflow (like node.js.yml) but I guess a single job is maybe easier to get started and so maybe more adapted for starter-workflows.

@ethomson
Copy link
Contributor

ethomson commented Apr 6, 2020

Cool, thanks @eregon. I have some comments to keep things more consistent with the rest of the workflows. Having things the same across all of them simplifies our handling of them.

If you could also put the following comment at the top of the YAML:

# This workflow uses actions that are not certified by GitHub.
# They are provided by a third-party and are governed by
# separate terms of service, privacy policy, and support
# documentation.

ci/ruby.yml Outdated
- uses: actions/checkout@v2
- name: Set up Ruby 2.6
uses: actions/setup-ruby@v1
- uses: ruby/setup-ruby@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pin this to the v1 hash please (using the full 40 characters)? This is going to be a requirement for using third-party actions in a starter workflow template.

Copy link
Contributor Author

@eregon eregon Apr 6, 2020

Choose a reason for hiding this comment

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

Using the full hash or a specific release like v1.31.0 is not going to work though, for the same reasons none of the other starter workflows use an hardcoded version number.
That would prevent users of that action to get bug fixes (e.g. bugs in the action or at some point GH Actions set CLASSPATH which broke JRuby, it's good if we can workaround rather than fail every usage of the workflow using JRuby) and new (compatible) features.
Notably new Ruby versions are considered new features in terms of versioning for ruby/setup-ruby.

So I think we really need ruby/setup-ruby@v1 here.

BTW, https://github.com/actions/toolkit/blob/master/docs/action-versioning.md mentions:

- uses: actions/javascript-action@v1 # recommended. starter workflows use this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I also noticed in this repo that for example ci/aws.yml uses aws-actions/amazon-ecr-login@v1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the full hash or a specific release like v1.31.0 is not going to work though, for the same reasons none of the other starter workflows use an hardcoded version number.

Yes, I understand the tradeoff here. Our policy is to use the full hash for third-party workflows that don't come from partners because they're immutable.

We're working on other solutions, but for now, this needs to be the full hash. Users are welcome to adopt a different mechanism (using a tag) if they decide to trust that.

Copy link
Contributor Author

@eregon eregon Apr 6, 2020

Choose a reason for hiding this comment

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

I think it's needed to trust the ruby organization or action maintainers here.
From #451

This workflow must only use actions that are produced by the language or ecosystem that the workflow supports.

I think that's a good step for security. For instance only myself, @ioquatix and ruby org owners have write access to the ruby/setup-ruby repository.

Also, ruby/setup-ruby doesn't require any secret and so AFAIK there is no secret data leak possible, isn't it? Can you elaborate on your security concerns for this action?

I think almost any action maintainer you will ask will say it's unreasonable to pin to a sha like this.
In other words, using a sha is unsupported for ruby/setup-ruby and I would think as well for most actions out there.
Using a specific release is only supported for a short while if there is an issue with v1.
So ruby/setup-ruby@v1 is the only supported form, because it's the only one that can provide bug fixes and adapt to changes in GitHub Actions (e.g., when GitHub Actions adds a MSYS2 installation on Windows).

If you absolutely want a sha here, then I see only 2 ways forward:

  • I make a PR on every release of ruby/setup-ruby to update the sha here. There are currently 40 releases, so it's very frequent, about once a week. I think this will be very annoying for both me and the maintainers of this repository.
  • Add a comment on top of that sha saying:
# this is unsupported, use ruby/setup-ruby@v1 instead

which of course feels silly.

Another problem, the rendered documentation would be often out of sync, which is bad for user experience.

I can tell you from experience Ruby users want updates, and want access to new Ruby versions, and that requires updating the action, and make new builds for those new versions. Also sometimes the build flags changed, so that uses a new release of the action (and so older versions still use the older builds which is good if there is a problem).

In fact, pinning to a sha would prevent something like ruby-version: 2.6 to get the new security release like Ruby 2.6.6. So now we have conflicting security concerns.


To summarize, I think GitHub needs to rethink this full sha requirement for 3rd party actions, because it simply cannot work for any maintained action. I think the existing requirements (repository inside the organization of the language, disclaimer on top of the workflow) are enough.

Copy link

@ioquatix ioquatix Apr 7, 2020

Choose a reason for hiding this comment

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

Pinning to a full (immutable) version would [be] good for security...

To be honest, I think you are expecting too much for just pinning to a specific commit. If an account is hijacked, there are simply too many other ways to take advantage of it. In addition, that doesn't even consider that someone like myself could be coerced (e.g. under duress by government or some other crazy scenario). So, it doesn't even take that much.

The only way to make GitHub actions truly secure against manipulations supporting infrastructure/code/resources:

  • Checkout all referenced packages and build a cache of all required files (including 3rd party packages like gems).
  • Checksum those files using a cryptographically secure hash.
  • Invite the user to review those files (good luck with that).
  • Save that cache for all future test runs.
  • Provide a function to rebuild the cache, prompting users to review any differences.

Pinning to a SHA is like putting a bandaid on a broken leg and it comes across as naive. I respect everyone is trying to do their best here... but can we all agree that pinning to a specific SHA doesn't really buy any increased security?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, indeed I meant as it "might be better for security in general, from some point of view". But for ruby/setup-ruby it seems currently mostly pointless regarding security because the prebuilt Rubies could change without changing the sha of the action (and they need to for *-head builds as those are automatically updated daily).

@ethomson Given that, do you think it still makes sense to enforce a full sha for ruby/setup-ruby? That means many troubles for both users and maintainers, and very little security as described in the last comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ethomson Given that, do you think it still makes sense to enforce a full sha for ruby/setup-ruby? That means many troubles for both users and maintainers, and very little security as described in the last comments.

I feel like there's a few different conversations going on here: ergonomics, quality, and security. I think that it makes sense to separate them.

First, the ergonomics. Pinning to a 40 character hash is ugly. There's no doubt about it. We all agree, and I mention it only for completeness.

Second, the quality aspects of pinning to a hash. Yes, using a moving tag supports automatic updates, which is nice for users in many ways. However, I want to caution that you cannot assume that people will be doing this. Many users explicitly do not want this. It's particularly common in enterprise situations.

When GitHub Actions ships in GitHub Enterprise Server it will support working fully disconnected from the internet. Administrators will install particular versions of actions, and there is no automatic update path there. (Many actions will need to be modified to support this way of working, if they want to support GHES, including yours.)

But even GitHub Enterprise Cloud, and non-enterprise users of github.com want and expect to be able to pin to a sha. This is especially true of people who are doing deploys to their infrastructure, and have secrets in their workflows. People do review their actions and pin to them, and people have even created tools to streamline the process.

I mention this to underscore that people will do this and they need to be supported. We'll improve the ergonomics for them by moving publishing out of a git repository, so that full version numbers are immutable. But this is a thing that people are going to do.

And finally, security. I agree that sha pinning does not prevent other vulnerabilities. If - for example - somebody take's over the Java download site and sneaks a vulnerability into the JDK download, then pinning to a SHA doesn't really help that. I think this is what @ioquatix meant with the bandaid on a broken leg analogy.

But:

  1. Everybody who downloads the JDK will get that vulnerability. It's not an attack unique to GitHub Actions.
  2. We don't have control over that. We trust Oracle to do their best to maintain a security posture for their downloads, just like our customers trust us to do the same.

I know that you're disappointed, but the fact that other theoretical security vulnerabilities exist doesn't mean that we should ignore things that provide improvements. We're all in this together, trying to build a secure ecosystem. We trust the Ruby maintainers to secure your side of this; but we need to secure our side as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added documentation regarding choosing a version in https://github.com/ruby/setup-ruby#versioning, and updated this PR to use a full sha with a comment on top.

I still think the full sha is very suboptimal, especially for a starter workflow (it's IMHO something newcomers to GitHub Actions shouldn't be bothered with, and it's a general consideration for GitHub Actions which should be documented but clearly here is not a great place), but let's try it.
If it doesn't work well, we'll need to reconsider, which might lead to changing the phrasing here, asking to use @v1 here, or -- worse case -- asking to remove this starter workflow. Hopefully people using a full sha will be responsible to update it whenever needed and not report issues for old versions which nobody can fix anyway.

Regarding working offline I'm somewhat doubtful how that would work with ruby/setup-ruby, if the company has private runners they probably want to build the ruby they want in the image anyway. That's something to consider when people ask for it and probably not much before. On a related note, is there a way to detect if running on a standard runner such as GitHub's runners?

Copy link

Choose a reason for hiding this comment

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

I mention this to underscore that people will do this and they need to be supported. We'll improve the ergonomics for them by moving publishing out of a git repository, so that full version numbers are immutable. But this is a thing that people are going to do.

@ethomson With this logic, why aren't the starter workflows in actions/ pinning to a sha instead of v1?

@eregon eregon force-pushed the add-ruby-setup-ruby branch from 391f084 to 67ffe1e Compare April 6, 2020 16:14
@eregon
Copy link
Contributor Author

eregon commented Apr 6, 2020

@ethomson Thanks for your feedback, I think I addressed all of it, except regarding @v1=>sha (see discussion there).

@eregon
Copy link
Contributor Author

eregon commented Apr 9, 2020

@ethomson Ready for a final review.

@eregon eregon force-pushed the add-ruby-setup-ruby branch from 8252f89 to 9dd8f4e Compare April 10, 2020 21:10
@ethomson
Copy link
Contributor

Thanks @eregon - this looks great to me. We really appreciate all the work that you've done, and we'll continue to review our processes and policies here in the hopes that we can make things easier for both you, the action authors, and our shared users.

@chrispat would you care to give this a final review?

@eregon
Copy link
Contributor Author

eregon commented Apr 16, 2020

Let's merge this?

@ethomson
Copy link
Contributor

Thanks again for the work on this @eregon. We update the templates based on this repository automatically, so this will appear as the new starter workflow for Ruby within a few days.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants