CLOUDSTACK-7958: Add configuration for limit to CIDRs for Admin API calls by wido · Pull Request #2046 · apache/cloudstack

wido · 2017-04-14T11:27:52Z

The global setting 'management.admin.cidr' is set to 0.0.0.0/0,::/0
by default preserve the current behavior and thus allow API calls
for Admin accounts from all IPv4 and IPv6 subnets.

Users can set it to a comma-separated list of IPv4/IPv6 subnets to
restrict API calls for Admin accounts to certain parts of their network(s).

This is to improve Security. Should a attacker steal the Access/Secret key
of a Admin account he/she still needs to be in a subnet from where Admin accounts
are allowed to perform API calls.

This is a good security measure for APIs which are connected to the public internet.

This PR also includes a commit to cleanup and improve NetUtils.

No existing methods have been altered. That has been verified by adding additional Unit Tests for this.

DaanHoogland

this refactor seems necessary evil.

DaanHoogland · 2017-04-14T12:33:01Z

            s_logger.warn("Haproxy stickiness policy for lb rule: " + lbTO.getSrcIp() + ":" + lbTO.getSrcPort() + ": Not Applied, cause:  backends are unavailable");
        }
-        if (publicPort.equals(NetUtils.HTTP_PORT) && !keepAliveEnabled || httpbasedStickiness) {
+        if (publicPort == NetUtils.HTTP_PORT && !keepAliveEnabled || httpbasedStickiness) {


why this? equals() seems more what is intended then ==
is making it an int really the best? as opposed to Integer (or String for that matter)

It is a port number so it should be a Int. It's a part of the NetUtils refactor. I found that only HTTP/HTTPS port were Strings.

I wanted to make the change in other files as small as possible.

DaanHoogland

great change, but I think logging can improve

DaanHoogland · 2017-04-14T12:39:31Z

    public static final ConfigKey<Long> DefaultUIPageSize = new ConfigKey<Long>("Advanced", Long.class, "default.ui.page.size", "20",
            "The default pagesize to be used by UI and other clients when making list* API calls", true, ConfigKey.Scope.Global);
+    public static final ConfigKey<String> ManagementAdminCidr = new ConfigKey<String>("Advanced", String.class, "management.admin.cidr",
+            "0.0.0.0/0,::/0", "Comma separated list of IPv4/IPv6 CIDRs from which Admin accounts can perform API calls", true, ConfigKey.Scope.Global);


argument for this default: backwards compatible
argument against: inherent security risk

I agree. I have set it to open for all for now. We can submit a different PR afterwards to change the default imho.

DaanHoogland · 2017-04-14T12:40:29Z

            }

+            if (CallContext.current().getCallingAccount().getType() == Account.ACCOUNT_TYPE_ADMIN) {
+                s_logger.debug("CIDRs from which Admin accounts are allowed to perform API calls " + adminCidrs);


you don't want to log this on every call.

You suggest setting this to TRACE instead of debug?

I was thinking trace here and debug or info on the first load of the cidr

DaanHoogland · 2017-04-14T12:41:13Z

+                    final String serializedResponse =
+                            apiServer.getSerializedApiError(HttpServletResponse.SC_UNAUTHORIZED, "IP-Address of remote not in configured Admin CIDR list",
+                                    params, responseType);
+                    HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_UNAUTHORIZED, responseType, apiServer.getJSONContentType());


this you do want to log on every attempt (WARN or INFO???)

True, true. I would say WARN?

wido · 2017-04-14T15:49:22Z

@DaanHoogland: I improved the logging as you suggested/requested.

A TRACE for every request and WARN when a request is denied. Tried this locally:

2017-04-14 15:45:58,901 WARN  [c.c.a.ApiServlet] (catalina-exec-17:ctx-5955fcab ctx-c572b42e) (logid:7b251506) Request by accountId 2 was denied since 192.168.122.1 does not match 127.0.0.1/8,::1/128

In this case only localhost (IPv4/IPv6) is allowed to perform requests.

wido · 2017-04-14T15:50:07Z

@PaulAngus This is what we talked about in Prague. Mind taking a look?

remibergsma · 2017-04-14T19:07:57Z

Nice @wido, will give it a go soon!

remibergsma · 2017-04-14T20:42:34Z

+                    final String serializedResponse =
+                            apiServer.getSerializedApiError(HttpServletResponse.SC_UNAUTHORIZED, "IP-Address of remote not in configured Admin CIDR list",
+                                    params, responseType);
+                    HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_UNAUTHORIZED, responseType, apiServer.getJSONContentType());


@wido Great work, works fine! I'd recommend adding a return; line after the writeHttpResponse on line 304 because otherwise it will generate invalid json:

Error processing json: {"listvirtualmachinesresponse":{"uuidList":[],"errorcode":401,"errortext":"IP-Address of remote not in configured Admin CIDR list"}}{"listvirtualmachinesresponse":{}}

With return; added it works as expected:

Regards, Remi

wido · 2017-04-15T10:34:20Z

Thanks @remibergsma. Added the return statement.

Thinking about it. Does a 401 sound good? Or should we maybe use a 403 Forbidden?

remibergsma · 2017-04-15T11:11:48Z

@wido I'm playing a bit with it, also because currently CloudMonkey displays a clear message but the UI will simply freeze and do nothing. Only when you look at the underlying API calls you'll see why it isn't working. I'm testing to see the http status code makes any difference of that we need to handle it in the UI somewhere.

We should also think about the order: first alert on the CIDR check and then check user/pass (as it is now) or the other way around.

Also noticed it doesn't work with spaces before/after comma's so we might want to add a .replaceAll("\\s","") or similar.

ustcweizhou · 2017-04-15T14:31:32Z

@wido very nice feature.
It would also be nice if this can be added to domain or account setting.

wido · 2017-04-17T08:48:32Z

@remibergsma: Yes, I am aware of that UI problem. Not sure how to fix it.

After thinking about it, I went for '403 Forbidden' and also stripped whitespace from the config key.

I think the order is OK right now unless other opinions?

@ustcweizhou: That is a lot more difficult then a global value, isn't it? Since you have to query it every time.

Or should configkey allow this very easily?

The CIDR list is now queried on bootstrap of ApiServlet as I didn't want to hammer the DB for every API call.

remibergsma · 2017-04-17T09:08:18Z

@wido I think we need to do the check on two places, also on the login() method. That makes sure we don't issue a session key when user/pass are OK but we still reject it based on the CIDR. In my testing that also fixes the UI issue. There are two ways to authenticate so that makes sense I'd say. It'll then also work with authentication plugins, such as LDAP/AD.

Switching the scope of the config is easy, but indeed you'll be querying it on every API call. That does have the benefit you don't need to restart the mgt server when you make a change, but the downside is also obvious. One way to resolve it, is to make a global config setting that switches the feature on/off (and that config is loaded at bootstrap) so you can opt-in for the more heavy checks.

I'll play a bit more with it tonight.

Update:
Implemented the above, will continue testing it in the coming days. Works great, and hitting the DB also seems fine. Loads of queries are done already, so one per API call extra seems to make no noticeable difference. But let's see how it behaves. At least good to keep an eye on.
FYI my code is here: MissionCriticalCloud/cosmic#350

wido · 2017-04-18T12:17:23Z

@remibergsma: I pulled your code, thanks! It now works per account @ustcweizhou

How does this look?

remibergsma · 2017-04-18T18:59:35Z

        UserAccount user = null;
        if (password != null && !password.isEmpty()) {
            user = getUserAccount(username, password, domainId, requestParameters);
-        } else {


@wido You may want to put back this else statement. In Cosmic sso has been removed, and when I came across this left-over I removed it. However, I think it shouldn't removed from CloudStack to not break sso.

wido · 2017-04-18T20:52:42Z

Good one @remibergsma. I reverted that piece and also the baremetal refusal of users.

blueorangutan · 2017-12-27T07:50:32Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-12-27T08:27:25Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1505

yadvr · 2017-12-28T09:19:50Z

@wido can you resolve the conflicts, thanks.

wido · 2017-12-28T09:32:04Z

I will in a few days.

Can we merge this one afterwards? I keep resolving conflicts which happen since other code is merged ;)

yadvr · 2017-12-28T10:14:50Z

Sure, ping me @wido

yadvr · 2018-01-02T13:05:32Z

Ping @wido - can you fix the conflicts?

This class had many unused methods, inconsistent names and redundant code. This commit cleans up code, renames a few methods and constants. Methods were renamed to clearly show that they are for IPv4 or IPv6. Tests were improved and added to test the changes that were made to the code. Signed-off-by: Wido den Hollander <wido@widodh.nl>

The global/account setting 'api.allowed.source.cidr.list' is set to 0.0.0.0/0,::/0 by default preserve the current behavior and thus allow API calls for accounts from all IPv4 and IPv6 subnets. Users can set it to a comma-separated list of IPv4/IPv6 subnets to restrict API calls for Admin accounts to certain parts of their network(s). This is to improve Security. Should a attacker steal the Access/Secret key of a account he/she still needs to be in a subnet from where accounts are allowed to perform API calls. This is a good security measure for APIs which are connected to the public internet. Signed-off-by: Wido den Hollander <wido@widodh.nl>

wido · 2018-01-02T17:14:56Z

Done!

I'll be back at work next week

yadvr · 2018-01-02T17:15:25Z

@blueorangutan package

blueorangutan · 2018-01-02T17:15:32Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-01-02T17:41:53Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1548

yadvr · 2018-01-02T17:46:54Z

@blueorangutan test

blueorangutan · 2018-01-02T17:47:32Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2018-01-03T10:00:51Z

 Description=CloudStack Agent
 Documentation=http://www.cloudstack.org/
-Requires=libvirtd.service
-After=libvirtd.service


@wido this needs to be reverted, as the renaming of libvirtd to libvirt-bin happens via the debian packging scripts. This will cause regressions for KVM. If found KVM hosts failing to start.

yadvr · 2018-01-03T10:02:41Z

@blueorangutan package

blueorangutan · 2018-01-03T10:03:32Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-01-03T10:39:26Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1554

yadvr · 2018-01-03T10:40:26Z

@blueorangutan test

blueorangutan · 2018-01-03T10:40:32Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2018-01-03T19:12:28Z

Trillian test result (tid-1990)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30710 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2046-t1990-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_ssvm.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 66 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	376.18	test_vpc_redundant.py

yadvr · 2018-01-04T05:26:01Z

Merging this based on 2 code reviews lgtm and test results. Checked and found the one failing smoketest to be env related.

ravening · 2021-12-22T11:32:13Z

@wido sorry to bring up the old pr but how can I configure under this account level?
I logged as in a regular user with "Domain admin" role but i dont see any settings tab under the account

weizhouapache · 2021-12-22T11:47:29Z

@wido sorry to bring up the old pr but how can I configure under this account level? I logged as in a regular user with "Domain admin" role but i dont see any settings tab under the account

@ravening please have a look at #4339

….20.0.0-scclouds' Impedir o _download_ de _templates_ para _storages_ secundários que estão em `read-only` Closes #2046 See merge request scclouds/scclouds!1297

wido force-pushed the admin-cidr branch 2 times, most recently from 001eb9b to 4807373 Compare April 14, 2017 12:12

DaanHoogland approved these changes Apr 14, 2017

View reviewed changes

DaanHoogland reviewed Apr 14, 2017

View reviewed changes

wido force-pushed the admin-cidr branch 2 times, most recently from 042e0d4 to 27b2f40 Compare April 14, 2017 15:47

wido self-assigned this Apr 14, 2017

wido added type:enhancement component:api component:management-server type:security labels Apr 14, 2017

remibergsma reviewed Apr 14, 2017

View reviewed changes

wido force-pushed the admin-cidr branch from 27b2f40 to 5ee46f2 Compare April 15, 2017 10:32

wido force-pushed the admin-cidr branch from 5ee46f2 to 7672c9c Compare April 17, 2017 08:47

wido force-pushed the admin-cidr branch from 7672c9c to 2503c06 Compare April 18, 2017 12:16

remibergsma reviewed Apr 18, 2017

View reviewed changes

wido force-pushed the admin-cidr branch from 2503c06 to 45f5c08 Compare April 18, 2017 20:52

wido force-pushed the admin-cidr branch from 45f5c08 to 88ca0e4 Compare May 4, 2017 11:38

wido added this to the 4.11 milestone Jun 2, 2017

wido added the status:ready-for-merge label Jul 13, 2017

wido added 2 commits January 2, 2018 18:05

wido force-pushed the admin-cidr branch from 9f22f10 to 374ee59 Compare January 2, 2018 17:14

yadvr reviewed Jan 3, 2018

View reviewed changes

This reverts the bin/app name to libvirtd

e46b4d7

yadvr approved these changes Jan 4, 2018

View reviewed changes

yadvr merged commit 9988c26 into apache:master Jan 4, 2018

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

Uh oh!

Conversation

wido commented Apr 14, 2017

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wido commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wido commented Apr 14, 2017

Uh oh!

remibergsma commented Apr 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wido commented Apr 15, 2017

Uh oh!

remibergsma commented Apr 15, 2017

Uh oh!

ustcweizhou commented Apr 15, 2017

Uh oh!

wido commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remibergsma commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wido commented Apr 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wido commented Apr 18, 2017

Uh oh!

blueorangutan commented Dec 27, 2017

Uh oh!

blueorangutan commented Dec 27, 2017

Uh oh!

yadvr commented Dec 28, 2017

Uh oh!

wido commented Dec 28, 2017

Uh oh!

yadvr commented Dec 28, 2017

Uh oh!

yadvr commented Jan 2, 2018

Uh oh!

wido commented Jan 2, 2018

Uh oh!

yadvr commented Jan 2, 2018

Uh oh!

blueorangutan commented Jan 2, 2018

Uh oh!

blueorangutan commented Jan 2, 2018

Uh oh!

yadvr commented Jan 2, 2018

Uh oh!

blueorangutan commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

wido commented Apr 14, 2017 •

edited

Loading

wido commented Apr 17, 2017 •

edited

Loading

remibergsma commented Apr 17, 2017 •

edited

Loading

yadvr commented Jan 4, 2018 •

edited

Loading