search

slsa-framework / slsa-verifier

Verify provenance from SLSA compliant builders
Apache License 2.0
229 stars 49 forks source link

Installer Action Node16 Deprecation Notice #806

Closed IAreKyleW00t closed 1 month ago

IAreKyleW00t commented 2 months ago

I noticed that the installer Action is still using Node16 and throws a warning at users since it has been deprecated. Would it be possible to bump this to Node20? Looking at the code, I don't think you're doing anything funky that would be a problem with an upgrade.

Side-note, but somewhat related, I noticed the Action doesn't support other platforms even though slsa-verifier does, and is a bit dated in general. I would be happy to volunteer to give it some love and have things align with some newer standards/practices. 😄

ramonpetgrave64 commented 2 months ago

Yes, we would welcome a PR. By other platforms, do you mean other OSs?

IAreKyleW00t commented 2 months ago

@ramonpetgrave64 - Yes, I mean support for Windows and macOS. I see binaries are built for these platforms so I figured it would make sense to have the Action also support them.

ramonpetgrave64 commented 2 months ago

Alright, it could take a bit of work to get it to work for other OSs, but the node20 upgrade seems simple enough.

You're welcome to test it on your fork and submit a PR. I'll be happy to merge.

IAreKyleW00t commented 2 months ago

The Node20 upgrades had no issues (didn't even make changes to dist/ lol), but looking through the code I noticed that the tool-cache is not actually being caching properly. tc.cacheFile() (or tc.cacheDir()) is never being called, which is what will actually cache the tool for later. There needs to be additional logic to check and use the cache with tc.find().

Is this intentional? I can address this as well with the OS support.

Also the current Action appears to be using the $GITHUB_ACTION_REF as the version for slsa-verifier that it uses. While this works, it would prevent someone from being able to use/access older versions of the slsa-verifier. Is seems like there was a conscious decision around this (#246), but I'm wondering if it's worth revisiting a version input to allow for users to specify what release they want installed (defaulting to the latest, or a "known good an stable" version).

ramonpetgrave64 commented 2 months ago

Thanks for testing it out.

I can't say for sure if it's intentional. But looking at Github's example, it seems like tc.downloadTool() is being used properly, and the other method calls aren't necessary. Does this sound right?

Regarding the ref, it will actually work if the user wants to install older versions of slsa-verifier.

Users can still pin to a release tag if they want. The version input is optional, and pinning to a release tag will install whichever version was latest at the time of release (i.e. @1.2.3 will install v1.2.3).

IAreKyleW00t commented 2 months ago

I can't say for sure if it's intentional. But looking at Github's example, it seems like tc.downloadTool() is being used properly, and the other method calls aren't necessary. Does this sound right?

This will only download the files to the location you specify but not actually cache them. They will only be moved into the hostedtoolcache once you call the tc.cacheFile() or tc.cacheDir() - the example shown does not actually utilize caching, it's only using the @actions/tool-cache package to perform the download. The docs for the package itself mention needing to use this. https://github.com/actions/toolkit/tree/main/packages/tool-cache#cache

The developer needs to use tc.find() to determine if the cache was hit and if re-downloading needs to be done. tc. downloadTool() will always download the tool if it's called.

let cachePath = await tc.find(...) // returns path if found
if (!cachePath)
  // download artifact
  const downloadPath = await tc.downloadTool(...)

  // verify download
  // ...

  // cache download for future use
  // eg: /opt/hostedtoolcache/slsa-verifier/2.6.0/x64/slsa-verifier
  cachePath = await tc.cacheFile(...)  // returns path to cached directory
}
core.addPath(cachePath)

Regarding the ref, it will actually work if the user wants to install older versions of slsa-verifier.

This is sort of true, but would not be the case for the new OSes we're adding support for and any of the additional fixes to the Action itself. Using something like @v1.2.3 would still work, but it would use the Action as it was made at tag v1.2.3, meaning they would still get an older Action, Node version, etc. This could cause some inconsistent behavior and I wasn't sure if this was desirable.

eg: If someone wanted to use @v2.5.1 to install SLSA Verifier for Windows it would fail because the Action at that tag does not support Windows.

This is where I think a version input could be useful, where the default could be to use the GITHUB_ACTION_REF like you describe, but also allow for that version of the Action to be used to install other versions of the SLSA Verifier if needed.

ramonpetgrave64 commented 2 months ago

Are you sure those are not just for self-hosted runners? What I'd like to only ever cache from the direct download link.

Thanks for clarifying about the ref. I'll invite @ianlewis to discuss.

IAreKyleW00t commented 2 months ago

Based on my experience and testing, even on GH runners you will need to tell it to actually cache the files. There may be other things behind the scenes that GH does to speed up repeated downloads, but I don't believe this is part of the @actions/tool-cache package based on the code. It looks like it will always write out to a file from the Stream, which implies (to me) that's it's downloading it every time.

The debug logs during the tc.cacheFile() indicate that this is the point at which it will cache a file(s)

##[debug]Caching tool slsa-verifier 2.6.0 x64
##[debug]source dir: /tmp/slsa-verifier_3ZrkE0/v2.6.0
##[debug]destination /opt/hostedtoolcache/slsa-verifier/2.6.0/x64
##[debug]finished caching tool

The debug logs for tc.downloadTool() show that it downloads the file each time.

##[debug]Downloading https://github.com/slsa-framework/slsa-verifier/releases/download/v2.6.0/slsa-verifier-linux-amd64
##[debug]Destination /tmp/slsa-verifier_3ZrkE0/v.2.6.0/slsa-verifier
##[debug]download complete

Unless there is something else I'm missing I think this always needs to be explicitly called, but I don't think adding some logic to account for this is hard to do anyways. I've actually got all this added and tested in my fork, but I can easily yoink stuff out!

ramonpetgrave64 commented 2 months ago

Since performance seems perfectly fine with the cache not actually working, like you said, I'm for removing the cache code. I've also read some blogs about cache poisoning, so it seems like it's not worth the trouble.

IAreKyleW00t commented 2 months ago

I think you're correct that performance is probably fine without it, but I think it would be nice to utilize it, especially for people who do use this on self-hosted runners (such as myself). Cache poisoning is definitely a good point though and not something I accounted for in my fork.

I think a good balance for this would be to cache the binary, but not cache the signature data and always validate the binary regardless of being cached or not (including for the bootstrap binary). This should ensure that any tampering would be noticed by the user. This would at least reduce the total bandwidth necessary when a cache-hit does occur.

Another option (which I have now) is to simply give the user a cache input option to override the behavior to avoid or accept the risks.

ianlewis commented 2 months ago

We could verify the provenance of the slsa-verifier binary that is retrieved from the cache (we do this anyway after download). This would also verify that it was the expected slsa-verifier version (preventing downgrade attacks etc).

However, as stated, it's likely just downloading it each time has adequate performance. If tc.downloadTool doesn't actually cache then I've actually never seen another action that utilizes the cache properly as I've only seen actions that simply call tc.downloadTool (e.g. the golangci-lint installer)

IAreKyleW00t commented 2 months ago

Verifying the bin in cache is something I can add in and probably something I should have started with haha

I'm not sure what else to say about the tool-cache - that repo is also not properly caching downloads since it is not calling the tc.cacheDir() or tc.cacheFile(), which again is what actually caches the files into the hostedtoolcache directory. The tc.downloadTool() function is simply a utility function to download a file from a URL. GH docs on this are rather poor since they essentially gloss over this details, but reviewing the code I genuinely don't think it will cache files just by using downloadTool alone...

https://github.com/actions/toolkit/blob/main/packages/tool-cache/src/tool-cache.ts#L410 https://github.com/actions/toolkit/blob/main/packages/tool-cache/src/tool-cache.ts#L113

I updated my fork to deliberately skip the cache even if it's found to trigger the exact same tc.downloadTool() call on the same URL on the same runner, and in the debug logs I see that it downloaded it both times.

I think it's fine to just discard the caching entirely if that's the route we want to go

ramonpetgrave64 commented 2 months ago

@IAreKyleW00t

I agree the docs are not well written. I had originally hoped that it was caching the files on github's servers. But looking at the code it seems like tc.downloadTool() only actually downloads, and nothing else.

I think we can should leave our use of tc as is, unless you've already figured out the cache verification problem, then we can take a look.

IAreKyleW00t commented 1 month ago

@ramonpetgrave64 Any chance someone is going to look at that PR? Happy to make adjustments or reduce the scope of needed.

ramonpetgrave64 commented 1 month ago

@IAreKyleW00t , I've been away for a w while, and I've just taken a look at your PR. Yes, please reduce the scope of the PR for now.

IAreKyleW00t commented 1 month ago

Sure thing, though I've lost interest in this and don't actually need those additional features or OSes for my needs, so I'll get it updated to Node20 and we can just move on.

811 is created with only the Node20 changes.

IAreKyleW00t commented 1 month ago

@ramonpetgrave64 Before we close this out, since I have the other work already done, how would it be best to get those changes in? Do you want me to split up the changes into smaller PRs and just open them one at a time?

ramonpetgrave64 commented 1 month ago

@IAreKyleW00t Yes, and we really should have opened separate Issues for the other concerns. Would you open them or edit this issue's title and description?

IAreKyleW00t commented 1 month ago

@ramonpetgrave64 Yes, I can open separate issues for these features/fixes since this was originally just for Node20. I'll split up the changes and open issues to address them as I'm ready