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

Enhance v1 with the ability to select a distribution #147

Closed
wants to merge 94 commits into from

Conversation

HanSolo
Copy link

@HanSolo HanSolo commented Mar 31, 2021

Description:
Instead of using only the Zulu distribution, this PR makes use of the foojay discoapi to select from different distributions (supported are AdoptOpenJDK, Corretto, Dragonwell, Liberica, OJDK Build, Oracle OpenJDK, SAP Machine, Trava and Zulu). To choose a distribution you can use the parameter "distro" to specify the distribution. If no distribution is provided it will fall back to Zulu which is the default at the current v1.
e.g.

steps:
- uses: actions/checkout@v2
- uses: actions/setup-java@v1
  with:
    java-version: '15.0.1' # The JDK version to make available on the path.
    java-package: jdk # (jre, jdk, or jdk+fx) - defaults to jdk
    architecture: x64 # (x64 or x86) - defaults to x64
    distro: aoj # The distribution you would like to use
- run: java -cp java HelloWorldApp

We run a Disco Testing Matrix that checks if the specified distributions are available via the Disco API to make sure it works as expected.

Related issue:
Add support for multiple distributions via the Disco API

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

giltene and others added 30 commits January 9, 2020 00:45
Update examples to use actions/checkout@v2
Update REAME and action.yml to describe java-version syntax options
Bumps [acorn](https://github.com/acornjs/acorn) from 5.7.3 to 5.7.4.
- [Release notes](https://github.com/acornjs/acorn/releases)
- [Commits](acornjs/acorn@5.7.3...5.7.4)

Signed-off-by: dependabot[bot] <support@github.com>
…orn-5.7.4

Bump acorn from 5.7.3 to 5.7.4
…tions/http-client-1.0.8

Bump @actions/http-client from 1.0.6 to 1.0.8
This allows calling the action multiple times in the
same job and retrieving the path and/or version in
other steps.

Fixes actions#65
Add output parameters for the tool path and version
The extendedJavaHome environment variable contains `.` symbols in the version. This causes the environment variable to be ignored by the action runner. It's best to replace those and other non standard symbols with and underscore.

For reference:
> Environment variable names used by the utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase letters, digits, and the '_' (underscore) from the characters defined in Portable Character Set .
Normalize extendedJavaHome environment variable
@konradpabjan
Copy link
Collaborator

Thanks for putting out this PR @HanSolo

I'll be frank, adding support for all these distributions would be amazing and users would love it ❤️ However we can't currently use the Disco API.

My comment in the linked issue touches on this: #142 (comment)

I do have have some security concerns too that might make this a blocker (following up on this). Using the Disco API introduces a single point of failure that if compromised could for example lead to malicious binaries being downloaded. Given that the Disco API is outside of the control of GitHub and the attack surface this could be very concerning for a lot of our customers (Enterprise in particular). The API is also fairly new (~3 months).

The main concern from GitHubs perspective is security. We're releasing v2 on Monday April 5th and that only supports Zulu and Adopt distributions by directly using the APIs provided by those distributions. We don't want to introduce a dependency or single point of failure which we absolutely do not need to. We currently do not want to use anything else which is why we explicitly opted for initially supporting just 2 distributions (the biggest ask from users that we've seen is also for adopt). It's more tedious and there is more maintenance to use the APIs directly, but that's what we've decided to currently use for the sake of security.

I few other things:

  • v1 is effectively in maintenance mode and only security fixes or crucial fixes will be back-ported
  • With v2 we're introducing a breaking change with the new distribution input. That will now be required while with these changes there would a level of favoritism towards a single distribution which we're trying to get away from

The work in this PR is amazing and I'm wouldn't want it to go to waste (I'm sure it took a bit of time) so I highly recommend advertising your fork in some of the issues that users are requesting support for more distributions.

@giltene
Copy link
Contributor

giltene commented Mar 31, 2021

...We're releasing v2 on Monday April 5th ...

Separate from the valid discussion of security (for which I think Disco can provide ample answers and address as needed as part of evolving this PR), I am surprised at the notion of releasing v2 on April 5th. I've seen no discussion of such an near-term schedule for what will enshrine a set of new breaking changes (that will be hard to "break" again if they aren't quite baked yet). Moving 'main' over to v2-preview was discussed in #137 (where I separately noted that it seems to early to do that) but that is a far cry from actually releasing v2...

v2 needs more time to bake. More user feedback on v2-preview, based on actual attempted uses by actual repos, is a big part of that, and v2-preview has not been available long enough to solicit such feedback. For example, there has not yet been an OpenJDK quarterly update cycle that would reveal the impact of some of the issues being debated about some of its proposed behaviors.

A variant of this PR base don v2-preview would probably be a good way to discuss the actual support of multiple distros (which was the main goal stated for the v2 ADR).

Since this is not a breaking change to v1, and it addresses a set of popularly requested distro support, I see no reason not to consider this for v1 as well.

@giltene
Copy link
Contributor

giltene commented Mar 31, 2021

The security questions are valid ones (e.g. why would one trust the distro binaries to which the disco API points to?), but it is worth noting using the Disco API does not introduce any more points-of-depedency that are outside of GitHub or setup-java control than already exist in setup-java@v1 or in the proposed setup-java@v2. E.g. the same questions can [and should?] be asked about the binaries currently being pulled from CDNs, or expected too be puled from the API-located URI endpoints in v2-preview. The proper answer to such trust questions is invariably "check the signature for the distro binary you got against the actual binary, and verify that it was signed by a trusted source".

E.g. Zulu CDNs already provide sig files for binary bundles for this very purpose (those sig files are not currently used by setup-java@v1 to validate downloaded distro binaries, but we can certainly add verification as an enhancement). Other distros (e.g. Corretto) do so as well. If setup-java (or it's users) are concerned about the authenticity of JDK distro binaries and want to ensure they were not tamperted with, and that they were not somehow redirected to use a malicious distro binary, the proper way to overcome that concern is to validate the binaries with the signatures provided, and to validate the the signatures themselves are provided by a trusted source. There are well established ways to do that.

To address this common concern in a distro-neutral way, we are planning to add signature metadata support in the Disco API in order to deliver signatures where available. Signatures are, unfortunately, not universally available for OpenJDK distros, but we would encourage all distros to create and provide signatures for their binary artifacts in order to allow clients to establish trust in the delivered binaries. This would eliminate the Disco as a supply-chain security concern. It would then, at most, be a denial-of-service concern, but since that can be said for any and all of the individual distro CDNs and API endpoints (all of which are outside of GitHub's control, and all of which could be similarly exposed to DOS attacks), that level of concern should not block things.

Clients like actions/setup-java, which would be able to locate the distro binary URIs and the associated signatures where available, can then e.g. choose to maintain a list of "approved" sources (the known signing entities for the distros they are willing to use) and only accept distro binaries signed by one of those approved entities. Clients can also choose to "take a risk" on distros that are not signed if they wish. E.g. if setup-java wants to "still" use some distros that do not currently provide signatures that can be authenticated as being created by a trusted source, implicitly trusting or hoping that the mechanism for locating binaries of those distro and the binaries themselves would not be tampered with by someone other than the actual intended publisher of those binaries, it could do that on a distro-by-distro basis.

It' is obviously worth noting that the security concern raised here about the disco API applies equally to the Zulu API and the AOJ API used in the current v2-preview, and to some degree to the Zulu CDN currently used in v1. To the degree that security and supply chain concerns around this new dependency on a 3rd party API implementation is blocking consideration of Disco as a distro-binary locating API, it should also block v2 consideration of those other APIs. The proper solution for such concern is likely the same in all cases (establish trust in the delivered binaries, so that you don't have to worry about the api that pointed you to them being compromised).

@konradpabjan
Copy link
Collaborator

The preview time is roughly on par with what we've done with other first party actions. We also have telemetry that gives us information about how many workflows are using the preview and we feel very confident with what we've seen. The main breaking change is the new distribution input but anyone pinned to @main right now has a warning annotation always show up. We haven't noticed any un-planned regressions or failures which would warrant us extending the preview period which is why we're sticking with April 5th as the release date.

`All setup-java actions pinned to the 'main' branch will fail on April 5th 2021. Please explicitly reference your action with the 'v1' tag ('actions/setup-java@v1') to avoid build failures. Find more details at https://github.com/actions/setup-java/issues/137`

Since this is not a breaking change to v1, and it addresses a set of popularly requested distro support

It's not a breaking change in the sense that your workflow will fail, but it's a "breaking" change in terms of the behavior of the action. Such large changes warrant a separate release.

The biggest request has been to support adopt distributions. It's the oldest open issue in this repo and most commented on. It's what we've been focusing on as described here:
https://github.com/actions/setup-java/blob/main/docs/adrs/0000-v2-setup-java.md#goals--anti-goals

The main focus of the v2 version of setup-java will be to add support for adoptium builds of openJDK in addition to Zulu builds of openJDK

All of a sudden trying to add support for more distributions right before the planned release date is a bit of a stretch that certainly wasn't a goal at least for the initial v2 release.


With regards to the contents of this PR and supporting the Disco API, here is something we could explore

Initial v2 release (releasing April 5th)

  • Download Zulu builds using native APIs
  • Download Adoptium builds using native APIs

=> A future v2 version could do something like

  • Download Zulu builds using native APIs
  • Download Adoptium builds using native APIs
  • Download and support a few more different distributions using the Disco API. Maybe Corretto and something else to start off with

Gradually expanding support for more distributions, not everything all at once. Down the road who knows, maybe all downloads can be moved over to Disco API or some other API depending on how it goes but the main thing is to do this in controllable, bit-sized phases that are easily reviewable. The contents of this PR can be used & adapted to get something working towards initially supporting a few extra different distributions.


Thanks for information about sig files, I was not aware of such as feature. Validation on the setup-java side would be a good thing to add and it would certainly alleviate some of our concerns. It would have been good to mention this beforehand in #142. Large discussions like this should not be happening in PRs but in issues.

It' is obviously worth noting that the security concern raised here about the disco API applies equally to the Zulu API and the AOJ API

Downloading malicious binaries from Zulu or AOJ is a perfectly valid concern, but adding a discoverability API in the middle increases the possible attack surface which is why we're at least hesitant about initially jumping on board and using one

@maxim-lobanov
Copy link
Contributor

Closing the PR because, as it was mentioned above, we don't accept any new changes to v1

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.