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

Add security check #7247

Merged
merged 25 commits into from
Mar 10, 2021
Merged

Add security check #7247

merged 25 commits into from
Mar 10, 2021

Conversation

mtrezza
Copy link
Member

@mtrezza mtrezza commented Mar 5, 2021

New Pull Request Checklist

Issue Description

Adds security check for Parse Server.

Related issue: closes #7246

Approach

Adds a modularized feature to run pre-defined checks to identify weak security settings across Parse Server.

Features:

  • Adds a new endpoint https://example.com/parse/security that returns the security report in JSON format for automatic processing or display in the Parse Dashboard.
  • Optionally runs security check at server start with log output; log output is always optional to discourage from exposing weak security settings in the logs.

Implementation:

  • Adds a new SecurityRouter with masterKey enforcement for the /security endpoint
  • Includes report version management to mitigate necessity of breaking changes in the report syntax
  • Checks are instantiated on demand, no memory residence
  • Compatible with future on-the-fly changes of server config without server restart
  • All tests and logic in a central module, PR is minimally invasive to Parse Server
  • Checks can be synchronous or asynchronous scripts
  • Customizable, allows to override built-in / add custom security checks via Parse Server configuration; this easy access should encourage developers to create their own security checks and share them with the community and consider them as built-in candidates
  • Highly modular to make it easy for developers to add new checks in less time

Example configuration

const server = new ParseServer({
  security: {
    enableCheck: true,         // Enables security checks including `/security` endpoint
    enableCheckLog: true,  // Enables log output; required to auto-run security check on server start
    checkGroups: [ ... ]        // Custom security checks
  },
  ...otherOptions
});

Example output

###################################
#                                 #
#   Parse Server Security Check   #
#                                 #
###################################

Warning: 2 weak security setting(s) found!
2 check(s) executed
0 check(s) skipped

- Parse Server Configuration
   ❌ Secure master key
      Warning: The Parse Server master key is insecure and vulnerable to brute force attacks. Choose a more complex master key with a combination of upper- and lowercase characters, numbers and special characters.
   ❌ Security log disabled
      Warning: Security report in log. Set Parse Server configuration `security.enableCheckLog` to false.
###################################
#                                 #
#   Parse Server Security Check   #
#                                 #
###################################

0 weak security setting(s) found
1 check(s) executed
0 check(s) skipped

- Parse Server Configuration
   ✅ Secure master key

Report Schema

The report returned by the /security endpoint follows this schema:

{
    report: {
      version: "1.0.0", // The report version, defines the schema
      state: "fail"     // The disjunctive indicator of failed checks in all groups.
      groups: [         // The check groups
        {
          name: "House",            // The group name
          state: "fail"             // The disjunctive indicator of failed checks in this group.
          checks: [                 // The checks
            title: "Door locked",   // The check title
            state: "fail"           // The check state
            warning: "Anyone can enter your house."   // The warning.
            solution: "Lock your door."               // The solution.
          ]
        },
        ...
      ]
    }
}

It includes the report version to implement other report schemas in the future. The report request can include the requested schema version, this way a schema change does not lead to a breaking change and multiple schemas can be offered in parallel, making it easier to follow a phased deprecation policy.

Adding checks

The modular implementation should make it easy for a developer to add new tests when submitting a PR for a new feature. In addition to the feature itself, developers already spend time writing tests and docs. The security checks implementation is designed to require minimal effort in an intuitive interface.

To add a new test:

  1. Look into ./Security/CheckGroups/ to see whether there is an existing CheckGroup[Category].js file of the category to which the test belongs (Database, Server configuration, etc.).
  2. If not, add a new ./Security/CheckGroups/CheckGroup[Category].js file and add the tests:
class CheckGroupNewCategory extends CheckGroup {
  setName() {
    return 'New Category';
  }
  setChecks() {
    return [
      new Check({
        title: 'Testing this',
        warning: 'Warning message if failed',
        solution: 'Solution message to resolve',
        check: () => {    
          return;     // Example of a passing check
        }
      }),
      new Check({
        title: 'Testing that',
        warning: 'Warning message if failed',
        solution: 'Solution message to resolve',
        check: async () => {  
          throw 1;     // Example of a failing check
        }
      }),
    ];
  }
}
  1. If a new file had to be created (which should rarely happen), the file has to be added to ./Security/CheckGroups/CheckGroups.js, which the collector that merges all check groups:
export { default as CheckGroup[NewCategory] } from './CheckGroup[NewCategory]';

Custom security checks

The security checks can be fully customized, by reusing all or only some of the built-in check groups and adding custom check groups. A custom check group collector can be passed into the Parse Server configuration:

const server = new ParseServer({
  security: {
    enableCheck: true,
    enableCheckLog: true,
    checkGroups: [ CustomCheckGroup, CheckGroupDatabase ]
  },
  ...otherOptions
});

Possible future extensions

  • Add severity as check property as level of vulnerability (high, medium, low)
  • Allow to skip individual checks that are unnecessary for a developer, e.g. database connection password when using a private cloud environment

Side effects:

  • Adds a nifty helper to permutate config parameters for testing, see Utils.getObjectKeyPermutations
  • Adds a centralized parameter validator, see Utils.validateParams

Credits

Based on the idea by @davimacedo
Thanks to @dblythy for the initiative in #6973 to keep the discussion around this idea alive

TODOs before merging

  • Add test cases
  • Add a basic set of security checks to start with (more can be added by others as PR)
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Change security check docs how to add new tests

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #7247 (77a9404) into master (36c2608) will increase coverage by 0.02%.
The diff coverage is 96.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7247      +/-   ##
==========================================
+ Coverage   94.04%   94.06%   +0.02%     
==========================================
  Files         172      179       +7     
  Lines       12970    13148     +178     
==========================================
+ Hits        12197    12368     +171     
- Misses        773      780       +7     
Impacted Files Coverage Δ
src/Utils.js 95.74% <94.44%> (-0.81%) ⬇️
src/Security/CheckGroups/CheckGroupDatabase.js 94.73% <94.73%> (ø)
src/Security/CheckRunner.js 94.82% <94.82%> (ø)
src/Security/CheckGroups/CheckGroupServerConfig.js 95.23% <95.23%> (ø)
src/Config.js 93.36% <100.00%> (+0.33%) ⬆️
src/Options/Definitions.js 100.00% <100.00%> (ø)
src/Options/index.js 100.00% <100.00%> (ø)
src/ParseServer.js 89.83% <100.00%> (+0.23%) ⬆️
src/Routers/SecurityRouter.js 100.00% <100.00%> (ø)
src/Security/Check.js 100.00% <100.00%> (ø)
... and 10 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 36c2608...d9aeb73. Read the comment docs.

@dblythy
Copy link
Member

dblythy commented Mar 6, 2021

Thanks for picking this up. You’ve taken this to another level that I couldn’t have imagined. I haven’t looked through it with much detail yet but it looks great, can’t wait to use in the next version. Feel free to remove me from the changelog, this is your fantastic work 👍

@mtrezza
Copy link
Member Author

mtrezza commented Mar 6, 2021

Thanks @dblythy, I think it would be beneficial to have you in the changelog because we did a lot of brainstorming together in preparation for this and you have an understanding what this is about, which may be helpful if someone looks for points of contact in the changelog. If you want to be removed I would do that of course.

I think you talked about adding a Parse Dashboard page to display these checks. It would be amazing to see these checks in a nice UI. If you already had some thoughts about it and have some requirements regarding the report schema (see example above), I can add this in before merging. I opened parse-community/parse-dashboard#1665 for this.

@mtrezza
Copy link
Member Author

mtrezza commented Mar 10, 2021

@davimacedo, @dplewis, this is ready for review, again as summary:

It contains only some sample checks, but the main purpose of this PR is to:

  • provide a clean framework for everyone to submit PRs for checks to add
  • begin requiring security checks for new features where applicable
  • provide the report schema as interface for a potential PR to display the report in Parse Dashboard

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Looks good! Very clean

@dplewis dplewis merged commit bee889a into parse-community:master Mar 10, 2021
Arul- pushed a commit to Arul-/parse-server that referenced this pull request Mar 25, 2021
* added Parse Server security option

* added SecurityRouter

* added Check class

* added CheckGroup class

* moved parameter validation to Utils

* added CheckRunner class

* added auto-run on server start

* added custom security checks as Parse Server option

* renamed script to check

* reformat log output

* added server config check

* improved contributing guideline

* improved contribution guide

* added check security log

* improved log format

* added checks

* fixed log fomat typo

* added database checks

* fixed database check

* removed database auth check in initial version

* improved contribution guide

* added security check tests

* fixed typo

* improved wording guidelines

* improved wording guidelines
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@ArMouReR
Copy link

Hi,
I have recently updated to 5.0.0-beta.7 and wanted to run security checks on my server.
Here is what I have added to index.js

  logLevel: "error",
  security: {
      enableCheck: true,		// Enables security checks including `/security` endpoint
      enableCheckLog: true		// Enables log output; required to auto-run security check on server start
  },
  filesAdapter: new S3Adapter(

The problem that I don't see anything in the log, moreover posting to /parse/security returns: "Cannot POST /parse/security"

What I have missed, how can debug it ?

@ArMouReR
Copy link

Answering to myself, if someone will land here:
You need to do get request to /parse/security.
And it can also be easy done from dashboard:
Go to: API console->REST console
Request: get
Endpoint: security
Use master key: On

Should it be added to docs?

@mtrezza
Copy link
Member Author

mtrezza commented Feb 17, 2022

Sure, please open a PR with the suggested changes.

@mtrezza mtrezza mentioned this pull request Mar 12, 2022
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
@mtrezza mtrezza deleted the add-security-check branch March 24, 2022 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add security checks
5 participants