microsoft / msticpy

Microsoft Threat Intelligence Security Tools
Other
1.72k stars 310 forks source link

Add security token auth and credential loading from msticpyconfig.yaml to SplunkUploader #731

Closed Tatsuya-hasegawa closed 7 months ago

Tatsuya-hasegawa commented 8 months ago

Hi,

I have added the auth features to SplunkUploader as same as Splunk QueryProvider.

Related issue. https://github.com/microsoft/msticpy/issues/693

This PR resolves the security concern about the current hard coding style of username and password.

Thank you ! Best regards,

Tatsuya-hasegawa commented 8 months ago

@ianhelle

I have updated all the points! I appreciate your comment.

Unfortunately, Splunk python SDK don't use snake case but camel case, it's unlucky for me. I added a code snippet to splunk_driver.py in order to rollback bearer_token to splunkToken in front of splunk client's connect().

These codes's splunk connection tests were ok for both query provider and uploader.

Yes, Of course I can. I did it in the new commit. The name "token" has already used as session token in the driver's connect parameter, so I used bearer_token instead of splunkToken referring to Splunk python SDK as following. https://github.com/splunk/splunk-sdk-python/blob/master/splunklib/binding.py#L523

Tatsuya-hasegawa commented 8 months ago

Hi @ianhelle

I recognize that the requested modification has been completed. :) Would you teach me the rest task for me, if that is not enough ? I thought processing workflows need your approval.

Best regards,

ianhelle commented 8 months ago

Hi @ianhelle

I recognize that the requested modification has been completed. :) Would you teach me the rest task for me, if that is not enough ? I thought processing workflows need your approval.

Best regards,

Hey sorry. I've been pulled away on other work. The changes look good. Just running a build and will complete if everything looks OK

ianhelle commented 8 months ago

Seems there are a few failing tests and linter failures. See if you can fix these (they look simple) and ping me if not. It would be worth installing our pre-commit script - it will run many of the checks before letting you commit.

pip install pre-commit
# from the repo root
pre-commit install
Tatsuya-hasegawa commented 8 months ago

@ianhelle

I have resolved my lint error. And this is my pre-check result. It looks passed. In my laptop, it was needed to change "isort" rev version to 5.12.0 in .pre-commit.yaml. I'm sorry for ignoring to treat this error until now. I believe I can do pre-check correctly from now on.

(base) hacket@hackeTlab msticpy % git commit -m  "modify just lints"
[INFO] Initializing environment for https://github.com/pycqa/isort.
[INFO] Installing environment for https://github.com/pycqa/isort.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pycqa/pydocstyle.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Check Yaml...............................................................Passed
Check JSON...........................................(no files to check)Skipped
Trim Trailing Whitespace.................................................Passed
black....................................................................Passed
pylint...................................................................Passed
flake8...................................................................Passed
isort (python)...........................................................Passed
pydocstyle...............................................................Passed
check_reqs_all...........................................................Passed
[add_token_auth_to_splunk_uploader 4fbaa95f] modify just lints
 2 files changed, 4 insertions(+), 4 deletions(-)

Thank you for teaching. 😄

ianhelle commented 7 months ago

Still some tests failing. I think instead of the check for arg_dict["bearer_token"] you should be prob be using arg_dict.get("bearer_token") There are also some very simple linter fixes - running black on the uploader file should fix this. The flake8 check (about additional blank lines will probably be fixed by this). Apologies for delay in getting these run.

Tatsuya-hasegawa commented 7 months ago

@Ianhelle

Thank you. I have modified these points! Best regards,

(base) hacket@hackeTlab msticpy % git commit -m "a few fix for pytest result." 
Check Yaml...........................................(no files to check)Skipped
Check JSON...........................................(no files to check)Skipped
Trim Trailing Whitespace.................................................Passed
black....................................................................Passed
pylint...................................................................Passed
flake8...................................................................Passed
isort (python)...........................................................Passed
pydocstyle...............................................................Passed
check_reqs_all...........................................................Passed
[add_token_auth_to_splunk_uploader 896da43a] a few fix for pytest result.
 2 files changed, 2 insertions(+), 1 deletion(-)
ianhelle commented 7 months ago

@Tatsuya-hasegawa - thanks for sticking with this. Apologies for taking so long to get it completed

Tatsuya-hasegawa commented 6 months ago

Thank you for merging this !