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.

Sign up for GitHub

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

Jump to bottom

feat(security): restrict admin actions to IP ranges #46473

Merged
merged 3 commits into from
Jul 22, 2024
Merged

feat(security): restrict admin actions to IP ranges #46473

merged 3 commits into from
Jul 22, 2024

Conversation

Altahrim
Copy link
Collaborator

@Altahrim Altahrim commented Jul 12, 2024 •
edited
Loading

Summary

Restrict admin actions to IP ranges
When administrator IP address is not in specified range, all admin actions are hidden/forbidden.

Checklist

@Altahrim Altahrim added 2. developing Work in progress security php Pull requests that update Php code labels Jul 12, 2024
@Altahrim Altahrim self-assigned this Jul 12, 2024
@AndyScherzinger AndyScherzinger added this to the Nextcloud 30 milestone Jul 12, 2024
nickvergessen
nickvergessen requested changes Jul 15, 2024
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

No in-if assignments please

lib/private/Security/RemoteIpAddress.php Outdated Show resolved Hide resolved
lib/private/Security/RemoteIpAddress.php Outdated Show resolved Hide resolved
@Altahrim Altahrim force-pushed the feat/restrict_admin_to_ips branch 4 times, most recently from 90da5d8 to 8b24270 Compare July 16, 2024 10:00
@Altahrim Altahrim marked this pull request as ready for review July 16, 2024 10:01
@Altahrim Altahrim force-pushed the feat/restrict_admin_to_ips branch 5 times, most recently from f534588 to da6dd95 Compare July 17, 2024 07:20
@Altahrim Altahrim added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 17, 2024
nickvergessen
nickvergessen approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Definitely needs documentation, so that other "admin alike endpoints" are aware and can integrate it and use IGroupManager::isAdmin() instead of checking for the admin group.

@Altahrim Altahrim added the pending documentation This pull request needs an associated documentation update label Jul 17, 2024
@Altahrim Altahrim force-pushed the feat/restrict_admin_to_ips branch 3 times, most recently from 17a6845 to fb9866d Compare July 19, 2024 09:08
@Altahrim Altahrim mentioned this pull request Jul 19, 2024
Merged
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 19, 2024
View reviewed changes
lib/public/Security/Ip/IFactory.php Outdated
* Creates a range from string
*
* @since 30.0.0
* @throws on invalid range

Check failure

Code scanning / Psalm

UndefinedDocblockClass Error

Docblock-defined class, interface or enum named OCP\Security\Ip\on does not exist
lib/public/Security/Ip/IFactory.php Outdated
* Creates a address from string
*
* @since 30.0.0
* @throws on invalid IP

Check failure

Code scanning / Psalm

UndefinedDocblockClass Error

Docblock-defined class, interface or enum named OCP\Security\Ip\on does not exist
@joshtrichards
Copy link
Member

Related open issues this PR may close: #29294 & #38609

This was referenced Jul 19, 2024
Open
Open
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 19, 2024
View reviewed changes
lib/private/Server.php Outdated
@@ -1403,6 +1406,10 @@

$this->registerAlias(\OCP\TaskProcessing\IManager::class, \OC\TaskProcessing\Manager::class);

$this->registerAlias(IRemoteAddress::class, RemoteAddress::class);

$this->registerAlias(\OCP\Security\Ip\Factory::class, \OC\Security\Ip\Factory::class);

Check failure

Code scanning / Psalm

UndefinedClass Error

Class, interface or enum named OCP\Security\Ip\Factory does not exist
nickvergessen
nickvergessen requested changes Jul 19, 2024
View reviewed changes
lib/public/Security/Ip/IFactory.php Outdated Show resolved Hide resolved
lib/public/Security/Ip/IFactory.php Outdated Show resolved Hide resolved
lib/private/Security/Ip/RemoteAddress.php Show resolved Hide resolved
lib/private/Security/Ip/RemoteAddress.php Show resolved Hide resolved
lib/private/Server.php Outdated Show resolved Hide resolved
Altahrim and others added 3 commits July 19, 2024 16:28
@Altahrim
feat(security): restrict admin actions to IP ranges
202e5b1 
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
@nickvergessen @Altahrim
feat(security): Add public API to allow validating IP Ranges and chec…
047479c 
…king for "in range"

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
@Altahrim
feat(Security): add Factory for IP addresses and ranges
f1d97a3 
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
@AndyScherzinger AndyScherzinger merged commit c2a571e into master Jul 22, 2024
166 checks passed
@AndyScherzinger AndyScherzinger deleted the feat/restrict_admin_to_ips branch July 22, 2024 08:10
@szaimen szaimen mentioned this pull request Jul 22, 2024
Open
@szaimen
Copy link
Contributor

szaimen commented Jul 22, 2024 •
edited
Loading

Cool feature 🎉🎉🎉🎉🎉

I wonder, would it make sense to document this under https://docs.nextcloud.com/server/latest/admin_manual/installation/harden_server.html?

@AndyScherzinger
Copy link
Member

I wonder, would it make sense to document this under https://docs.nextcloud.com/server/latest/admin_manual/installation/harden_server.html?

Yes, exactly there. I think @sorbaugh already discusses this with @Altahrim - we talked about this 30 minutes ago 😁

@Altahrim Altahrim mentioned this pull request Jul 24, 2024
Merged
@Altahrim
Copy link
Collaborator Author

Added here: nextcloud/documentation#12059

@Altahrim Altahrim removed 3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update labels Jul 24, 2024
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@sorbaugh sorbaugh sorbaugh approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@kesselb kesselb Awaiting requested review from kesselb

Assignees

@Altahrim Altahrim

Labels
php Pull requests that update Php code security 🍂 2024-Autumn
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Altahrim @joshtrichards @szaimen @AndyScherzinger @nickvergessen @kesselb @sorbaugh