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

Docs: Add plugin security development guide #1195

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

josmperez
Copy link
Contributor

Draft of new plugin security guide for collaboration with David Harris and others.

Copy link

github-actions bot commented Oct 9, 2024

Hello! 👋 This repository uses Auto for releasing packages using PR labels.

❌ This PR cannot be merged until the following issues are addressed:

  • This PR is missing one of the following labels: patch, minor, major, no-changelog.
  • Optionally if using a patch, minor or major label also add the release label if you would like this PR to trigger npm package publishing.
🏷️ More info about which labels to use
  • If the changes only affect the docs website, documentation, or this repository's tooling add the no-changelog label.
  • If there are changes to any of the npm packages src files please choose from one of the following labels:
    • 🐛 if this PR fixes a bug add the patch label
    • 🚀 if this PR includes an enhancement add the minor label
    • 💥 if this PR includes a breaking change add the major label
  • Optionally, if you would like this PR to publish new versions of packages when it is merged add the release label.

Copy link

@irenerl24 irenerl24 left a comment

Choose a reason for hiding this comment

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

Great doc, LGTM!


## General best practices

### Security audits and reviews
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want people to use gosec it should become part of the default scaffolding, maybe as a step in GH actions workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is - the validator runs in the release workflow, but we should mention that


### Data handling

- **Prevent data exposure**: Don’t expose sensitive data like user credentials, API tokens, or personally identifiable information (PII) through logs, requests, or the browser console.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should provide some example of how to do this. One way is to scan automatically for potentially exposed secrets with trufflehog. Which we also want to offer as part of the GH action checks: https://github.com/grafana/grafana-community-team/issues/224

### Data handling

- **Prevent data exposure**: Don’t expose sensitive data like user credentials, API tokens, or personally identifiable information (PII) through logs, requests, or the browser console.
- **Encrypt data**: Always encrypt sensitive data in transit using SSL/TLS, and encrypt at rest when needed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure how we are asking people to do that - or do you mean on the server side / not in the plugin?

Or do we want to say - store sensitive configuration in secureJsonData https://grafana.com/developers/plugin-tools/how-to-guides/data-source-plugins/add-authentication-for-data-source-plugins#store-configuration-in-securejsondata?


- **Use HTTPS**: Ensure all external requests use HTTPS.
- **Set timeouts**: Set timeouts for external requests to prevent them from hanging indefinitely.
- **Implement retry logic**: Use retry logic with exponential backoff for external requests.
Copy link
Collaborator

Choose a reason for hiding this comment

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

how is this bullet point "Implement retry logic" related to security?


- [OWASP Top 10](https://owasp.org/www-project-top-ten/) – The most critical security risks for web applications.
- [GoSec](https://github.com/securego/gosec) – A security analysis tool for Go.
- [npm audit](https://docs.npmjs.com/cli/v6/commands/npm-audit) – Check for vulnerabilities in npm dependencies.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- [npm audit](https://docs.npmjs.com/cli/v6/commands/npm-audit) – Check for vulnerabilities in npm dependencies.
- [npm audit](https://docs.npmjs.com/cli/commands/npm-audit) – Check for vulnerabilities in npm dependencies.

to avoid linking to outdated npm cli versions

Copy link
Contributor

@sympatheticmoose sympatheticmoose left a comment

Choose a reason for hiding this comment

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

Added some comments, but generally I think this needs a lot more discussion. I suspect elements are not quite accurate for plugin development, and others are too vague/generic to be actionable by developers


## General best practices

### Security audits and reviews
Copy link
Contributor

Choose a reason for hiding this comment

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

it is - the validator runs in the release workflow, but we should mention that


### Security audits and reviews

Before submitting a plugin for publication, conduct security audits and reviews. Use tools like `gosec` for Go and `eslint` for TypeScript to catch common issues. Peer reviews should focus on identifying injection vulnerabilities, improper handling of sensitive data, and weak authentication.
Copy link
Contributor

Choose a reason for hiding this comment

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

If its just about creating secure plugins, i don't see a need to mention submitting for publication, it applies to private plugins too.


### Minimizing dependencies

Keep dependencies to a minimum. Update packages regularly and remove unused ones. Use tools like `npm audit` and `go mod tidy` to identify outdated or vulnerable dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

this should mention our update cmd as well

### Secure coding practices

- **Don’t hardcode secrets**: Avoid hardcoding API keys, tokens, or passwords in your code.
- **Use least privilege**: Ensure your plugin uses the least privileged account or role when accessing resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this applicable?


Ensure [secure authentication methods](../how-to-guides/data-source-plugins/add-authentication-for-data-source-plugins.md) are in place for API interactions. Always verify that the user accessing your plugin has the appropriate permissions.

- **Authorization checks**: Confirm that users have the required roles before allowing access to certain actions within the plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

would need a guide


When the frontend communicates with the backend:

- **Use HTTPS**: Always use HTTPS to protect data.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct?


### Configuration validation

Validate user-provided configuration values to prevent configuration errors that could lead to security vulnerabilities like injection attacks.
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels too vague


### Storing secrets

Store API keys, tokens, and other secrets securely.
Copy link
Contributor

Choose a reason for hiding this comment

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

rpt of frontend?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔬 In review
Development

Successfully merging this pull request may close these issues.

4 participants