green-coding-solutions / eco-ci-energy-estimation

Eco CI Energy estimation for Github Actions Runner VMs
https://metrics.green-coding.io/ci-index.html
MIT License
55 stars 16 forks source link

Update README.md #4

Closed rajbos closed 1 year ago

rajbos commented 1 year ago

Add missing info, syntax highlighting and some best practices for using properly versioned actions, instead of using main (which can break users when breaking changes are introduced).

Also indicated the data being uploaded, since that was not mentioned before.

ArneTR commented 1 year ago

Hey Hey @rajbos ,

thanks again for a very nice contribution. It always helps tremendoulsly to get some input on docs from a user for us.

I totally agree on all the textual / stylstic changes and also that referencing @main has drawbacks.

Why did you choose for "green-coding-berlin/eco-ci-energy-estimation@5ab20ee5329ae02c598a3b967f58121a4d9d8287" and not for something like "green-coding-berlin/eco-ci-energy-estimation@v1"

To my understanding the latter is the standard. It will get you all minor version updates, but not breaking changes which come with bumping the major version number ...?

rajbos commented 1 year ago

Happy to help out!

SHA hashes is the only way to use GitHub Actions with security in mind, something I've been telling everyone to use for a couple of years now, since every single demo skips this. More information in this blogpost of mine (the first link in that posts also links to a session I had on GitHub Universe in 2021 on this topic). Version numbers are just git tags, which can be changed without you knowing (as a user).

When using commit SHA's, you can still get updates from Dependabot since it understands this syntax. The nice thing then is that you get a notification of an incoming update (so you are at least aware) and can then review the incoming deltas.

Second issue with using version numbers is that the hones to properly implement SemVer to the Action is with the maintainer: If a user specifies v1 and you publish v1.1: the user will still use v1, and not the update. You will need to delete and retag the latest version with v1 for them to pull in any updates. Something which I described in this blogpost.

ArneTR commented 1 year ago

Wow, this is really a tricky one.

But I have a hard time deciding what is the best way to go for the action at the moment, because I believe most people expect that when the include the action with @v1 they get all minor version updates.

So using @main would be actually what most people most likely wanna have. If people are really security concerned they probably know about your preferred method of tagging with a commit hash. So I do not want to force that on everybody ...

@dan-mm Can you please give this a read and also your opinion? I am a bit torn here.

ceddlyburge commented 1 year ago

Hi There, I'm commenting here on ArneTRs request after creating an issue.

I feel a bit uncomfortable using @main. It's not idiomatic for GitHub actions, and it seems like a bit of a security issue. The action has full access to the build, including potentially sensitive things , such as tokens in environment variables. A commit getting in to main feels like a much easier thing for a hacker to achieve than a release / tag, and a change in my workflow to use the new version.

It isn't a problem for the thing I'm doing at the moment though, as there is nothing sensitive in it

rajbos commented 1 year ago

@ceddlyburge , if you can get a commit into the repo, then you can also get a tag into the repo, since that uses the same authentication. Releases are not used at runtime (at the moment), but only for showing things on the marketplace, more info in my blogpost here.

If you want to dive into security measures you need to take as an Actions user, check my session on GitHub Universe 2021 to gain more info.

Hence the secure option here: use a commit hash and the version in the comments: that is a setup that Dependabot also understands and will keep the update in sync for you. Then you can review the incoming changes.

ceddlyburge commented 1 year ago

Hi Rob. I guess I was thinking that it would be easier to get a PR accepted, with a vulnerability in it, than it would be to create a tag. Also, if I use the version number in my workflow yml files, the hacker has to change this, as well as the github action repository. I am neither a security not a github actions expert, but it still seems to be that using @main (which is what I was advised to do) is non idiomatic and less secure. You seem to be proposing something else though, using a commit hash. This seems like it would address my concerns, so maybe I should look in to that. Cheers, Cedd

ArneTR commented 1 year ago

@dan-mm Can you update one of our workflows to integrate eco-ci-energy-estimation with a SHA hash and then also putting Depandabot in to monitor for updates? @djesic can help with configuring Dependabot

I would really like to see how that behaves as we can then maybe pull this PR in as a "safer" integration route.

dan-mm commented 1 year ago

Hi - I'm closing this PR as I've made the changes from this ReadMe already in the main branch.

We now use the commit hash + comment with tag number method in our Readme