You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wido <gi...@git.apache.org> on 2017/04/14 11:28:03 UTC
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
GitHub user wido opened a pull request:
https://github.com/apache/cloudstack/pull/2046
CLOUDSTACK-7958: Add configuration for limit to CIDRs for Admin API calls
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wido/cloudstack admin-cidr
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/2046.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2046
----
commit 770b0bdd4f20deefcb11d9c7b7713e06e3281e8e
Author: Wido den Hollander <wi...@widodh.nl>
Date: 2017-04-13T15:23:24Z
Cleanup and Improve NetUtils
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 <wi...@widodh.nl>
commit fb6ab51ba384cb2ae3eed788f3c241b4c28c9bf5
Author: Wido den Hollander <wi...@widodh.nl>
Date: 2017-04-13T15:23:36Z
CLOUDSTACK-7958: Add configuration for limit to CIDRs
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.
Signed-off-by: Wido den Hollander <wi...@widodh.nl>
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #2046: CLOUDSTACK-7958: Add configuration for limit to CIDR...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/2046
@DaanHoogland: I improved the logging as you suggested/requested.
A TRACE for every request and WARN when a request is denied. Tried this locally:
<pre>
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
<pre>
In this case only localhost (IPv4/IPv6) is allowed to perform requests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111568831
--- Diff: server/src/com/cloud/api/ApiServlet.java ---
@@ -290,6 +292,17 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
CallContext.register(accountMgr.getSystemUser(), accountMgr.getSystemAccount());
}
+ if (CallContext.current().getCallingAccount().getType() == Account.ACCOUNT_TYPE_ADMIN) {
+ s_logger.debug("CIDRs from which Admin accounts are allowed to perform API calls " + adminCidrs);
--- End diff --
you don't want to log this on every call.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111568225
--- Diff: core/src/com/cloud/network/HAProxyConfigurator.java ---
@@ -538,7 +536,7 @@ private String getLbSubRuleForStickiness(final LoadBalancerTO lbTO) {
if (stickinessSubRule != null && !destsAvailable) {
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) {
--- End diff --
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)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111591078
--- Diff: server/src/com/cloud/api/ApiServlet.java ---
@@ -290,6 +292,17 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
CallContext.register(accountMgr.getSystemUser(), accountMgr.getSystemAccount());
}
+ if (CallContext.current().getCallingAccount().getType() == Account.ACCOUNT_TYPE_ADMIN) {
+ s_logger.debug("CIDRs from which Admin accounts are allowed to perform API calls " + adminCidrs);
--- End diff --
I was thinking trace here and debug or info on the first load of the cidr
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111589706
--- Diff: server/src/com/cloud/api/ApiServlet.java ---
@@ -290,6 +292,17 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
CallContext.register(accountMgr.getSystemUser(), accountMgr.getSystemAccount());
}
+ if (CallContext.current().getCallingAccount().getType() == Account.ACCOUNT_TYPE_ADMIN) {
+ s_logger.debug("CIDRs from which Admin accounts are allowed to perform API calls " + adminCidrs);
+ if (!NetUtils.isIpInCidrList(InetAddress.getByName(remoteAddress), adminCidrs.split(","))) {
+ auditTrailSb.append(" " + HttpServletResponse.SC_UNAUTHORIZED + " " + "IP-Address of remote not in configured Admin CIDR list");
+ 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());
--- End diff --
True, true. I would say WARN?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #2046: CLOUDSTACK-7958: Add configuration for limit to CIDR...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/2046
@PaulAngus This is what we talked about in Prague. Mind taking a look?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111589627
--- Diff: api/src/org/apache/cloudstack/config/ApiServiceConfiguration.java ---
@@ -28,6 +28,8 @@
"API end point. Can be used by CS components/services deployed remotely, for sending CS API requests", true);
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);
--- End diff --
I agree. I have set it to open for all for now. We can submit a different PR afterwards to change the default imho.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111589567
--- Diff: core/src/com/cloud/network/HAProxyConfigurator.java ---
@@ -538,7 +536,7 @@ private String getLbSubRuleForStickiness(final LoadBalancerTO lbTO) {
if (stickinessSubRule != null && !destsAvailable) {
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) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111589656
--- Diff: server/src/com/cloud/api/ApiServlet.java ---
@@ -290,6 +292,17 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
CallContext.register(accountMgr.getSystemUser(), accountMgr.getSystemAccount());
}
+ if (CallContext.current().getCallingAccount().getType() == Account.ACCOUNT_TYPE_ADMIN) {
+ s_logger.debug("CIDRs from which Admin accounts are allowed to perform API calls " + adminCidrs);
--- End diff --
You suggest setting this to TRACE instead of debug?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111568764
--- Diff: api/src/org/apache/cloudstack/config/ApiServiceConfiguration.java ---
@@ -28,6 +28,8 @@
"API end point. Can be used by CS components/services deployed remotely, for sending CS API requests", true);
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);
--- End diff --
argument for this default: backwards compatible
argument against: inherent security risk
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #2046: CLOUDSTACK-7958: Add configuration for limit ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/2046#discussion_r111568888
--- Diff: server/src/com/cloud/api/ApiServlet.java ---
@@ -290,6 +292,17 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
CallContext.register(accountMgr.getSystemUser(), accountMgr.getSystemAccount());
}
+ if (CallContext.current().getCallingAccount().getType() == Account.ACCOUNT_TYPE_ADMIN) {
+ s_logger.debug("CIDRs from which Admin accounts are allowed to perform API calls " + adminCidrs);
+ if (!NetUtils.isIpInCidrList(InetAddress.getByName(remoteAddress), adminCidrs.split(","))) {
+ auditTrailSb.append(" " + HttpServletResponse.SC_UNAUTHORIZED + " " + "IP-Address of remote not in configured Admin CIDR list");
+ 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());
--- End diff --
this you do want to log on every attempt (WARN or INFO???)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---