Filter everything when whitelist is empty #781

rthallisey · 2018-02-21T17:13:40Z

Describe what this PR does and why we need it:
We currently filter nothing when blacklist is empty and whitelist is empty.

Another way of saying this is if I have this whitelist config:

    white_list:
      - ".*-apb$"

In my registry I have nothing that ends in *-apb. The result is that everything goes unfiltered.

[2018-02-21T12:12:20.868-05:00] [DEBUG] - APBs passing white/blacklist filter:
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/centos/python-36-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/centos/postgresql-95-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/openshift/mongodb-24-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/openshift/jenkins-2-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/centos/ruby-23-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/openshift/ruby-20-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/centos/nodejs-6-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/centos/nginx-110-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/centos/perl-520-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/openshift/wildfly-100-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/centos/mariadb-102-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/openshift/nodejs-010-centos7
[2018-02-21T12:12:20.868-05:00] [DEBUG] - -> docker.io/centos/python-34-centos7
...

Changes proposed in this pull request

Filter everything when nothing is whitelisted

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes: #745
fixes: ansibleplaybookbundle/ansible-playbook-bundle#215

shawn-hurley · 2018-02-21T18:50:33Z

As long as we are okay with the log message:
[2018-02-21T18:48:12.717Z] [INFO] - APBs filtered by white/blacklist filter:-> docker.io/openshift/wildfly-100-centos7-> docker.io/centos/mysql-57-centos7-> docker.io/centos/nginx-18-centos7-> docker.io/centos/php-56-centos7-> docker.io/centos/python-35-centos7-> docker.io/centos/ruby-24-centos7-> docker.io/openshift/php-55-centos7-> docker.io/openshift/mongodb-24-centos7-> docker.io/centos/postgresql-94-centos7-> docker.io/openshift/wildfly-101-centos7-> docker.io/openshift/wildfly-81-centos7-> docker.io/centos/ruby-23-centos7-> docker.io/centos/php-71-centos7-> docker.io/centos/postgresql-95-centos7-> docker.io/openshift/ruby-20-centos7-> docker.io/openshift/wildfly-90-centos7-> docker.io/openshift/python-33-centos7-> docker.io/openshift/mysql-55-centos7-> docker.io/openshift/jenkins-1-centos7-> docker.io/centos/mongodb-26-centos7-> docker.io/centos/nginx-112-centos7-> registry.centos.org/dotnet/dotnet-20-runtime-centos7-> docker.io/centos/mongodb-34-centos7-> docker.io/centos/nodejs-8-centos7-> docker.io/centos/mariadb-101-centos7-> docker.io/centos/redis-32-centos7-> docker.io/openshift/nodejs-010-centos7-> registry.centos.org/dotnet/dotnet-20-centos7-> docker.io/centos/python-34-centos7-> docker.io/centos/perl-524-centos7-> docker.io/centos/nodejs-6-centos7-> docker.io/centos/ruby-22-centos7-> docker.io/centos/mongodb-32-centos7-> docker.io/centos/nginx-110-centos7-> docker.io/centos/postgresql-96-centos7-> docker.io/openshift/postgresql-92-centos7-> docker.io/centos/php-70-centos7-> docker.io/centos/mariadb-102-centos7-> docker.io/openshift/jenkins-2-centos7-> docker.io/centos/mysql-56-centos7-> docker.io/centos/httpd-24-centos7-> docker.io/centos/python-36-centos7-> docker.io/openshift/perl-516-centos7-> docker.io/centos/python-27-centos7-> docker.io/centos/nodejs-4-centos7-> docker.io/centos/perl-520-centos7

then I think this is good

eriknelson · 2018-02-21T19:09:04Z

@shawn-hurley is that log actually all one line? Because it's totally unreadable in that form. I do think we should continue to Info log the apbs that were filtered, I just want it to be actually useful.

eriknelson · 2018-02-21T19:11:49Z

pkg/registries/filter.go

@@ -192,7 +192,11 @@ func applyMatchSets(
 	filteredVals := []string{}
 	totalSet := toMatchSetT(totalList)

-	if len(whiteMatchSet) != 0 && len(blackMatchSet) != 0 {
+	if len(whiteMatchSet) == 0 {


I am concerned with this change. There is an intended use-case for specifying only a blacklist. You are saying you want to accept everything except those that don't pass my blacklist filter.

What I could do is default the white_list to ".*" if only black_list exists.

jmrodri · 2018-02-21T19:55:36Z

My understanding was that we filtered NOTHING, and that you can use the white and black lists to filter things as you choose.

my registry contents

mediawiki-apb
postgresql-apb
mysql
mariadb
shitty-apb
shitty-cool-apb

=========================

whitelist EMPTY
blacklist EMPTY

mediawiki-apb
postgresql-apb
mysql
mariadb
shitty-apb
shitty-cool-apb

=========================

whitelist EMPTY
blacklist shitty*apb

mediawiki-apb
postgresql-apb
mysql
mariadb

=========================

whitelist -apb
blacklist shitty*apb

mediawiki-apb
postgresql-apb

=========================

whitelist -apb
blacklist EMPTY

mediawiki-apb
postgresql-apb
shitty-apb
shitty-cool-apb

=========================

whitelist EMPTY
blacklist *apb, mysql, mariadb

EVERYTHING FILTERED

jmrodri

NACK I don't want to do this. We should not require you to enter a whitelist entry.

shawn-hurley · 2018-02-21T19:58:20Z

whitelist EMPTY
blacklist EMPTY

mediawiki-apb
postgresql-apb
mysql
mariadb
shitty-apb
shitty-cool-apb

This is not the case. We default to closed and no apb's show up. apbs are opt in. This was an ask of the security team.

rthallisey · 2018-02-21T19:58:29Z

@jmrodri My view was the opposite. I thought for security purposes we wanted to allow nothing.

eriknelson · 2018-02-21T19:58:39Z

whitelist EMPTY
blacklist shitty*apb

    mediawiki-apb
    postgresql-apb
    mysql
    mariadb

This is the case I'm concerned with. It's a legitimate use-case to want to blacklist X, but accept everything else. We should be smart enough to ignore mysql and mariadb since they are not APBs. We need to be able to detect whether or not an image is an APB beyond its name.

shawn-hurley · 2018-02-21T20:01:54Z

@eriknelson if we saying APBs are opt in, which was the ask, then I think it follows that you can't just add a blacklist and that defaults to letting everything in. I could be wrong on this one but I do think this is the case.

Also re: the log yes that is all one line for me.

rthallisey · 2018-02-21T20:03:30Z

This is the case I'm concerned with. It's a legitimate use-case to want to blacklist X, but accept everything else.

Not ideal to add an extra config option, but this would do the trick.

    black_list:
      - ".*-apb$"
    white_list:
      - ".*"

We should be smart enough to ignore mysql and mariadb since they are not APBs. We need to be able to detect whether or not an image is an APB beyond its name.

We check for the APB label, but that is outside the whitelist/blacklist filter

eriknelson · 2018-02-21T20:07:23Z

APBs are opt in, which was the ask

There was no original ask or requirement. I personally implemented the filters in the way that I felt appropriate. If you only specify a blacklist, I feel that you are expressing you want those filtered, but everything else allowed, in the absence of a whitelist. I still feel that should be the case, but I'm open to arguments to the contrary. Ultimately, this is probably a PM decision.

jmrodri · 2018-02-21T20:12:33Z

@jmrodri My view was the opposite. I thought for security purposes we wanted to allow nothing.

So is there currently a security risk that we need to fix? I didn't think there was, I believe this all stems from the removal of the hard-coded -apb we had in the code. Prior to that things seemed be okay until someone wanted to add an apb not named -apb.

I still think that leaving them empty shows everything. From an ease of use and configuration. I only want to learn enough regex to lock things down, not to open everything up.

If I want to extra paranoid in my corporate deployment, I can come up with a naming strategy that I approve. Then whitelist those. And blacklist everything else. If I'm in a development shop, I just leave them empty and see everything.

If the user wants to block everything for some reason, we have given them a mechanism to do that.

eriknelson · 2018-02-21T20:17:51Z

~~Going to send a mailing list question on the issue.~~ We're holding a bluejeans to discuss.

shawn-hurley · 2018-02-21T20:18:17Z

@eriknelson I think that is correct that you initially implemented in the way described.

When we were discussing security changes with security team, they asked that we make the APBs default to empty if no whitelist and blacklist.

Here is the PR for that change #411

enj · 2018-02-21T23:30:01Z

So, if you guys are willing to change the current behavior (and possibly break some users), I would go with the Opt-in Filter Behavior as defined in #785. If you believe it will cause too much issue for users, then we may simply to have live with a fail open default config option.

With the Opt-in Filter Behavior, a user can always recreate the current behavior by setting whitelist to .* and blacklist to whatever they currently have. The choice is just extremely explicit. I do not know how the ASB handles config validation, but if you followed OpenShift's logic, the broker would fail to start if only blacklist or None was set (unless there is some valid reason to have no allowed APBs).

Using .* as the whitelist may be a very reasonable option in situations where you have full trust in the source of the APB (i.e Red Hat). But as others start to create APBs, we should assume they are dangerous since they are effectively black boxes (and it is unreasonable to expect users to look through the source code to make sure the APB is safe).

rthallisey · 2018-02-22T14:33:06Z

Given the feedback on the issue, we going with opt-in. This PR will enable opt-in so I'll change the docs to reflect this behavior.

shawn-hurley

I'll approve once the doc comments are updated to match the opt-in workflow

jmrodri · 2018-02-24T04:46:56Z

@eriknelson @shawn-hurley docs were updated, please re-review.

rthallisey · 2018-02-26T14:25:55Z

@shawn-hurley @jmrodri @eriknelson Do you think we should backport this?

backport of openshift#781

backport of #781

* Filter everything when whitelist is empty * update docs

Filter everything when whitelist is empty

0a73a19

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 21, 2018

shawn-hurley approved these changes Feb 21, 2018

View reviewed changes

eriknelson suggested changes Feb 21, 2018

View reviewed changes

jmrodri suggested changes Feb 21, 2018

View reviewed changes

eriknelson mentioned this pull request Feb 21, 2018

Define the default filtering policy #785

Closed

shawn-hurley requested changes Feb 22, 2018

View reviewed changes

update docs

2d7fe67

jmrodri approved these changes Feb 24, 2018

View reviewed changes

shawn-hurley approved these changes Feb 26, 2018

View reviewed changes

rthallisey merged commit fc9644f into openshift:master Feb 26, 2018

rthallisey deleted the empty-whitelist branch February 26, 2018 14:24

rthallisey pushed a commit to rthallisey/ansible-service-broker that referenced this pull request Feb 26, 2018

Filter everything when whitelist is empty

6b6e7a4

backport of openshift#781

rthallisey mentioned this pull request Feb 26, 2018

Bug 1549189 - Filter everything when whitelist is empty #793

Merged

rthallisey pushed a commit that referenced this pull request Feb 26, 2018

Filter everything when whitelist is empty (#793)

883dfe9

backport of #781

rthallisey mentioned this pull request Mar 13, 2018

Give more respect to the whitelist param #685

Closed

jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018

Filter everything when whitelist is empty (openshift#781)

f2a148b

* Filter everything when whitelist is empty * update docs

Filter everything when whitelist is empty #781

Filter everything when whitelist is empty #781

rthallisey commented Feb 21, 2018 •

edited

shawn-hurley commented Feb 21, 2018

eriknelson commented Feb 21, 2018

eriknelson Feb 21, 2018 •

edited

rthallisey Feb 21, 2018

jmrodri commented Feb 21, 2018

jmrodri left a comment

shawn-hurley commented Feb 21, 2018

rthallisey commented Feb 21, 2018

eriknelson commented Feb 21, 2018

shawn-hurley commented Feb 21, 2018

rthallisey commented Feb 21, 2018 •

edited

eriknelson commented Feb 21, 2018 •

edited

jmrodri commented Feb 21, 2018

eriknelson commented Feb 21, 2018 •

edited

shawn-hurley commented Feb 21, 2018

enj commented Feb 21, 2018

rthallisey commented Feb 22, 2018

shawn-hurley left a comment •

edited

jmrodri commented Feb 24, 2018

rthallisey commented Feb 26, 2018

Filter everything when whitelist is empty #781

Filter everything when whitelist is empty #781

Conversation

rthallisey commented Feb 21, 2018 • edited

shawn-hurley commented Feb 21, 2018

eriknelson commented Feb 21, 2018

eriknelson Feb 21, 2018 • edited

Choose a reason for hiding this comment

rthallisey Feb 21, 2018

Choose a reason for hiding this comment

jmrodri commented Feb 21, 2018

jmrodri left a comment

Choose a reason for hiding this comment

shawn-hurley commented Feb 21, 2018

rthallisey commented Feb 21, 2018

eriknelson commented Feb 21, 2018

shawn-hurley commented Feb 21, 2018

rthallisey commented Feb 21, 2018 • edited

eriknelson commented Feb 21, 2018 • edited

jmrodri commented Feb 21, 2018

eriknelson commented Feb 21, 2018 • edited

shawn-hurley commented Feb 21, 2018

enj commented Feb 21, 2018

rthallisey commented Feb 22, 2018

shawn-hurley left a comment • edited

Choose a reason for hiding this comment

jmrodri commented Feb 24, 2018

rthallisey commented Feb 26, 2018

rthallisey commented Feb 21, 2018 •

edited

eriknelson Feb 21, 2018 •

edited

rthallisey commented Feb 21, 2018 •

edited

eriknelson commented Feb 21, 2018 •

edited

eriknelson commented Feb 21, 2018 •

edited

shawn-hurley left a comment •

edited