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

[Marketplace Contribution] Simple SFTP #19787

Conversation

xsoar-bot
Copy link
Contributor

@xsoar-bot xsoar-bot commented Jun 27, 2022

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Contributor

@vibhuabharadwaj

Description

Simple SFTP Integration built on pysftp module that can be used to copy and read files from an SFTP server

Auto-Generated Documentation Requiring Modification

Video Link

Short demo video of the Pack usage. Speeds up the review. Optional but recommended. Use a video sharing service such as Google Drive or YouTube.

@content-bot content-bot added the Contribution Thank you! Contributions are always welcome! label Jun 28, 2022
@content-bot content-bot changed the base branch from master to contrib/xsoar-contrib_vibhuabharadwaj-contrib-SimpleSFTP June 28, 2022 00:00
@content-bot content-bot requested a review from ilappe June 28, 2022 00:00
@content-bot
Copy link
Collaborator

Thank you for your contribution. Your generosity and caring are unrivaled! Rest assured - our content wizard @ilappe will very shortly look over your proposed changes.

@ilappe ilappe removed their assignment Jun 29, 2022
@ilappe ilappe removed their request for review June 29, 2022 07:48
Copy link
Contributor

@MosheEichler MosheEichler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vibhuabharadwaj,
You did an Excellent Job!! :)

I added a few little comments, please review them

  1. Please fill the README file, you can use the demisto-sdk generate-docs command.
  2. please change the picture of the integration.
  3. Please run demisto-sdk format on the SimpleSFTP.yml file.

Please notify me when you are done so we can schedule a Demo session before merging the PR.

Thank you.

Packs/SimpleSFTP/pack_metadata.json Show resolved Hide resolved
Packs/SimpleSFTP/pack_metadata.json Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
Note:
-------
This integration will need a custom docker image which includes the pysftp module as a dependency. To create the docker image on your server you can run :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This integration will need a custom docker image which includes the pysftp module as a dependency. To create the docker image on your server you can run :
This integration will need a custom docker image which includes the pysftp module as a dependency. To create the docker image on your server you can run:

Comment on lines 10 to 13
- display: Use system proxy settings
name: proxy
required: false
type: 8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to the end of cofiguration (after Username)
And please add:

- display: Trust any certificate (not secure)
  name: insecure
  required: false
  type: 8

- arguments:
- name: directory
required: true
name: sftp-listdir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a description of the command.

- arguments:
- name: filePath
required: true
name: sftp-copyfrom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a description of the command.

- name: filePath
required: true
name: sftp-copyfrom
dockerimage: "demisto/pysftp\t"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dockerimage: "demisto/pysftp\t"
dockerimage: "demisto/netmiko:1.0.0.24037"

See the below comment we recommend changing the python dependency to Paramiko. So you can use the netmiko docker image the dependency is already there.

Comment on lines 1 to 5
Note:
-------
This integration will need a custom docker image which includes the pysftp module as a dependency. To create the docker image on your server you can run :

/docker_image_create name=demisto/pysftp base="<demisto/python3>" dependencies=pysftp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the above comment about the docker iamge

@@ -0,0 +1,43 @@
import demistomock as demisto # noqa: F401
import pysftp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this answer, it seems better to use Paramiko. Please change it to Paramiko here is the documentation on how to use it.

@MosheEichler MosheEichler added the pending-contributor The PR is pending the response of its creator label Jul 5, 2022
@MosheEichler
Copy link
Contributor

Hi @vibhuabharadwaj, is this still something you want to contribute? Please let us know if you need any further assistance.

@vibhuabharadwaj
Copy link
Contributor

Hi @MosheEichler ,
I will need more time to implement your suggestions since changing from pysftp to paramiko basically means I need to rewrite the whole integration as all the API calls, parameters etc are different. This was supposed to be a simpler version with PySftp but I see your point in terms of extending the functionality its better to rewrite in paramiko.

@vibhuabharadwaj
Copy link
Contributor

I should have something ready by next week and I'll update the PR here with the changes. Thank you for your patient follow-ups and for your review!

@MosheEichler
Copy link
Contributor

@vibhuabharadwaj HI!

Thank you for contributing and sorry about the change from pysftp to paramiko. But I think it's worth it. Take your time and fill free to reach out for anything you need.

Thanks again!

- Changed to use paramiko
- Added Port parameter
- Refactored to handle errors/exceptions and print traceback
- Changed docker image to demisto/sftp with paramiko as dependency
- Added optional command argument "returnFile" to return file to war room along with file
- Added main function
- Added image
- Added new arg returnFile
- Added descriptions for args and commands
Please review as I added most of the changes. I did not know where to add the "marketplacev2, xsoar " key in the yml.
Changed description
@vibhuabharadwaj
Copy link
Contributor

I'm ready to schedule a demo when you're done reviewing @MosheEichler
Also wanted to discuss the similarity of this vs Remote Access v2 integration. I see that remote access v2 also uses paramiko but I think that uses scp while this integration uses sftp. Both are quite similar.

Copy link
Contributor

@MosheEichler MosheEichler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Please run demisto-sdk format -i Packs/SimpleSFTP/Integrations/SimpleSFTP/SimpleSFTP.yml.

Copy link
Contributor

@MosheEichler MosheEichler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The demo was amazing please see my notes.

Packs/SimpleSFTP/Integrations/SimpleSFTP/SimpleSFTP.py Outdated Show resolved Hide resolved
if demisto.args()["returnFile"] == "True":
demisto.results(fileResult(filename=filePath[filePath.rindex("/") + 1:], data=data))
entry = {
'Type': entryTypes['note'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to return a file instead of plain text (I mean take in mind what if the file will be huge).
Please see here

vibhuabharadwaj and others added 3 commits August 1, 2022 12:10
Co-authored-by: Moshe Eichler <78307768+MosheEichler@users.noreply.github.com>
Updated logic for returning file
Co-authored-by: Moshe Eichler <78307768+MosheEichler@users.noreply.github.com>
@MosheEichler MosheEichler merged commit 3a36850 into demisto:contrib/xsoar-contrib_vibhuabharadwaj-contrib-SimpleSFTP Aug 2, 2022
MosheEichler added a commit that referenced this pull request Aug 4, 2022
* [Marketplace Contribution] Simple SFTP (#19787)

* "pack contribution initial commit"

* Update SimpleSFTP.py

- Changed to use paramiko
- Added Port parameter
- Refactored to handle errors/exceptions and print traceback
- Changed docker image to demisto/sftp with paramiko as dependency
- Added optional command argument "returnFile" to return file to war room along with file
- Added main function

* Update SimpleSFTP.yml

- Added image
- Added new arg returnFile
- Added descriptions for args and commands

* Update SimpleSFTP.py

* Update SimpleSFTP.yml

Please review as I added most of the changes. I did not know where to add the "marketplacev2, xsoar " key in the yml.

* Update pack_metadata.json

Changed description

* Update pack_metadata.json

* Update SimpleSFTP_description.md

* Update Packs/SimpleSFTP/Integrations/SimpleSFTP/SimpleSFTP.py

Co-authored-by: Moshe Eichler <78307768+MosheEichler@users.noreply.github.com>

* Update SimpleSFTP.py

Updated logic for returning file

* Update Packs/SimpleSFTP/pack_metadata.json

Co-authored-by: Moshe Eichler <78307768+MosheEichler@users.noreply.github.com>

* Update SimpleSFTP.yml

* Update SimpleSFTP.yml

Co-authored-by: Vibhu A Bharadwaj <53234515+vibhuabharadwaj@users.noreply.github.com>
Co-authored-by: Moshe Eichler <78307768+MosheEichler@users.noreply.github.com>

* Fortinet FortiGate Modeling Rules (#20292)

* Added Modeling Rules for FortiGate

* Changed the time field to match the original CEF

* Added Release notes

* Added ReleaseNotes

* Added FortinetFortiGateModelingRules_schema.json

* Changed the value in first field in the json file

* Delete create_certs.sh

* Added README file

* Bigger font for header

* Changed styling

* Revert "Delete create_certs.sh"

This reverts commit 0d994bb.

* Changed file names

* Changed the README file

* Revert "Added FortinetFortiGateModelingRules_schema.json"

This reverts commit 0c86f6d.

* Added Fortinet

* Added Fortigate to known words

* Changed the ReleaseNotes

* Changed the Yaml file

Co-authored-by: evisochek <72695126+evisochek@users.noreply.github.com>

* fix the display of the credentials parameter (#20345)

* Cyblethreatintel updates (#20097)

* Cyblethreatintel updates (#19832)

* Cyble Events Integration

Pack having Cyble Event incident integration

* Revert "Cyble Events Integration"

This reverts commit 11f934b.

* integration update

* UT fixes

* UT fixes

* change docker image and readme

* review changes

* reversal of one of review change

change is breaking the flow

* review changes

* readme update

* docker image update

* flake8 error fixes

* UT fixes

* UT coverage

* ut coverage

* ut fixes

* flake8 fixes

* ut fix

* ut fixes

* Added `breakingChanges` and update the RN

Co-authored-by: sudheer-samethadka <101622497+sudheer-samethadka@users.noreply.github.com>
Co-authored-by: israelpolishook <ipolishuk@paloaltonetworks.com>

* Commit

* commit

* commit

* commit

* commit

* Add the proxy and insecure to the integration

* Update docker

* commit

* Correcting typo

* commit

* commit

* Update docker

Co-authored-by: cyble-dev <101622497+cyble-dev@users.noreply.github.com>
Co-authored-by: sudheer-samethadka <101622497+sudheer-samethadka@users.noreply.github.com>
Co-authored-by: israelpolishook <ipolishuk@paloaltonetworks.com>

* New version for solving authentication problem (#20222)

* new version for solving authentication problem

* docker image

* SplunkPy pre-release docker image

* RN's

* return the line connection_args['autologin'] = True

* Update Packs/SplunkPy/ReleaseNotes/2_4_5.md

Co-authored-by: yuvalbenshalom <ybenshalom@paloaltonetworks.com>

* Update Packs/SplunkPyPreRelease/ReleaseNotes/1_0_12.md

Co-authored-by: yuvalbenshalom <ybenshalom@paloaltonetworks.com>

* Trig build

* misspelled

* Trig build

* same like costumer's version -
only basic

* before changes but with new docker

* like costumer, and conf.json rolled back to the old configuration

* autologin + basic + new docker

* basic was added to SplunkPyPreRelease.pypre

* Test playbook was moved to skipped

* trig build

Co-authored-by: yuvalbenshalom <ybenshalom@paloaltonetworks.com>

* CSP and demistomock

* validate fixes

* image and demost

* image

* ignore

* Utilities

* fix lint error

Co-authored-by: xsoar-bot <67315154+xsoar-bot@users.noreply.github.com>
Co-authored-by: Vibhu A Bharadwaj <53234515+vibhuabharadwaj@users.noreply.github.com>
Co-authored-by: Moshe Eichler <78307768+MosheEichler@users.noreply.github.com>
Co-authored-by: nkanon <109467661+nkanon@users.noreply.github.com>
Co-authored-by: evisochek <72695126+evisochek@users.noreply.github.com>
Co-authored-by: Shai Yaakovi <30797606+yaakovi@users.noreply.github.com>
Co-authored-by: cyble-dev <101622497+cyble-dev@users.noreply.github.com>
Co-authored-by: sudheer-samethadka <101622497+sudheer-samethadka@users.noreply.github.com>
Co-authored-by: israelpolishook <ipolishuk@paloaltonetworks.com>
Co-authored-by: rshunim <102469772+rshunim@users.noreply.github.com>
Co-authored-by: yuvalbenshalom <ybenshalom@paloaltonetworks.com>
Co-authored-by: meichlerpanw <meichler@paloaltonetworks.com>
@xsoar-bot xsoar-bot deleted the vibhuabharadwaj-contrib-SimpleSFTP branch November 23, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Thank you! Contributions are always welcome! docs-approved pending-contributor The PR is pending the response of its creator
Projects
None yet
5 participants