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

fix: IPv6 ranges not recognized for options masterKeyIPs, maintenanceKeyIPs #8501

Closed
wants to merge 48 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Apr 10, 2023

Pull Request

Issue

Currently, an ipv6 range does not seem to validate ipv4 addresses

Closes: #8421

Approach

Adds cases to allow all IPs

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

mtrezza and others added 24 commits March 25, 2022 19:47
## [5.2.1-alpha.1](parse-community/parse-server@5.2.0...5.2.1-alpha.1) (2022-03-26)

### Bug Fixes

* return correct response when revert is used in beforeSave ([parse-community#7839](parse-community#7839)) ([f63fb2b](parse-community@f63fb2b))
## [5.2.1-alpha.2](parse-community/parse-server@5.2.1-alpha.1...5.2.1-alpha.2) (2022-03-26)

### Performance Improvements

* reduce database operations when using the constant parameter in Cloud Function validation ([parse-community#7892](parse-community#7892)) ([48bd512](parse-community@48bd512))
# [5.3.0-alpha.2](parse-community/parse-server@5.3.0-alpha.1...5.3.0-alpha.2) (2022-03-27)

### Bug Fixes

* security upgrade parse push adapter from 4.1.0 to 4.1.2 ([parse-community#7893](parse-community#7893)) ([ef56e98](parse-community@ef56e98))
@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: allow all masterKeyIPs with ipv6 range fix: Allow all masterKeyIPs with ipv6 range Apr 10, 2023
@parse-github-assistant
Copy link

Thanks for opening this pull request!

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (8d3117e) 94.32% compared to head (2b284a4) 94.34%.

❗ Current head 2b284a4 differs from pull request most recent head 8ca0279. Consider uploading reports for the commit 8ca0279 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8501      +/-   ##
==========================================
+ Coverage   94.32%   94.34%   +0.02%     
==========================================
  Files         186      186              
  Lines       14780    14795      +15     
==========================================
+ Hits        13941    13959      +18     
+ Misses        839      836       -3     
Files Coverage Δ
src/middlewares.js 96.96% <100.00%> (+0.14%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/middlewares.js Outdated Show resolved Hide resolved
Copy link
Member

@Moumouls Moumouls left a comment

Choose a reason for hiding this comment

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

i'll send a pr, we are not going in the right way here...

@dblythy
Copy link
Member Author

dblythy commented Apr 14, 2023

?

@Moumouls
Copy link
Member

@dblythy i'll send a pr in 1hour hold on 🙂

@Moumouls
Copy link
Member

Here my implementation suggestion #8510

Signed-off-by: Daniel <daniel-blyth@live.com.au>
@mtrezza
Copy link
Member

mtrezza commented Jun 19, 2023

@dblythy Could you please update your PR to get it ready for merge? #8510 is stale and @Moumouls isn't responding, so let's proceed with this PR.

@dblythy dblythy requested a review from a team June 20, 2023 03:08
@mtrezza
Copy link
Member

mtrezza commented Jun 20, 2023

Does this PR contain code of #8510, or is there anything it should contain?

Copy link
Member

@mtrezza mtrezza 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

@mtrezza mtrezza requested a review from Moumouls June 27, 2023 10:11
@mtrezza
Copy link
Member

mtrezza commented Jun 30, 2023

@andreisucman Could you test out this PR and see if it resolves the issue you described?

@andreisucman
Copy link

andreisucman commented Jun 30, 2023

@mtrezza Here is my test:

  1. I deleted node modules and package-lock.json
  2. I added parse server from this PR like so: "parse-server": "github:parse-community/parse-server#pull/8501", and ran npm i
  3. I set fake IP addresses to the .env like so "fe80::ddc6:9db0:e1c5:6000%12,192.112.68.125"
    They get split into an array in the server config like so masterKeyIps: process.env.MASTER_KEY_IPS.split(","),
  4. I tested changing user name via a cloud function, which involves saving the User object with the master key like so:
if (name.length < 4) return;
      rUser.set("name", name);
      await rUser.save(null, {
        useMasterKey: true,
 });

The operation ran with success on both localhost and production. No error logs.

@mtrezza
Copy link
Member

mtrezza commented Jul 1, 2023

Maybe that's because it's via Cloud Function, could you try again with an actual client (or via REST API) from outside of Cloud Code?

@andreisucman
Copy link

andreisucman commented Jul 1, 2023

@mtrezza Ok, I tested again in live environment

--- Test 1

Firstly I tested the current version - 6.3-alpha.3. just in case.

  1. I set the allowedIps to the same fake values "fe80::ddc6:9db0:e1c5:6000%12,192.112.68.125" and applied to the cluster
  2. I replaced the existing pods with new to use the update config
  3. Then I downloaded the config from the cluster and confirmed that allowedIps are set to the fake string
  4. Then I ran this function in node js using parse node sdk.
    require("dotenv").config();
    const Parse = require("parse/node");

    Parse.initialize(process.env.APP_ID, undefined, process.env.MASTER_KEY);
    Parse.serverURL = `${process.env.SERVER_URL}/parse`;
    const toSave = [];
    ...
    await Parse.Object.saveAll(toSave, { useMasterKey: true });

It worked with success even though the allowedIps has fake values different from the ingress ip.

----- Test 2

  1. I deleted node modules and package-lock.json
  2. I set the server to "parse-server": "github:parse-community/parse-server#pull/8501",
  3. I rebuilt the image and checked that my docker registry has only this single image (pushed a few seconds ago)
  4. I deleted the pods with the existing app
  5. I downloaded the cluster secret config and confirmed that the allowedIps are set to the fake Ips string "fe80::ddc6:9db0:e1c5:6000%12,192.112.68.125" (in its base64 encoded form - ZmU4MDo6ZGRjNjo5ZGIwOmUxYzU6NjAwMCUxMiwxOTIuMTEyLjY4LjEyNQ==)
  6. I installed new app pods with the current secret config
  7. I ran the same function from previous test using nodejs sdk and it succeeded.

Nothing is hardcoded in my app, so I'm pretty sure that the app used the provided fake allowedIps.

I recommend you to take my fake allowedIps values and test it on your end. And when you confirm that its working correctly on your dev server, I can test again in my live environment.

These 2 tests were for masterKeyIps option set to a different IP that is not in use.

And for this test masterKeyIps option set to the foreign (non-localhost) IP that you are making the request from and no ip addresses set, I can't do them as they would require dealing with pod ip addresses and rebuilding helm charts - too much work atm.

Maybe you could test it in a simplified production environment on your end.

@Moumouls
Copy link
Member

Moumouls commented Jul 6, 2023

The other pr is ready: #8510

@mtrezza
Copy link
Member

mtrezza commented Nov 19, 2023

Closing via #8510

@mtrezza mtrezza closed this Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The option "masterKeyIps" cannot be disabled
5 participants