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

Better words #12089

Closed
wants to merge 1 commit into from
Closed

Better words #12089

wants to merge 1 commit into from

Conversation

richsalz
Copy link
Contributor

@richsalz richsalz commented Jun 8, 2020

Remove some offensive/archaic terminology from OpenSSL.

RAND_DRBG_TYPE, /* Public */
RAND_DRBG_TYPE /* Private */
};
static unsigned int rand_drbg_flags[3] = {
RAND_DRBG_FLAGS | RAND_DRBG_FLAG_MASTER, /* Master */
RAND_DRBG_FLAGS | RAND_DRBG_FLAG_PARENT, /* Main */
Copy link
Member

Choose a reason for hiding this comment

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

Main is a better idea, would you mind using "main" instead of "parent"? My thinking is that "main" would indicate that it's the top or root DRBG, while a "parent" can be somewhere in the middle of the chain, so not quite as definitive if you see what I mean...

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't "root" be even better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either root or parent is fine to replace master; you guys decide.

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually a little dubious that "master" on its own is offensive, whereas it is considered as such when put together with "slave" (for obvious reasons)...

However, I had a quick look at https://en.wikipedia.org/wiki/Master/slave_(technology), and other proposed substitutes seem to be "main" and "primary", primarily. I see that as an argument for choosing "main" in our case.

Copy link
Member

Choose a reason for hiding this comment

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

Main implies it is somewhat the one that is mainly used. Root or primary please.

Choose a reason for hiding this comment

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

As mentioned in a separate comment, "master/minion" is also completely fine terminology that preserves the intention that "this thing is controlling the other thing".

Copy link
Contributor

Choose a reason for hiding this comment

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

Master is in the public APIs already. We'd have to change it via the deprecation policy.

I'm open to others names: primal, initial, basal, chief, primary, dominus, foremost, principle, prime, doyen, best, first, suzerain, seed, ...

The entire RAND framework needs some polishing @mspncp and I have some convergent ideas as to the direction we think it should go. Nonetheless, RAND_bytes() and RAND_priv_bytes() must remain and be the primary calls -- I suspect the rest is rarely used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for the deprecation policy, I believe these lines are enough:

diff --git a/include/openssl/rand_drbg.h b/include/openssl/rand_drbg.h
index 5ee93a3eab..382d472414 100644
--- a/include/openssl/rand_drbg.h
+++ b/include/openssl/rand_drbg.h
@@ -112,6 +112,10 @@ int RAND_DRBG_set_reseed_defaults(
                                   time_t child_reseed_time_interval
                                   );
 
+# ifndef OPENSSL_NO_DEPRECATED_3_0
+#  define OPENSSL_CTX_get0_master_drbg(ctx) OPENSSL_CTX_get0_parent_drbg(ctx)
+#  define RAND_DRBG_get0_master() RAND_DRBG_get0_parent()
+# endif

If you want a HISTORY entry in doc/man3/RAND_DRBG_get0_parent.pod (point: new name) I can do that.

HOWEVER first the project has to decide it wants this. As I've emailed, https://mta.openssl.org/pipermail/openssl-project/2020-June/002040.html, there's a thread started.

Copy link
Member

Choose a reason for hiding this comment

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

On a technical note, the OPENSSL_CTX_get0_master_drbg() compat define is unnecessary because that function does not exist in 1.1.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks.

RAND_DRBG *OPENSSL_CTX_get0_public_drbg(OPENSSL_CTX *ctx);
RAND_DRBG *OPENSSL_CTX_get0_private_drbg(OPENSSL_CTX *ctx);
RAND_DRBG *RAND_DRBG_get0_master(void);
RAND_DRBG *RAND_DRBG_get0_parent(void);
Copy link
Member

Choose a reason for hiding this comment

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

This one is troublesome, as it exists in 1.1.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple enough to keep the old name in addition, or deprecate it; let me know what you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

You'll have to keep the old name in addition no matter what. If it were me, I would deprecate it, but from what I gather, that may be a bit too rad, so just add a backward compat macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a macro in the header file once the hold is removed. I hope that will happen soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the function part of the stable ABI? If yes then a macro is not sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the function part of the stable ABI? If yes then a macro is not sufficient.

Sorry, I missed Rich's reply #12089 (comment)

@t-j-h
Copy link
Member

t-j-h commented Jun 8, 2020

-1

@t-j-h t-j-h added the hold: need omc decision The OMC needs to make a decision label Jun 8, 2020
@kaduk
Copy link
Contributor

kaduk commented Jun 8, 2020

-1

Hi Tim, a flat "-1" feels like applying stop energy without any guidance on how to make progress. I trust that you will compose a more detailed response when you have a chance to do so.

@beldmit
Copy link
Member

beldmit commented Jun 9, 2020

The most fixes like "white space => whitespace" should be applied anyway as they are grammar improvements...

@t8m
Copy link
Member

t8m commented Jun 10, 2020

Rich, could you please split the PR in two? One fixing the non-public API parts and avoiding changes for the 'master' word. That one should be uncontroversial. And keep the public API changes here for OMC to decide.

@t8m
Copy link
Member

t8m commented Jun 10, 2020

The reason I am asking to avoid all the changes to the master word is that replacing it with parent is in my opinion wrong in most cases in this PR. It should be replaced with primary or root or main.

@tmshort
Copy link
Contributor

tmshort commented Jun 10, 2020

I thought about the term "blanks" for "whitespace", but I'm not sure it quite matches.

@richsalz
Copy link
Contributor Author

No, I will not split the PR. As I said before, I am happy to use whatever word other than parent once you folks decide.

@mlindner
Copy link

mlindner commented Jun 10, 2020

This is quite absurd. Blacklist is standard engineering terminology. Please read up on the etymology of the word before inserting your own misguided ideas into things. https://www.etymonline.com/word/blacklist

@mlindner
Copy link

More so this is API breaking for no purpose other than suiting some people's social ideas. This shouldn't be relevant in engineering discussion. Master/slave are also hardcoded terms in many engineering specifications, especially in device drivers.

@mlindner
Copy link

The most fixes like "white space => whitespace" should be applied anyway as they are grammar improvements...

Yes I agree. Those are typos and "whitespace" is a single word. The master/slave wording is unfortunate but is standard terminology and there is no reason to change it. The removal of "blacklist" is pure nonsense based on misunderstanding.

@slontis
Copy link
Member

slontis commented Jun 10, 2020

This is quite absurd. Blacklist is standard engineering terminology. Please read up on the etymology of the word before inserting your own misguided ideas into things. https://www.etymonline.com/word/blacklist

Please do some extra reading.. e.g: https://tools.ietf.org/id/draft-knodel-terminology-01.html
Maybe it will change your mind.

@mlindner
Copy link

mlindner commented Jun 10, 2020

This is quite absurd. Blacklist is standard engineering terminology. Please read up on the etymology of the word before inserting your own misguided ideas into things. https://www.etymonline.com/word/blacklist

Please do some extra reading.. e.g: https://tools.ietf.org/id/draft-knodel-terminology-01.html
Maybe it will change your mind.

One mistaken understanding broadly spread does not suddenly make it correct. "Black and white" ascribe values to colors, it has nothing to do with the color of skin. The terminology predates westerners ever encountering people from subsaharan Africa.

@mlindner
Copy link

Even master/slave, while historically unfortunate is pretty standard. See: https://en.wikipedia.org/wiki/I%C2%B2C We could change to "master/minion" while maintaining the same idea of one piece of code controlling the other. "parent/child" does not work as the parent doesn't control the child, they just create the child.

@tiran
Copy link
Contributor

tiran commented Jun 10, 2020

One mistaken understanding broadly spread does not suddenly make it correct. "Black and white" ascribe values to colors, it has nothing to do with the color of skin. The terminology predates westerners ever encountering people from subsaharan Africa.

Who is talking about skin color? @richsalz is changing words to do me a favor!

I like to wear black cloths and listen to black music (Metal, Goth, DW). I don't like the fact that my choice of music and attire has a negative connotation like black list. As a creature of the night and non-native speaker I'm also often confused that white list is a positive list and black list is negative. I embrace night and loath sun light, so I associate black with positive and white with negative aspects. The term block list more understandable for me. 🤪

@mlindner
Copy link

mlindner commented Jun 10, 2020

One mistaken understanding broadly spread does not suddenly make it correct. "Black and white" ascribe values to colors, it has nothing to do with the color of skin. The terminology predates westerners ever encountering people from subsaharan Africa.

Who is talking about skin color? @richsalz is changing words to do me a favor!

I like to wear black cloths and listen to black music (Metal, Goth, DW). I don't like the fact that my choice of music and attire has a negative connotation like black list. As a creature of the night and non-native speaker I'm also often confused that white list is a positive list and black list is negative. I embrace night and loath sun light, so I associate black with positive and white with negative aspects. The term block list more understandable for me. 🤪

Just to clarify, it should never be "black list" or "white list". It should always be "blacklist" or "whitelist". Compound words are quite different than adjective+noun which is not what is being implied here. "blacklist" and "whitelist" are also verbs. "blocklist" is not a word and you can't use "block list" as a verb.

"I will blacklist this MAC Address from the lookup table."

@mlindner
Copy link

mlindner commented Jun 10, 2020

I thought about the term "blanks" for "whitespace", but I'm not sure it quite matches.

"whitespace" doesn't give value judgement so there is zero reason at all (more than any other change in this PR) to remove it. It comes from paper being white and text being black so the spaces between the characters are "whitespace". Removing the word "whitespace" from vocabulary would be extremely confusing as it's the term used to lump newline/line-break characters, tab characters, space characters and other such things into one classification. There's even a regex symbol "\s" that lumps all these as white-space.

@@ -505,7 +505,7 @@ int PEM_get_EVP_CIPHER_INFO(char *header, EVP_CIPHER_INFO *cipher)
return 0;
header += strspn(header, " \t");

/* We expect "ENCRYPTED" followed by optional white-space + line break */
Copy link

@mlindner mlindner Jun 10, 2020

Choose a reason for hiding this comment

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

I believe "white-space" and "whitespace" are both valid terminology. "white space" is not, so those are good to change, but I think keeping "white-space" is fine.

@richsalz
Copy link
Contributor Author

richsalz commented Jun 10, 2020

Thanks for sharing your opinions, @mlindner. While it's okay that you don't find the terms objectionable, many do, and you should similarly be okay with that as well, and respectful of their feelings no matter how nonsensical you feel it is.

As for the API/ABI changes, please see this comment upthread. #12089 (comment)

@richsalz
Copy link
Contributor Author

Even master/slave, while historically unfortunate is pretty standard.

"Historically unfortunate."

while maintaining the same idea of one piece of code controlling the other.

Please see #12089 (comment) and my reply. The parent is an input source it does not "control" the child.

@mlindner
Copy link

Even master/slave, while historically unfortunate is pretty standard.

"Historically unfortunate."

I don't see the issue here.

while maintaining the same idea of one piece of code controlling the other.

Please see #12089 (comment) and my reply. The parent is an input source it does not "control" the child.

Thanks for the detail. Having authority over the other is a master/minion relationship, parent/child I've only ever seen used in tree structures where there's an explicit ordering (parent comes first). If the master is an input source where information is fed to the minions then I think minion makes sense as the master/slave meaning made sense.

@mlindner
Copy link

mlindner commented Jun 11, 2020

Thanks for sharing your opinions, @mlindner. While it's okay that you don't find the terms objectionable, many do, and you should similarly be okay with that as well, and respectful of their feelings no matter how nonsensical you feel it is.

I think using accurate language is more important than avoiding offense at the words chosen if they are accurate to the situation they are used in.

@tmshort
Copy link
Contributor

tmshort commented Jun 11, 2020

https://www.cnet.com/news/master-and-slave-tech-terms-face-scrutiny-amid-anti-racism-efforts/

@mlindner
Copy link

mlindner commented Jun 11, 2020

https://www.cnet.com/news/master-and-slave-tech-terms-face-scrutiny-amid-anti-racism-efforts/

Just to clarify, I do think that the terms master/slave aren't the best and it's an accident of history/historically unfortunate that they've turned into common technical terms. However some of the terms are engrained into standards of technology so there is no getting rid of them. They're stuck for the foreseeable future, not matter what changes. If master/slave doesn't represent the DRBG setup correctly then it's fine to change them to be more accurate as well. That's the least egregious change in this entire PR and actually makes some sense. The rest, not so much (besides fixing obvious grammar issues).

@levitte
Copy link
Member

levitte commented Jun 11, 2020

The dispute about compound words is quite futile, really. Words like "whitelist" and "blacklist" have been composed of two, where the colours have been attribute the idea of "good" and "bad". The words could just as well have been "goodlist" and "badlist".

@mlindner
Copy link

The dispute about compound words is quite futile, really. Words like "whitelist" and "blacklist" have been composed of two, where the colours have been attribute the idea of "good" and "bad". The words could just as well have been "goodlist" and "badlist".

Yes, but they aren't. Colors have been associated with value judgements since antiquity. "The powers of light and darkness." etc and is present in many types of fiction. Extrapolating them to skin color is the work of nonsense and has never even occurred to me until this ridiculous pull request was posted and talked about. (Also "black magic" also has nothing to do with skin color, that's from fiction and "black mages" were even in Final Fantasy 1 games and earlier sources of fiction. It's referring to arcane arts as opposed to holy magic. It's saying "this is mysterious". Again, stretching this out to some kind of race issue is another work of nonsense.)

@t8m
Copy link
Member

t8m commented Jun 11, 2020

Yes, but they aren't. Colors have been associated with value judgements since antiquity. "The powers of light and darkness." etc and is present in many types of fiction. Extrapolating them to skin color is the work of nonsense and has never even occurred to me until this ridiculous pull request was posted and talked about. (Also "black magic" also has nothing to do with skin color, that's from fiction and "black mages" were even in Final Fantasy 1 games and earlier sources of fiction. It's referring to arcane arts as opposed to holy magic. It's saying "this is mysterious". Again, stretching this out to some kind of race issue is another work of nonsense.)

You seem to misunderstand what the current social justice movement is about - it actually is about forcibly removing any value association of dark, light or any other colors from minds of all people, all this because of some people having different skin tones and regardless of where the value association came from.

But yes, I have to agree we are way off-topic here.

@openssl openssl deleted a comment Jun 13, 2020
@openssl openssl deleted a comment from tiran Jun 13, 2020
@openssl openssl deleted a comment from tmshort Jun 13, 2020
@openssl openssl deleted a comment Jun 13, 2020
@openssl openssl deleted a comment Jun 13, 2020
@openssl openssl deleted a comment from mattcaswell Jun 13, 2020
@levitte
Copy link
Member

levitte commented Jun 13, 2020

The interesting bit in that draft on terminology is all the informational references, among others to other softwares that have already changed their terminology. Is all that misinformation?

@vdukhovni
Copy link

The interesting bit in that draft on terminology is all the informational references, among others to other softwares that have already changed their terminology. Is all that misinformation?

Yes, similar changes to those proposed have been made in various other projects. What's in question is the wisdom and priority of such changes, not their present popularity.

@RedShift1
Copy link

No matter which word you choose you will find someone who's offended by it. Don't change it.

@ekzotech
Copy link

Words like 'parent/child' might be unappropriate for some people, i.e. 'child-free' or those who can't have their own child. If you mind BLM, why not to mind child-free?

@xydone
Copy link

xydone commented Jun 14, 2020

We live in a clown world

@SethFalco
Copy link

Just want to give my opinion on the topic.
I'm happy to see an open discussion occurring on the matter, despite the fact I feel it's a bit trivial. I still think it's good to see this discussed, and either acted on, or determined that no action is needed, than only represented by a few thoughts and feelings in peoples heads.

I just hope if any terms do change, it is in fact because the majority believe and agree, and that there is a consensus that it should happen, and not only for "political correctness".

I appreciate that language evolves, I just firmly believe this has total disregard for context.

Many examples exist here but to further it: would "Master's degree" have to change, or maybe "Brownies", or "Brownie points", if we disregard context and where the words are derived, could they be linked with slavery or race too?

@stancl
Copy link

stancl commented Jun 15, 2020

Screen Shot 2020-06-15 at 11 06 53

The word "parent" causes trauma in me as I grew up without one parent. Use a different word, I find it offensive.

@gaal-dev
Copy link

black magic -> dark magic ? Originally, it just means satanic forces that must be avoided:)

@azymohliad
Copy link

black magic -> dark magic ? Originally, it just means satanic forces that must be avoided:)

I wonder if "black magic" is really offensive to anyone, or is it just guessing? I fail to see any relation to races and discrimination here. I'm not a native English speaker, but calling a race "black" seems even more inappropriate to me (because technically their skin color is dark brown, not black) than banning "black" as a color.

@lewislarsen
Copy link

Screen Shot 2020-06-15 at 11 06 53

The word "parent" causes trauma in me as I grew up without one parent. Use a different word, I find it offensive.

I am an orphan and the use of the word "parent" also causes me significant trauma, I support the use of a different word.

@stancl
Copy link

stancl commented Jun 15, 2020

Sorry to hear that Louise, my condolences go to you.

@openssl openssl locked as too heated and limited conversation to collaborators Jun 15, 2020
crypto/rand/rand_local.h Outdated Show resolved Hide resolved
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

There are a number of places where the 'parent' to 'primary' transition isn't finished.

@mspncp
Copy link
Contributor

mspncp commented Jun 23, 2020

The "parent" -> "primary" replacement is actually a mistake, as Mark already noted. It should be "master" -> "primary".

@levitte
Copy link
Member

levitte commented Jun 23, 2020

The "parent" -> "primary" replacement is actually a mistake, as Mark already noted. It should be "master" -> "primary".

I believe we're both pointing out the same thing, that there are places where it currently says "parent" that should say "primary"

.gitignore Outdated
@@ -24,6 +24,7 @@
/include/crypto/*_conf.h
/include/openssl/configuration.h
/include/openssl/opensslv.h
/include/openssl/fipskey.h
Copy link
Member

Choose a reason for hiding this comment

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

This probably slipped in by mistake

@mspncp
Copy link
Contributor

mspncp commented Jun 23, 2020

A RAND_DRBG instance drbg has an optional parent from which it reseeds. Nowhere is it assumed that the reseeding chain is limited to two elements, that's why it is called parent, not master resp. primary. Changing drbg->parent to drbg->primary is a mistake.

struct rand_drbg_st {
CRYPTO_RWLOCK *lock;
/* The library context this DRBG is associated with, if any */
OPENSSL_CTX *libctx;
RAND_DRBG *parent;

@mspncp
Copy link
Contributor

mspncp commented Jun 23, 2020

Ok, forget my last comment. Either I looked in the wrong place, or the error has been corrected by Rich in the meantime.

Replaced "master" DRBG with "pimary."  Fixed some markup with <private>,
etc., in some DRBG pages; use I<private>, etc.  Looking for master finds
many false positives because of TLS concepts like "master key" and such,
but I believe I fixed all. Add compat macro for the old name.

To minimize false positives, replace "white space" with "whitespace"
and teach find-doc-nits to prefer the latter. (This was partially done in
commit 6f72b21, but this finished the job.)

Replace "blacklist" with "block list" and "black magic" comments with
"magic."
@t-j-h
Copy link
Member

t-j-h commented Jul 3, 2020

@t-j-h t-j-h closed this Jul 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
hold: need omc decision The OMC needs to make a decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.