search

jenkins-infra / repository-permissions-updater

Artifactory permissions synchronization tool and data set
77 stars 1.03k forks source link

gerrit-checks-api plugin #2947

Closed tdraebing closed 1 year ago

tdraebing commented 1 year ago

Repository URL

https://github.com/tdraebing/gerritchangequery-plugin

New Repository Name

gerrit-checks-api-plugin

Description

The gerrit-checks-api plugin adds a REST-API endpoint that allows to query for job runs that work on a Gerrit change/patchset. It uses a lucene index provided by the lucene-search plugin to enable performant querying. Such a REST API endpoint is required to efficiently provide data for the Checks-API in Gerrit. The plugin is compatible with the gerrit-trigger plugin and the GerritCodeReview plugin.

GitHub users to have commit permission

@tdraebing @lucamilanesio

Jenkins project users to have release permission

tdraebing lucamilanesio

Issue tracker

GitHub issues

jenkins-cert-app commented 1 year ago

Security audit, information and commands

The security team is auditing all the hosting requests, to ensure a better security by default. This message informs you that the security team was notified about the request and will soon participate in this issue to assist. The team is usually starting by a quick superficial audit and if it's not sufficient, they are planning a deeper audit.

Commands Security team only:
  • /audit-ok => the audit is complete, the hosting can continue :tada:.
  • /audit-skip => the audit is not necessary, the hosting can continue :tada:.
  • /audit-required => the superficial audit was not sufficient, a deeper look is necessary :mag:.
  • /audit-findings => the audit reveals some issues that require corrections :pencil2:.
Anyone:
  • /audit-review => the findings from the audits were corrected, this command will ping the security team to review the findings :eyes:. It's only applicable when the previous audit required changes.
Only one command can be requested per comment.

(automatically generated message)

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It appears you have some issues with your hosting request. Please see the list below and correct all issues marked Required. Your hosting request will not be approved until these issues are corrected. Issues marked with Warning or Info are just recommendations and will not stall the hosting process.

You can re-trigger a check by editing your hosting request or by commenting /hosting re-check

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It appears you have some issues with your hosting request. Please see the list below and correct all issues marked Required. Your hosting request will not be approved until these issues are corrected. Issues marked with Warning or Info are just recommendations and will not stall the hosting process.

You can re-trigger a check by editing your hosting request or by commenting /hosting re-check

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It appears you have some issues with your hosting request. Please see the list below and correct all issues marked Required. Your hosting request will not be approved until these issues are corrected. Issues marked with Warning or Info are just recommendations and will not stall the hosting process.

You can re-trigger a check by editing your hosting request or by commenting /hosting re-check

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It appears you have some issues with your hosting request. Please see the list below and correct all issues marked Required. Your hosting request will not be approved until these issues are corrected. Issues marked with Warning or Info are just recommendations and will not stall the hosting process.

You can re-trigger a check by editing your hosting request or by commenting /hosting re-check

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It appears you have some issues with your hosting request. Please see the list below and correct all issues marked Required. Your hosting request will not be approved until these issues are corrected. Issues marked with Warning or Info are just recommendations and will not stall the hosting process.

You can re-trigger a check by editing your hosting request or by commenting /hosting re-check

tdraebing commented 1 year ago

/hosting re-check

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It appears you have some issues with your hosting request. Please see the list below and correct all issues marked Required. Your hosting request will not be approved until these issues are corrected. Issues marked with Warning or Info are just recommendations and will not stall the hosting process.

You can re-trigger a check by editing your hosting request or by commenting /hosting re-check

tdraebing commented 1 year ago

/hosting re-check

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It looks like you have everything in order for your hosting request. A human volunteer will check over things that I am not able to check for (code review, README content, etc) and process the request as quickly as possible. Thank you for your patience.

Hosting team members can host this request with /hosting host

alecharp commented 1 year ago

a few comment on the plugin:

Having the libraries in the classpath is not enough to be sure that you have access to the classes you are expecting. This because different versions of the libraries can be on the classpath, which is not ordered. Using the plugins let us make sure there is only one version of the library on the classpath.

yaroslavafenkin commented 1 year ago

Hi @tdraebing,

https://github.com/tdraebing/gerritchangequery-plugin/blob/2cfc06b1ea8bef9e8ef6ce2a13c49ae25f8daccd/src/main/java/io/jenkins/plugins/gerritchecksapi/rest/GetCheckRuns.java#L47, I have an impression this needs to check for some permissions (not sure which exactly). Having no permission check there means Jenkins users with Overall/Read permission can use the endpoint to get information about runs for any specified change & specified patchset, which is very likely inaccessible to them otherwise.

yaroslavafenkin commented 1 year ago

/audit-findings

daniel-beck commented 1 year ago

https://github.com/tdraebing/gerritchangequery-plugin/blob/2cfc06b1ea8bef9e8ef6ce2a13c49ae25f8daccd/src/main/java/io/jenkins/plugins/gerritchecksapi/rest/GetCheckRuns.java#L42 might be a bit too generic; there are other Gerrit plugins.

tdraebing commented 1 year ago

Thanks for the feedback!

@alecharp I implemented the requested changes and will let you know as soon as they were. reviewed and merged (https://review.gerrithub.io/c/tdraebing/gerritchangequery-plugin/+/546313 and following)

@yaroslavafenkin I will perform some tests in that regard. Usually, if you have the right to see a build, e.g. in the UI, you should be allowed to get this information as well. I already pushed a change to only allow access to the endpoint, if you have overall READ access and I am working on checking each run that was found whether it is readable by the user.

@daniel-beck You might be right, although this plugin is meant to be a shared REST API for the gerrit-trigger and gerrit-code-review plugin, so that those plugins do not have to implement the same API and that the REST API looks the same independent of the Gerrit plugin being used. Can multiple ExtensionPoints by multiple plugins have the same urlName, i.e. gerrit in this case? If yes, I would think it is fine to stay this way. If not, I would suggest to change the path to gerrit-checks/runs. WDYT?

I will let you know as soon as all fixes are merged and ready.

Thanks, Thomas

daniel-beck commented 1 year ago

You might be right, although this plugin is meant to be a shared REST API for the gerrit-trigger and gerrit-code-review plugin, so that those plugins do not have to implement the same API and that the REST API looks the same independent of the Gerrit plugin being used.

That makes sense. I was lacking the context here, basically just looked at Yaroslav's review when we discussed this. My apologies for the noise.

Can multiple ExtensionPoints by multiple plugins have the same urlName, i.e. gerrit in this case? If yes, I would think it is fine to stay this way. If not, I would suggest to change the path to gerrit-checks/runs. WDYT?

I don't think anything prevents that, but one of the plugins/Actions will "win", i.e. all behaviors contributed by other plugins will be unavailable through HTTP.

tdraebing commented 1 year ago

The plugin is ready for reevaluation.

@alecharp

https://github.com/tdraebing/gerritchangequery-plugin/blob/2cfc06b1ea8bef9e8ef6ce2a13c49ae25f8daccd/.github/workflows/release-drafter.yml#L7 could be removed as the principal branch is main.

Done

https://github.com/tdraebing/gerritchangequery-plugin/blob/2cfc06b1ea8bef9e8ef6ce2a13c49ae25f8daccd/README.md?plain=1#L135 is not correct, based on the license section on the pom and the LICENSE.md file.

Done

running mvn verify shows two spotbugs issue which should be addressed.

Done:

[INFO] >>> spotbugs-maven-plugin:4.2.3:check (spotbugs) > :spotbugs @ gerrit-checks-api >>>
[INFO]
[INFO] --- spotbugs-maven-plugin:4.2.3:spotbugs (spotbugs) @ gerrit-checks-api ---
[INFO] Fork Value is true
[INFO] Done SpotBugs Analysis....
[INFO]
[INFO] <<< spotbugs-maven-plugin:4.2.3:check (spotbugs) < :spotbugs @ gerrit-checks-api <<<
[INFO]
[INFO]
[INFO] --- spotbugs-maven-plugin:4.2.3:check (spotbugs) @ gerrit-checks-api ---
[INFO] BugInstance size is 0
[INFO] Error size is 0
[INFO] No errors/warnings found
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  02:08 min
[INFO] Finished at: 2022-11-28T09:58:23+01:00
[INFO] ------------------------------------------------------------------------

the Jenkinsfile could use some configurations to build on jdk 11/17 if possible

The lucene-search plugin currently seems to have issues with Java 11. I anyway wanted to look into some security issue that plugin has and will try to make it run with Java 11. Afterwards, I will add the necessary configuration here as well.

JEP-229 is almost fully configured but you are missing a -Dchangelist.format=%d.v%s in .mvn/maven.config

Done

plugin manages httpclient library (https://github.com/tdraebing/gerritchangequery-plugin/blob/2cfc06b1ea8bef9e8ef6ce2a13c49ae25f8daccd/pom.xml#L56), which should be replaced with https://github.com/jenkinsci/apache-httpcomponents-client-4-api-plugin.

Done

The library should be excluded from gerrit-code-review

This dependency is now optional, since no classes are directly being used by this plugin.

the caffeine library versioning is already managed through https://github.com/jenkinsci/caffeine-api-plugin

Done

Please fill the index.jelly with content: https://github.com/tdraebing/gerritchangequery-plugin/blob/main/src/main/resources/index.jelly It's displayed at the update center.

Done

@yaroslavafenkin

https://github.com/tdraebing/gerritchangequery-plugin/blob/2cfc06b1ea8bef9e8ef6ce2a13c49ae25f8daccd/src/main/java/io/jenkins/plugins/gerritchecksapi/rest/GetCheckRuns.java#L47, I have an impression this needs to check for some permissions (not sure which exactly). Having no permission check there means Jenkins users with Overall/Read permission can use the endpoint to get information about runs for any specified change & specified patchset, which is very likely inacces

Now, read permissions for each job that provides runs for the returned result are checked. Only if the user has read permissions for that job, those runs are included in the response.

@daniel-beck

I changed the Rest API endpoint path to be more specific and also better extendable by this plugin itself.

NotMyFault commented 1 year ago

/audit-review

yaroslavafenkin commented 1 year ago

@tdraebing, last change on the main branch was 18 days ago. Did you forget to push the changes?

tdraebing commented 1 year ago

@yaroslavafenkin I am actually working on GerritHub: https://review.gerrithub.io/plugins/gitiles/tdraebing/gerritchangequery-plugin/+/refs/heads/main. The branch is up-to-date there. Might be that there is an issue with the replication to GitHub. Let me check.

tdraebing commented 1 year ago

@yaroslavafenkin Replication was fixed. The repository on GitHub is now up-to-date.

yaroslavafenkin commented 1 year ago

Reviewed my part, looks reasonable.

yaroslavafenkin commented 1 year ago

/audit-ok

NotMyFault commented 1 year ago

Hey @tdraebing,

thanks for addressing the feedback. In the meantime, archetypes have been updated, to generate less noise for plugin authors.

Additionally, I'd like to highlight:

While this plugin passes the security audit, I'd like to point out that the lucene-search plugin, you depend on, is subject to 2 unresolved security vulnerabilities: CVE-2022-36922 and CVE-2022-36910 Can you achieve the same functionality differently, or would you be interested in delivering a fix for the lucene-search plugin, if its behavior is ineluctable for you? This is no obligation, but I don't think it's a good idea for new plugins to depend on other plugins with open, unresolved and publicly disclosed vulnerabilities. Inevitably, the plugin manager will emit severe warnings for the plugin installed, recommending uninstalling it.

tdraebing commented 1 year ago

Hi @NotMyFault,

If you want, you can pin the release drafter version to @v5,

Done, although it seems that the release drafter doesn't work, when I use changes in GerritHub instead of pull requests on GitHub. At least, GitHub complains that the ".template" key does not have a value. Thus, I might remove the workflow and will then have to write the release notes myself.

The artifact id is gerrit-checks-api, but the repository name is gerritchangequery-plugin, and so is the name in the Readme and a variety of other non-sourcecode files. I'd recommend renaming and aligning all occasions with one name; likely gerrit-checks-api, given that's what the Readme outlines in the introduction block, I guess.

Done. I have changed all references of the old name in the repository. I haven't changed the repository name though, since this would be some effort and as I understand it, a new repository will anyway be created in the jenkinsci organisation, right? The issue description uses the correct name.

https://github.com/tdraebing/gerritchangequery-plugin/blob/4939ff7d1015ab81326a58884e661040e30aa184/.mvn/maven.config#L3 should be removed. This is only used for releases via CD.

@yaroslavafenkin asked me to add it. Actually, it might be that I would opt-in to CD as soon as the plugin becomes stable (Some refactorings etc.) are still planned. Since I don't expect too many changes to happen to this plugin, since it is pretty lightweight, I think it would make sense to release changes to it as soon as possible.

I'd like to point out that the lucene-search plugin, you depend on, is subject to 2 unresolved security vulnerabilities: https://github.com/advisories/GHSA-6954-h5c8-m29f and https://github.com/advisories/GHSA-m8w5-vwq3-gp8f

I am aware of those and plan to work on fixes for those as soon as I can set aside some time. I need the lucene index to efficiently search all runs even on a large Jenkins instance and don't want to reinvent the wheel to do so.

NotMyFault commented 1 year ago

don't want to reinvent the wheel to do so.

Sounds absolutely reasonable, nonetheless I'd like to ask other @jenkins-infra/hosting members, whether we want to host this new plugin depending on an existing plugin with open, disclosed and unresolved security vulnerabilities. Historically, instance admins were eager to move away from plugins with open security vulnerabilities, if there's no fix available. There are a plenty of Jira issues about this topic. Do we want to halt the request until the open issues have been fixed?

We don't vote on hosting requests, but I'd like to express that I'm not super happy with the dependency and don't think this is a great step forward.

timja commented 1 year ago

I don't think we should host a new plugin depending on a plugin with unresolved security vulnerabilities. Those should be resolved first.

tdraebing commented 1 year ago

Understandable. I already started to prepare fixes for the lucene-search plugin. We also don't want to have those security issues on our instances :-)

NotMyFault commented 1 year ago

/hosting re-check

github-actions[bot] commented 1 year ago

Hello from your friendly Jenkins Hosting Checker

It appears you have some issues with your hosting request. Please see the list below and correct all issues marked Required. Your hosting request will not be approved until these issues are corrected. Issues marked with Warning or Info are just recommendations and will not stall the hosting process.

You can re-trigger a check by editing your hosting request or by commenting /hosting re-check

tdraebing commented 1 year ago

⛔ Required: Your baseline specified does not meet the minimum Jenkins version required, please update 2.277.4</jenkins.version> to at least 2.361.4 in your pom.xml. Take a look at the baseline recommendations.

@NotMyFault Is this really required? This would require a lot of our stakeholders to update Jenkins, if they want to use this plugin (which would be a good idea, but will still likely not happen in a lot of cases :-(). This also requires massive changes in the lucene-search plugin to make it work with Java 11. Something that I am already working on, but which will take time, since it requires quite a huge version jump of Lucene. Thus, this will take a while and postpone the release further. Would it be ok to release this plugin first with its current baseline Jenkins version and update that as soon as possible?

NotMyFault commented 1 year ago

I'm fine with ignoring this one here, given this issue has been submitted before the baseline was bumped.

NotMyFault commented 1 year ago

/hosting host

jenkins-infra-bot commented 1 year ago

Hosting request complete, the code has been forked into the jenkinsci project on GitHub as https://github.com/jenkinsci/gerrit-checks-api-plugin

GitHub issues has been selected for issue tracking and was enabled for the forked repo.

A pull request has been created against the repository permissions updater to setup release permissions. Additional users can be added by modifying the created file.

Please delete your original repository (if there are no other forks), under 'Danger Zone', so that the jenkinsci organization repository is the definitive source for the code. If there are other forks, please contact GitHub support to make the jenkinsci repo the root of the fork network (mention that Jenkins approval was given in support request 569994). Also, please make sure you properly follow the documentation on documenting your plugin so that your plugin is correctly documented.

You will also need to do the following in order to push changes and release your plugin:

In order for your plugin to be built by the Jenkins CI Infrastructure and check pull requests, please add a Jenkinsfile to the root of your repository with the following content: buildPlugin(useContainerAgent: true, jdkVersions: [8, 11])

Welcome aboard!