Storing plugin credentials in DB

[devmrfitz]

Description

Storing plugin credentials in DB

Related issues

#978

Type of change

Please delete options that are not relevant.

• [ ] Bug fix (non-breaking change which fixes an issue).
• [ ] New feature (non-breaking change which adds functionality).
• [X] Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist

• [ ] I have read and understood the rules about how to Contribute to this project
• [ ] The pull request is for the branch develop
• [ ] A new analyzer or connector was added, in which case:
  • [ ] Usage file was updated.
  • [ ] Advanced-Usage was updated (in case the analyzer/connector provides additional optional configuration).
  • [ ] Secrets were added in env_file_app_template, env_file_app_ci and in the Installation docs, if necessary.
  • [ ] If the analyzer/connector requires mocked testing, _monkeypatch() was used in its class to apply the necessary decorators.
  • [ ] If a File analyzer was added, its name was explicitly defined in test_file_scripts.py (not required for Observable Analyzers).
• [ ] If external libraries/packages with restrictive licenses were used, they were added in the Legal Notice section.
• [ ] The tests gave 0 errors.
• [ ] Linters (Black, Flake, Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.
• [ ] The commits were squashed into a single one (optional, they will be squashed anyway by the maintainer)
• [ ] If changes were made to an existing model/serializer/view, the docs were updated and regenerated (check CONTRIBUTE.md).

Important Rules

• If your changes decrease the overall tests coverage (you will know after the Codecov CI job is done), you should add the required tests to fix the problem
• Everytime you make changes to the PR and you think the work is done, you should explicitly ask for a review

Real World Example

Please delete if the PR is for bug fixing. Otherwise, please provide the resulting raw JSON of a finished analysis (and, if you like, a screenshot of the results). This is to allow the maintainers to understand how the analyzer works.

[lgtm-com[bot]]

This pull request introduces 5 alerts when merging 97b8774891b56ccff87b79cbcced637d1f86589e into fa186504b91137d2374ab35065f2cb1f16cd2a71 - view on LGTM.com

new alerts:

• 5 for Unused global variable

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 995b0a0e574dce0671fa6d1ed402786b9b1833a0 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[0ssigeno]

The issue talks about manage the secrets through a GUI. This allows to configure them in the Django Admin interface and, even if technically is a GUI, I do not think that this was what was wanted. I will give a code review in a sec but, before merging this, we will wait @mlodic review, so that we can confirm that solves the issue in a wanted way

[devmrfitz]

@0ssigeno I was planning to implement a proper frontend for it. That's why the PR is a draft. Do you want me to not do that?

[0ssigeno]

I saw the review pop up and though that you was almost done :D

Nono keep going, having a complete frontend is absolutely better!

[devmrfitz]

I tend to ask reviews at every step. Is that the preferred way or should I avoid it?

On Tue, 9 Aug 2022, 3:47 pm Simone Berni, @.***> wrote:

I saw the review pop up and though that you was almost done :D

Nono, having a complete frontend is absolutely better!

— Reply to this email directly, view it on GitHub https://github.com/intelowlproject/IntelOwl/pull/1136#issuecomment-1209190802, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN5NJU3XG4YTAO7TJCTIY2LVYIV3HANCNFSM556GLAKA . You are receiving this because you authored the thread.Message ID: @.***>

[0ssigeno]

On the review, i normally give a feedback about the entire code of the PR. If you want to have a discussion, ask stuff, or simply confirm that the "path" chosen is correct, i think that mention us is a better solution!

But this is just my own preference, at the end it changes nothing :D

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 26d3e3a2f0f84932c6c42f9270b0b0240a9f4ba4 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging e5808fba98216249a632a96a32c3cd9eaa801420 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

[devmrfitz]

@0ssigeno Could you re-review now?

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 74fd4f39176219ec35aae946953ac7e2d92e5e2a into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

We can even discuss if we can automatically launch the migrate_secrets.py during the docker startup if some api_key are found; this is just an idea, it has pros and cons.

@0ssigeno I think I had discussed this during my proposal formulation time, and it was decided that automatically running the script would make people dependent on the script i.e. they would just put the secrets in env_file_app and expect it to get loaded into the db automatically, hence removing the point of this migration-to-db in the first place

[lgtm-com[bot]]

This pull request introduces 1 alert when merging f018a4da859c05efd76c7f69d2c224474fe4395b into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

@0ssigeno @drosetti Any idea why, despite making the migrations, the table is not present in backend-tests?

[0ssigeno]

Ok this is tricky. Long story short, django works this way: For each app in settings, it calls the ready before doing the migration. Meaning that, if in the ready method you do something (like your case) with another DB table, it will not find that. For the fixing, need a little bit more time :D

[0ssigeno]

In this case, the error is caused by

    def ready(self):
        from .serializers import AnalyzerConfigSerializer  # to avoid import issue

        set_permissions(settings.DEFAULT_CACHE)
        # we "greedy cache" the config at start of application
        # because it is an expensive operation
        AnalyzerConfigSerializer.read_and_verify_config() # <----------- HERE

You can test it on your local environment by pruning the intel_owl_postgres_data volume, and restart the application. It should return the same error as the CI

[0ssigeno]

This should fix it. If you can, test it a little bit manually to ensure that it works, but I'm quite confident:

import sys

class AnalyzersManagerConfig(AppConfig):
    name = "api_app.analyzers_manager"

    def ready(self):

        set_permissions(settings.DEFAULT_CACHE)
        # we "greedy cache" the config at start of application
        # because it is an expensive operation
        if 'runserver' in sys.argv:

            from .serializers import AnalyzerConfigSerializer  # to avoid import issue
            AnalyzerConfigSerializer.read_and_verify_config()

[devmrfitz]

This should fix it. If you can, test it a little bit manually to ensure that it works, but I'm quite confident:

import sys

class AnalyzersManagerConfig(AppConfig):
    name = "api_app.analyzers_manager"

    def ready(self):

        set_permissions(settings.DEFAULT_CACHE)
        # we "greedy cache" the config at start of application
        # because it is an expensive operation
        if 'runserver' in sys.argv:

            from .serializers import AnalyzerConfigSerializer  # to avoid import issue
            AnalyzerConfigSerializer.read_and_verify_config()

I did a slight modification (as gunicorn/uwsgi don't use runserver). Does it look good?

[0ssigeno]

Cool! I thought about some corner case, so we could even add the call AnalyzerConfigSerializer.read_and_verify_config() inside the file wsgi.py, that should be called once everything from Django has been set up. Maybe is even better? But as you prefer, no constraints here

[lgtm-com[bot]]

This pull request introduces 1 alert when merging febcfcc2cc8da314f323261189c5eafc925638a7 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[0ssigeno]

Tests still fail 🤔 Let's try the wsgi.py road?

[lgtm-com[bot]]

This pull request introduces 1 alert when merging ffd764fb89b1d07e1f58927a109ac795347869fd into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

@0ssigeno looks good now?

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 8e40d5c2d16138dcc21bb3564b96b4398b018007 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 4579251e6d2d4b56a6545bdf3b140e0f76d107bb into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging cf8644781b2ad9ea1e3c7d0438149e84dbeb3272 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging a38440eaaf36f7ed7dcb647b52cd5b46ffcee61f into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging b1fdad21050bfa126cb09c5625f01e86a7cf7800 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging def0f0fe24388402fe005d9c81c606b2c8c53953 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 75e0d2dd011b7c8f10b466d11094a027b1b60b8c into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

Talking about the backend part I agree with Simone, so I don't add anything.

From the frontend prospective, I tested the UI/UX and there are several changes to do:

[ ] I noticed the button to access to the organization config in the "plugin" section appears after the organization owner create an org config. I think this button should always be available for the organization owner without hiding it at the beginning.

[ ] We should add in the "plugin" section a button to go in the secrets page (the same logic of the organization config, you can put the new button on the left of the organization config button)

Alright.

3. The secret input don't load the secret: if you store a secret, save and then edit it again the UI doesn't load the secret.

That's as per the design.

[ ] I like you hide the secret, but the input element seems disabled I prefer something like "password" -> "p***d"

Alright.

[ ] In order to make the GUI more similar to the exiting sections you could modify the new sections (user plugin, org plugin, secrets, for all of them you used the same base code, very good!) similar to the "Jobs": You could split in two sub-section (analyzer and connector) and also add a bare where is possible to filter the elements.

Alright.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 0e615d1f1d1734ac0175f035e3ac26b3826f1a04 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging b96a1ae8680d4a8d348ab87f0fc26b5f96ec2901 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging e8d92ae53e20a35971f66d23890baec5aee630d4 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

1. I noticed the button to access to the organization config in the "plugin" section appears after the organization owner create an org config. I think this button should always be available for the organization owner without hiding it at the beginning.

I was unable to reproduce this issue.

            {isUserOwner ? (
              <Link
                to="/me/organization/config"
                style={{ color: "inherit", textDecoration: "inherit" }}
              >
                <Row>
                  <Button className="my-2">
                    <BsPeopleFill className="me-2" /> Organization{" "}
                    {organization.name}&apos;s custom config
                  </Button>
                </Row>
              </Link>
            ) : null} 

The code also suggests that it shouldn't have occurred. Must have been a fluke.

[devmrfitz]

* [ ]  In order to make the GUI more similar to the exiting sections you could modify the new sections (user plugin, org plugin, secrets, for all of them you used the same base code, very good!) similar to the "Jobs": You could split in two sub-section (analyzer and connector) and also add a  bare where is possible to filter the elements.

That's what I'd initially planned. But useDataTable used from certego-ui is not built to allow editing and modification of elements. It is built like a list and retrieve view whereas we need a list and write type of view. That's why I created an entire new component, PluginData.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 2f77875352c761e3c7ebdd19174014d5795d3c95 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[drosetti]

1. I noticed the button to access to the organization config in the "plugin" section appears after the organization owner create an org config. I think this button should always be available for the organization owner without hiding it at the beginning.

I was unable to reproduce this issue.

I tested with a clean environment, maybe is something that only happens at the startup. Another thing I noticed happen with the plugin config in case you delete the organization, the button is still active and the page show an unexpected error. I think the button should be disabled and the error be more specific.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging a154e693b12778472b2970817a06bca5d98b641f into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 5036fb2d35e790ee99993533b6383cedc7ac6595 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[codecov[bot]]

Codecov Report

Merging #1136 (bcfc22c) into develop (aa8820f) will increase coverage by 11.25%. The diff coverage is 81.48%.

:exclamation: Current head bcfc22c differs from pull request most recent head 9d44de6. Consider uploading reports for the commit 9d44de6 to get more accurate results

@@             Coverage Diff              @@
##           develop    #1136       +/-   ##
============================================
+ Coverage    66.75%   78.01%   +11.25%     
============================================
  Files           95      194       +99     
  Lines         3706     7436     +3730     
  Branches       519     1098      +579     
============================================
+ Hits          2474     5801     +3327     
- Misses         941     1213      +272     
- Partials       291      422      +131     

Impacted Files	Coverage Δ
...pp/analyzers_manager/file_analyzers/vt/vt3_scan.py	0.00% <0.00%> (ø)
..._manager/observable_analyzers/dns/dns_responses.py	71.42% <ø> (ø)
...i_app/analyzers_manager/file_analyzers/pdf_info.py	73.91% <20.00%> (ø)
...lyzers_manager/observable_analyzers/vt/vt3_base.py	37.24% <37.24%> (ø)
...i_app/analyzers_manager/file_analyzers/suricata.py	41.17% <41.17%> (ø)
api_app/analyzers_manager/file_analyzers/clamav.py	44.44% <44.44%> (ø)
...pp/analyzers_manager/observable_analyzers/cymru.py	70.83% <50.00%> (ø)
...i_app/analyzers_manager/file_analyzers/doc_info.py	48.73% <52.94%> (ø)
.../analyzers_manager/observable_analyzers/maxmind.py	30.33% <53.84%> (ø)
...p/analyzers_manager/file_analyzers/cape_sandbox.py	56.43% <56.43%> (ø)
... and 211 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e917a8d...9d44de6. Read the comment docs.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 69aba3d7a359077a2f62d579785805f17b191296 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 6cd156eb764b3fc47fabefee6fcbe2a580cc398e into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 73e29b04e158c6530b33266ab727a5b11cd36a37 into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

* [ ]  3 - My fault, I didn't highlight it enough: I think is important the secret button is placed on the left of the plugin buttons and horizontally aligned with the button below, in this way we save vertical space and in the table can be show more elements. We could also put org plugin button above the user config button: in this way when an user create and org the user config is always below and we improve the UX (maybe i don't explain me well, in case tell me and i'll be more accurate).

Does this look good?

[drosetti]

* [ ]  3 - My fault, I didn't highlight it enough: I think is important the secret button is placed on the left of the plugin buttons and horizontally aligned with the button below, in this way we save vertical space and in the table can be show more elements. We could also put org plugin button above the user config button: in this way when an user create and org the user config is always below and we improve the UX (maybe i don't explain me well, in case tell me and i'll be more accurate).

Does this look good?

The two buttons in in the second row should have the same height, "your custom config" text should be in one line. I'm not sure about the width of the org plugin config: maybe if is only above "you custom config" suggests to the user that there are two "columns", and in the right there are the plugin config buttons. I think we will wait @mlodic

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 6b78cadb45ee17a83aa1795622b5a0b87666390b into fc109d342557d83d3e67e62187c9cd8f301310a7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

* [ ]  4 - My fault (again): Even if we don't have a class in the certego-ui for the table with inputs we need to improve the current section (with a sub-section for the analyzer and another for the connectors, but also with a table with an header row where users can filter the secrets). Maybe the best way could be add in the certego-ui an abstract class with a table and two class that extends it: one for the tables currently used and another for a table with inputs. In this way the job/plugins tables maintain the current behaviour (use the table currently use), we have a table with inputs for the plugin config/secrets and we also have a table class extended by the other two, to avoid code duplication (where is possible).

It would be a table with filtering, dropdown fields and hidden as well as visible editable input fields. Wouldn't it be quite an overcomplicated view for data with relatively (as compared to jobs) much fewer and less frequently changing entries?

[mlodic]

I am trying to figure out the discussions and the possible design problems that arose.

1. buttons rendering

The two buttons in in the second row should have the same height, "your custom config" text should be in one line. I'm not sure about the width of the org plugin config: maybe if is only above "you custom config" suggests to the user that there are two "columns", and in the right there are the plugin config buttons. I think we will wait @mlodic

Right now how those buttons are rendered does not satisfy my expectations too. One idea that just came up to my mind is to replicate the same button sections that IntelOwl uses in the "Job Result" page. See this as an example:

We could put those 3 buttons at the top right using the same rendering used there. All the buttons would be in the same line (we have space) and it would be more intuitive for the user because he would expect the "action" buttons always in the same place and rendered in the same way. Thoughts about that?

2. general adjustments

• [x] I noticed that some analyzers could have long names. Can we use more the available space in the Custom Config/Plugins Secrets sections? maybe doubling the space for analyzer names.

• [x] I think that "your Custom Config" and "Plugin Secrets" are not the best names that we can give to those sections. I'd use something more intuitive and that binds them together logically. My idea is "Parameters Config" and "Secrets Config". (and "Organization Parameters Config"). In that it is clear what we are changing in those sections.

• [x] Plus, it would make sense to add a "Note: ..." in this visualization, similarly to how Eshaan added a note in the "Plugin" section to explain clearly to the users how they can use that page. For example, we can say that the "custom config" basically allow the users to change the "Parameters" for each plugin + we can explicitly say that the User config overwrites the Org config (otherwise this is written only in the docs so it could not be found easily by users). Same thing for "Plugin Secrets".

• [ ] For "plugin secrets" we should not show the attribute key but the attribute value in my opinion. In that way it is clearer which variable we are changing. So, in that example, "api_key_name" would be "ANOMALI_THREATSTREAM_API_USER"

• [x] While it is cool and right that the "plugin secrets" values are obscured, we should add a way to show them. Otherwise there is no way for the users to understand what he insterted. Eshaan already solved this problem in the "API access" section: We can re-use this.

• [x] ("plugin secrets" section) If the user clicks on the modify button and tries to change a "pluigin secrets" already saved, if the new value of the secret would be empty, it would trigger a 500 error:

KeyError at /api/plugin-credential/2

'value'


/opt/deploy/intel_owl/api_app/serializers.py, line 737, in validate

                raise ValidationError(

                    f"{category} {attrs['plugin_name']} does not "

                    f"have attribute {attrs['attribute']}."

                )

            inclusion_params = attrs.copy()

            exclusion_params = {}

            inclusion_params.pop("value")

     

• [x] I think that it makes no sense for an end user that could not even know what JSON is to get a "please insert a JSON valid value here" if the would just insert test instead of "test". I know we already discussed about the JSON thing and backend side makes sense. The point is not changing that behavior but to allow the user to insert a string like test without the need to add ". (we could add " via javascript if we need it, solving the problem for the user) That would simplify everything from an user perspective

3. new sections' permissions

• [x] If I login with an user that belongs to an org and then I go to the "Organization Config" section, I would see this: But this is wrong because:

• I am already part of an org and right now users cannot be part of more than one org. so we should not ask the user to create an org in this case.

• A normal org member should be able to see how his admin has configured the analyzers. What he should not do is to change them. So we should provide the same view but read-only.

• [ ] ~"Plugin secrets" section should be viewed and modified only by staff users. Is already like that?~ ~Also, I think that the section should not be visible here~ ~by normal users because there is no need that they know that that section exists. Same thing for the "Django Admin Interface" link~

EDITED: I forgot that, obviously, also the secrets (like the params) should have 2 different types of configuration, one for the user and one for the org. Otherwise all the multi-tenancy things won't work. So basically both cases should work the same. From GSoC project description:

The model will also have a field to denote the scope of the credentials (user-level
or organisation-level). Only the organisation owner can edit organisation level
credentials. Now, we just need to add the UI to the frontend and call the created
APIView’s endpoints.

4. Refactor of new sections

4 - My fault (again): Even if we don't have a class in the certego-ui for the table with inputs we need to improve the current section (with a sub-section for the analyzer and another for the connectors, but also with a table with an header row where users can filter the secrets). Maybe the best way could be add in the certego-ui an abstract class with a table and two class that extends it: one for the tables currently used and another for a table with inputs. In this way the job/plugins tables maintain the current behaviour (use the table currently use), we have a table with inputs for the plugin config/secrets and we also have a table class extended by the other two, to avoid code duplication (where is possible).

It would be a table with filtering, dropdown fields and hidden as well as visible editable input fields. Wouldn't it be quite an overcomplicated view for data with relatively (as compared to jobs) much fewer and less frequently changing entries?

Even if I agree with the fact the those new sections are changed and used rarely if compared to other sections, on the other hand it is important to keep uniform the different sections of the application. In the end, those new sections contain tables so Daniele's point of view makes sense to me. The point is that these tables need to be editable tables so that would require several changes, at least the ones that Daniele highlighted. Then, about the "overcomplicated view": this may make sense in some cases but we should take into account IntelOwl deployments where a lot of configuration is added too. In that case, filtering capabilities would be really useful. Plus, as already said, I prefer to keep the frontend uniform between sections because this helps UX so I think it would make sense to do that work too. However I'd do that once all the other things are solved, in a different PR that would be the refactor of these sections. In this way, meanwhile, we could already start to use the new sections once this PR is solved.

ADDED

5. Additional related work

see #1164

Thoughts?

[devmrfitz]

I forgot that, obviously, also the secrets (like the params) should have 2 different types of configuration, one for the user and one for the org. Otherwise, all the multi-tenancy things won't work. So basically both cases should work the same.

I was thinking of modifying the CustomConfig model and merging PluginCredential into it, since they are becoming so similar. What's your perspective?

Also, weirdly didn't get an email of your last comment.

@mlodic

[devmrfitz]

I think that it makes no sense for an end user that could not even know what JSON is to get a "please insert a JSON valid value here" if the would just insert test instead of "test". I know we already discussed about the JSON thing and backend side makes sense. The point is not changing that behavior but to allow the user to insert a string like test without the need to add ". (we could add " via javascript if we need it, solving the problem for the user) That would simplify everything from an user perspective

That still maintains the case of: If a variable was initially abc123 and I change it to 123, is it now a string or number? Assumptions are risky because in some analyzers (like CyberChef), we know that the 2 will produce different behaviours

[mlodic]

That still maintains the case of: If a variable was initially abc123 and I change it to 123, is it now a string or number? Assumptions are risky because in some analyzers (like CyberChef), we know that the 2 will produce different behaviours

if the analyzer supports a string, then 123 would be a string and we do the " addition, while if the analyzer supports an int that would be converted to an int. This is what a simple user would expect from the application. We should solve the problem for the users, to keep things simple and intuitive.

I was thinking of modifying the CustomConfig model and merging PluginCredential into it, since they are becoming so similar. What's your perspective?

This can make sense. We could also support them in the same visualization in the end. Like we have the following for "Plugins" we could have the same for the "Config" ("Parameters", "Secrets")

[devmrfitz]

Apologies for taking so long. The refactor needed to merge the two models is much more extensive and requires much more abstraction that I had initially assumed

[devmrfitz]

@mlodic Do I seem to be going on the right path?

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 0fbbb110af3ef6518cc98e6ae4b748eab161f08b into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 23e6e43055586019d9c4e58e39ad65e7c7fd3317 into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 019b720507e1f1276495360eaf1b298ea68ebdbd into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 0df0ac5c5f7a8dde3d2bd73f95f95b72a208efc4 into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

@mlodic @drosetti Does this look good?

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 8dab9244f17cf721754a2ee9e5d56fa924a6a111 into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 6804de83b5123bce706981cb96b2a51cf3f98cc7 into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 226b9e6e2188a2fc8ca3bed0d4549aa63190f14b into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 67e7ed8d3de58480361fda8e018489c8bfdeecb2 into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging a66969159113fe4f65e06db9d4887b2815889546 into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 92255ddbc9440eb6798c9f52218158e8ee2b594b into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

For "plugin secrets" we should not show the attribute key but the attribute value in my opinion. In that way it is clearer which variable we are changing. So, in that example, "api_key_name" would be "ANOMALI_THREATSTREAM_API_USER"

Since we are no longer using environment variables as plugin secrets, the env_var_key attribute is redundant. If we show it here, we would be introducing an unnecessary dependency over it. Instead, we could look into naming the actual secrets in a better way i.e. naming the secret something like ANOMALI_THREATSTREAM_API_USER instead of api_key_name.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 538b0fc4370598b82ac45a7080581afe994381d4 into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

@mlodic All done. Kindly re-review

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 83972eec70e817aba2ac34667c18e66e73d9f0be into 9a82ac93ef4c966c8a73f384ff17c60a66b9cebe - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[mlodic]

@mlodic @drosetti Does this look good?

it does! but it would be even cooler if "analyzers/connectors" and "org plugin" would be in the same line

[mlodic]

plugin secrets

that could make sense but would require to do non-retrocompatible changes with the analyzer_config.json file. I don't know if we want to go in that path right now. I mean, it could be faster for you to just modify that in the frontend so we can move that refactor to a later time. Thoughts?

[devmrfitz]

plugin secrets

that could make sense but would require to do non-retrocompatible changes with the analyzer_config.json file. I don't know if we want to go in that path right now. I mean, it could be faster for you to just modify that in the frontend so we can move that refactor to a later time. Thoughts?

The refactor we do in future would again be a db-level migration, as we'll need to go from picking env_var_key to api_key_name. Hence we risk getting locked into the old format.

[devmrfitz]

@mlodic @drosetti Does this look good?

it does! but it would be even cooler if "analyzers/connectors" and "org plugin" would be in the same line

That would require modifying the RouterTabs component in @certego/certego-ui. Should I look into that?

[lgtm-com[bot]]

This pull request introduces 1 alert when merging f41665457be05bcd1eb53f082d8a0f175fbdff75 into e917a8d7034a1049a7ff4f4b70502046c2d069aa - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[mlodic]

That would require modifying the RouterTabs component in @certego/certego-ui. Should I look into that?

that would be great! Otherwise another solution to save space could be to put that section (new buttons you made) in the line of the "Analyzers 166 total" name. Wouldn't it be easier?

[devmrfitz]

I was able to achieve this without breaking backward compatibility in @certego/certego-ui. Does it look good?

[mlodic]

it looks great! I'll merge that certego-ui PR and make a release so you can pin that release here, ok?

[mlodic]

ah,. just a little thing here: if you notice, there is extra space below the selected button, so just a little fix on sizes should help

[lgtm-com[bot]]

This pull request introduces 1 alert when merging bee3742b0ed4e8f9fd3dad0dad956c89c4e09ab8 into e917a8d7034a1049a7ff4f4b70502046c2d069aa - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[devmrfitz]

if you notice, there is extra space below the selected button, so just a little fix on sizes should help

@mlodic Fixed

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 5931510f93abcd89f69bfbde87f6ed8d9be04003 into df4e6f4f0fbce68672c2e9bcf6161c9c06944d58 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[mlodic]

published certego 0.1.2 with your change

[devmrfitz]

published certego 0.1.2 with your change

:+1: Have already updated the package.json. Any other pending changes for this PR?

[mlodic]

I'm checking the results:

New buttons "your plugin config": super cool Overall user experience: cool, love it

Just very little adjustments:

• [x] I noticed that the column where there is the name of the analyzer/connectors expands based on how much space it needs. Cool. However, sometimes (like the following example) the result could not be so friendly in the end if all the element in that column are all with different lenghts. Could it be possible to intervene in some way here frontend-side? fixed here

• [x] there is still the old name here: fixed here

• [x] can we find fancier icons? cause these are the same of the plugins (something like an investigator for secrets for instance) fixed here

• [x] if I add two values for the same parameter, a validation error is correctly triggered but that is an unmanaged error frontend side. A proper toast should be shown instead

• [x] the explanation is really useful but it would be even more if there would an embedded link to the organization's configuration Plus, you could add the very same explicit section also in the org part here:

After those we can move on to either the point 4 or 5. I would focus on point 5 if possible because it would make the project more complete.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging a39324002da87cd7dca2ff1231d1903dbad78675 into 8786a28c9799ef65928f2faef19b1beb2916e1c7 - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 8d6f6ada95059257beaa03efa5d8f1ee5a8247a8 into ba8a4f42bc60a3e5b6b6c286a540f38e6e6b9bfc - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging bcfc22c66b55d59eea828015633f6fc86b58e22c into ba8a4f42bc60a3e5b6b6c286a540f38e6e6b9bfc - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 9d44de6ebf43d5a2c02d28d98326331de3698a51 into ba8a4f42bc60a3e5b6b6c286a540f38e6e6b9bfc - view on LGTM.com

new alerts:

• 1 for Clear-text logging of sensitive information