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

Change all references from whitelist to allowlist #1054

Merged
merged 1 commit into from Aug 4, 2020

Conversation

rhatdan
Copy link
Contributor

@rhatdan rhatdan commented Jul 7, 2020

We want to move to more enclusive names/terms in our code, and remove problematic language
from code and comments.

We want to change reference for whitelist/blacklist to allowlist/denylist.

https://www.redhat.com/en/blog/making-open-source-more-inclusive-eradicating-problematic-language

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM, should we also take care of slave (except when it is an option for mount?

@@ -214,9 +214,9 @@ Runtimes MAY attach the container process to additional cgroup controllers beyon
}
```

### <a name="configLinuxDeviceWhitelist" />Device whitelist
Copy link
Member

Choose a reason for hiding this comment

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

Since "whitelist" implies a deny-by-default configuration (with "blacklist" implying the reverse), we should add some text to clarify that this is the case. At least to my ear, "allowlist" doesn't have that implication (at least not as apparently as "whitelist").

Copy link
Member

Choose a reason for hiding this comment

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

block/unblock?

Copy link
Member

Choose a reason for hiding this comment

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

I should've better clarified my point here -- the title "device allowlist" is totally fine (and I prefer it to "allowed device list"). What I was suggesting is to add a single sentence which says something like:

The allow-list MUST have a default deny-all policy, meaning that if it is omitted no device access is permitted.

This would not be a change in behaviour (since that is how all implementations have worked and is technically already required by the term "whitelist") but instead just a clarification.

@cyphar
Copy link
Member

cyphar commented Jul 11, 2020

@giuseppe Indeed. In particular, the text on pseudoterminals should also be revised to use "ptmx" and "pty" or something like that. There was a twitter thread about this a few days ago.

@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 11, 2020

Well allowlist and denylist was the name referenced in blog above, which is why I went with it. I am fine with picking some other name or just adding clarification data.

@rhatdan rhatdan force-pushed the allowlist branch 2 times, most recently from 9b17daf to a995de8 Compare July 11, 2020 12:51
@giuseppe
Copy link
Member

@giuseppe Indeed. In particular, the text on pseudoterminals should also be revised to use "ptmx" and "pty" or something like that. There was a twitter thread about this a few days ago.

I've performed the same change in crun after seeing that discussion :-) my first thought was for using pty/tty as I was also wrongly assuming that -mx stands for master. Since it is not the case, I think ptmx/pty is much clearer unless the kernel is going to use a different naming.

@giuseppe
Copy link
Member

Well allowlist and denylist was the name referenced in blog above, which is why I went with it. I am fine with picking some other name or just adding clarification data.

thanks. LGTM

@rhatdan
Copy link
Contributor Author

rhatdan commented Jul 13, 2020

@cyphar @thaJeztah PTAL

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

found one broken anchor, but lgtm otherwise

config-linux.md Outdated Show resolved Hide resolved
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM (not a maintainer)

@@ -354,7 +354,8 @@ type LinuxRdma struct {

// LinuxResources has container runtime resource constraints
type LinuxResources struct {
// Devices configures the device whitelist.
// Devices configures the only devices allowed to be used within
// the container.
Copy link
Member

Choose a reason for hiding this comment

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

s/whitelist/allowlist/ reads better IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched back.

We want to move to more enclusive names/terms in our code, and remove problematic language
from code and comments.

We want to change reference for whitelist/blacklist to allowlist/denylist.

https://www.redhat.com/en/blog/making-open-source-more-inclusive-eradicating-problematic-language

We also want to fix slave references to pty for pseutoterminals.

We will change the slave refererences to whatever the kernel specifies, once the kernel fixes it's
references.

Co-authored-by: Sebastiaan van Stijn <thaJeztah@users.noreply.github.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Contributor Author

rhatdan commented Aug 4, 2020

@hqhq
Copy link
Contributor

hqhq commented Aug 4, 2020

LGTM

Approved with PullApprove

1 similar comment
@vbatts
Copy link
Member

vbatts commented Aug 4, 2020

LGTM

Approved with PullApprove

@vbatts vbatts merged commit 278a7d7 into opencontainers:master Aug 4, 2020
@AkihiroSuda AkihiroSuda mentioned this pull request Jan 24, 2023
@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone Feb 1, 2023
@AkihiroSuda AkihiroSuda mentioned this pull request Jun 26, 2023
12 tasks
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.

None yet

7 participants