search

jenkins-infra / repository-permissions-updater

Artifactory permissions synchronization tool and data set
79 stars 1.05k forks source link

SEC1 Security plugin #3734

Closed rahuldarekar222 closed 9 months ago

rahuldarekar222 commented 10 months ago

Repository URL

https://github.com/sec0ne/resilient-foss-jenkins-plugin

New Repository Name

secone-security-plugin

Description

Sec1 Security jenkins plugin will enable jenkins community to scan their SCM for open source vulnerabilities. They will get their personalized account with customized dashboard having different analytics.

GitHub users to have commit permission

@rahuldarekar222 @saurabhthatte007

Jenkins project users to have release permission

rahuldarekar saurabhthatte_sec1

Issue tracker

Jira

jenkins-cert-app commented 10 months 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 a Jenkins Security Scan was triggered on your repository. It takes ~10 minutes to complete.

Commands The bot will parse all comments, and it will check if any line start with a command. 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-findings => the audit reveals some issues that require corrections :pencil2:.
Anyone:
  • /request-security-scan => the findings from the Jenkins Security Scan were corrected, this command will re-scan your repository :mag:.
  • /audit-review => the findings from the audit 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, version: 1.26.21)

github-actions[bot] commented 10 months 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

jenkins-cert-app commented 10 months ago

The Jenkins Security Scan discovered 9 finding(s) :mag:. For each of them, either apply the recommended correction, suppress the warning or provide a justification.

Once you're done, either re-run the scan with /request-security-scan or request the Security team to review your justifications with /audit-review.

Stapler: Missing POST/RequirePOST annotation

You can find detailed information about this finding here.

SecOneScannerPlugin.java#528 ``` Potential CSRF vulnerability: If DescriptorImpl#doFillCredentialsIdItems connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST ```
SecOneScannerPlugin.java#504 ``` Potential CSRF vulnerability: If DescriptorImpl#doCheckCriticalThreshold connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST ```
SecOneScannerPlugin.java#498 ``` Potential CSRF vulnerability: If DescriptorImpl#doCheckStatusAction connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST ```
SecOneScannerPlugin.java#476 ``` Potential CSRF vulnerability: If DescriptorImpl#doCheckScmUrl connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST ```

Stapler: Missing permission check

You can find detailed information about this finding here.

SecOneScannerPlugin.java#528 ``` Potential missing permission check in DescriptorImpl#doFillCredentialsIdItems ```
SecOneScannerPlugin.java#504 ``` Potential missing permission check in DescriptorImpl#doCheckCriticalThreshold ```
SecOneScannerPlugin.java#498 ``` Potential missing permission check in DescriptorImpl#doCheckStatusAction ```
SecOneScannerPlugin.java#476 ``` Potential missing permission check in DescriptorImpl#doCheckScmUrl ```

Jenkins: Plaintext password storage

You can find detailed information about this finding here.

SecOneScannerPlugin.java#93 ``` Field should be reviewed whether it stores a password and is serialized to disk: accessToken ```
rahuldarekar222 commented 10 months ago

/hosting re-check

github-actions[bot] commented 10 months ago

Hello from your friendly Jenkins Hosting Checker

It looks like you have everything in order for your hosting request. A member of the Jenkins hosting team will check over things that I am not able to check(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

rahuldarekar222 commented 10 months ago

/request-security-scan

jenkins-cert-app commented 10 months ago

The Jenkins Security Scan did not find anything dangerous with your plugin, congratulations! :tada:

:bulb: The Security team recommends that you are setting up the scan in your repository by following our guide.

daniel-beck commented 10 months ago

Please explain https://github.com/sec0ne/resilient-foss-jenkins-plugin/blob/7097b84810d568355231c38cb72161e950ed6841/src/main/java/io/jenkins/plugins/SecOneScannerPlugin.java#L93-L94 given https://github.com/sec0ne/resilient-foss-jenkins-plugin/blob/7097b84810d568355231c38cb72161e950ed6841/README.md?plain=1#L144-L146

rahuldarekar222 commented 10 months ago

we are not storing this and while passing it we are encoding it with Base64 encoder as standard

daniel-beck commented 10 months ago

How would your users use the feature without storing a plaintext credential somewhere, e.g. in a Jenkinsfile?

rahuldarekar222 commented 10 months ago

We have removed accessToken as an option from the plugin. Now user must provide the credentialsId

alecharp commented 10 months ago

Hello, I'm reviewing your plugin repository and I have a few comment on the current project configuration:

Beside the pom.xml dependencies issue, I don't have anything that would block the hosting process. The credentials management is not a problem but it would simplify the maintenance of the plugin.

alecharp commented 10 months ago

Sorry, I missed something: you haven't imported the .mvn folder in your repository. This is mandatory as you have the incrementals structure of the project.version in your project. Please see https://www.jenkins.io/doc/developer/plugin-development/incrementals/#enabling-incrementals

rahuldarekar222 commented 10 months ago

suggested changes applied and updated dependency versions to latest available versions.

alecharp commented 10 months ago

thank you. I don't think you setup the pom to use the BOM. That would be beneficial for the plugin. Also I found the API plugin for JSON library: https://github.com/jenkinsci/json-api-plugin. Please use this Jenkins plugin rather than the library.

rahuldarekar222 commented 10 months ago

suggested changes applied. let us know if there is anything else that needs tot be done.

alecharp commented 10 months ago

Thank you for updating the dependencies.

I think there was a confusion about the .mvn folder. You need to include the extensions.xml file from it. There is no problem have the wrapper folder but you didn't include the mvnw{,.cmd} scripts to be able to use the wrapper.

Please see the previous link I shared.

rahuldarekar222 commented 10 months ago

added extensions.xml

NotMyFault commented 10 months ago

added extensions.xml

The extension file added is wrong. Please refer to archetypes which .mvn and .github files you need to add with which content in. The same applies to the Jenkinsfile.

If you are at it, remove the and maven central block from the POM. That's unneeded.

Following the maven naming convention, you want to name your artifactId secone-security without a number in the name.

rahuldarekar222 commented 10 months ago

done with changes. is there any way we can communicate and close this instead of working on the issues over comments section?

NotMyFault commented 10 months ago

done with changes.

None of my changes requested have been implemented.

done with changes. is there any way we can communicate and close this instead of working on the issues over comments section?

Closing the issue results in your plugin not being hosted.

rahuldarekar222 commented 10 months ago

Done pushed changes. Seems my token did not had push permission for workflow

NotMyFault commented 10 months ago

sec0ne/resilient-foss-jenkins-plugin

${artifactId} is a placeholder you replace with your actual artifactId. The Jenkinsfile differs from my linked default Jenkinsfile and the wrong <build> block in the pom hasn't been removed still.

rahuldarekar222 commented 10 months ago

replaced Jenkinsfile content with contents on Jenkinsfile replaced ${artifactId} placeholder in CODEOWNERS and release-drafter.yml removed build block from pom.xml

rahuldarekar222 commented 9 months ago

apologies for chasing, please let us know if you need anything else from our side. Thanks for the help.

alecharp commented 9 months ago

Even if you are now using credentialsId, the way you are using it is not correct. You should review that, using the link I provided previously. It would simplify some part of the SecOneScannerPlugin.

The repository name you requested is sec1-security but the artifactId in the pom.xml is secone-security. Please address that.

github-actions[bot] commented 9 months 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

rahuldarekar222 commented 9 months ago

/hosting re-check

github-actions[bot] commented 9 months 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

rahuldarekar222 commented 9 months ago

update the code as suggested and modified the request. New repository name: secone-security-plugin

Hello, I'm reviewing your plugin repository and I have a few comment on the current project configuration:

  • this is absolutely not necessary, as it's mostly for human and my brain, but I find that having an ordered pom.xml simplifies its comprehension. For that I usually run mvn tidy:pom on the project.
  • the scm section in the POM would have to be updated once migrated to the jenkinsci organization. It would be probably easier to do that now (using the requested repository name) so that it's not forgotten before you ran your first release of the plugin.

  • may I suggest that you use the BOM to manage the dependencies of your plugin to other Jenkins plugin.

  • both org.jenkins-ci.plugins:git and org.json:json dependencies have dependencies to binaries with known CVE, you need to update those.

  • I'm trying to find a different dependency than org.json as it would probably be better to use a Jenkins plugin which would manage the dependency to the library. We have a Jackson2 API and Databind plugin for this for example.

    • After looking at the code in SecOneScannerPlugin, you should use Jackson2 Databind library to handle the response you get from your service.
  • You may put your classes within a plugin specific package rather that the io.jenkins.plugins they are currently in. Most plugins use the plugin name to make sure there is no conflict with other plugin. I would suggest using io.jenkins.plugins.secone.security.
  • You may delete CustomDescriptor class
  • You should delete the comment code in SecOneScannerPlugin about the accessToken (field, getter and setter)

  • For the instanceUrl, rather than using a environment variable, I would suggest that you use a global configuration field of your plugin. This would let Jenkins Administrator to easily configure the endpoint URL. In future version, you could even add a list of potential endpoint and let the project users decide which once to use in the Jenkins project configuration.

    • this is not blocking for the hosting process, just a nice to have in my opinion.

  • I don't know why you need to use the getApiKey() method as normally, you have the API Key in the credentialsId field of the class

  • May I suggest that you using int in the Threshold class to simplify its management in the configuration?

Beside the pom.xml dependencies issue, I don't have anything that would block the hosting process. The credentials management is not a problem but it would simplify the maintenance of the plugin.

we have updated the groupId as suggested but seems now we are getting below: Required: The from the pom.xml should be io.jenkins.plugins instead of io.jenkins.plugins.secone.security

alecharp commented 9 months ago

You may put your classes within a plugin specific package rather that the io.jenkins.plugins they are currently in. Most plugins use the plugin name to make sure there is no conflict with other plugin. I would suggest using io.jenkins.plugins.secone.security

I suggested to change the base package name of your classes, not the groupId in the POM file.

sec1-security commented 9 months ago

/hosting re-check

github-actions[bot] commented 9 months ago

Hello from your friendly Jenkins Hosting Checker

It looks like you have everything in order for your hosting request. A member of the Jenkins hosting team will check over things that I am not able to check(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

NotMyFault commented 9 months ago

I recommend adding the default .gitignore https://github.com/jenkinsci/archetypes/blob/master/common-files/gitignore

rahuldarekar222 commented 9 months ago

Done committed .gitignore

NotMyFault commented 9 months ago

/request-security-scan

jenkins-cert-app commented 9 months ago

The Jenkins Security Scan did not find anything dangerous with your plugin, congratulations! :tada:

:bulb: The Security team recommends that you are setting up the scan in your repository by following our guide.

rahuldarekar222 commented 9 months ago

/hosting re-check

github-actions[bot] commented 9 months ago

Hello from your friendly Jenkins Hosting Checker

It looks like you have everything in order for your hosting request. A member of the Jenkins hosting team will check over things that I am not able to check(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 9 months ago

I don't see anything that need fixing in the repository before hosting. The credentials management is not optimal but there is no security concerns and it can be improved later by maintainer or contributors.

Do you agree @NotMyFault?

NotMyFault commented 9 months ago

I don't see anything that need fixing in the repository before hosting. The credentials management is not optimal but there is no security concerns and it can be improved later by maintainer or contributors.

Do you agree @NotMyFault?

No objections from me :)

alecharp commented 9 months ago

/hosting host

jenkins-infra-bot commented 9 months ago

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

A Jira component named secone-security-plugin has also been created with rahuldarekar as the default assignee for issues.

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: https://github.com/jenkinsci/archetypes/blob/master/common-files/Jenkinsfile

Welcome aboard!