Fix: confusing phrasing #639
Conversation
|
We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story. The labels on this github issue will be updated when the story is started. |
bruce-ricard
force-pushed
the
improve_docs
branch
from
cdc80f5 to
df25737
Compare
docs/credhub-security-faq.md
Outdated
| started in internal encryption mode, CredHub deterministically | ||
| generates a site-specific AES256 key on startup. CredHub concatenates | ||
| a user-defined password from its configuration file with a | ||
| started in internal encryption mode, CredHub recalculates |
There was a problem hiding this comment.
Maybe "calculates" is better? It could be the first time it calculates it, if the user just provided a new encryption password. But it will recalculate it every time after that.
How can we phrase this?
There was a problem hiding this comment.
I'm having trouble understanding what key it's talking about here. I was assuming it's the key used to encrypt data at rest, but I see that it's changed on startup, which doesn't fit that idea. Is this key used to communicate with clients?
There was a problem hiding this comment.
Perfect, thanks for sharing that. That is pretty much exactly the confusion I'm trying to solve. Apparently my first attempt is a failure.
It is the key used to encrypt data at rest.
I see that it's changed on startup
Which part makes you say that? Because it's not.
There was a problem hiding this comment.
The previous version of the docs says that it "generates a site-specific AES256 key on startup" which IMO is very confusing, as it appears that it creates a new key. But it doesn't. It's the same key every time that is computed. That's how it works.
There was a problem hiding this comment.
Ah, ok. So saying "recalculates" I think is just as confusing as "generates". I don't understand what you're distinguishing between the first startup and subsequent startups, if the action is the same and the resulting key is the same each time.
There was a problem hiding this comment.
the action is the same and the resulting key is the same each time
Yes
I don't understand what you're distinguishing between the first startup and subsequent startups
Fair enough. So would "calculates" be better then?
There was a problem hiding this comment.
What made me think it was changed at every startup was "recalculates" and the "randomly generated" part of the salt (as a naive reader). Now that I understand better, I agree that "generate" isn't a good way to explain it. So, yes, I like "calculates". And in the spirit of the "deterministically" part that you removed, maybe a brief explanation like "The key will be the same every time CredHub starts as long as the user-provided password has not changed."
bruce-ricard
force-pushed
the
improve_docs
branch
from
df25737 to
1232b22
Compare
docs/credhub-security-faq.md
Outdated
| a user-defined password from its configuration file with a | ||
| started in internal encryption mode, CredHub recalculates | ||
| the AES256 key on startup. To do this, CredHub concatenates | ||
| the user-provided encryption password from its configuration file with a | ||
| randomly-generated salt stored in its database and hashes the |
There was a problem hiding this comment.
It's not clear from this when CredHub is randomly generating the salt. Could be at every startup from what this says.
There was a problem hiding this comment.
OK. To me, "stored in its database" was enough to explain that it is only generated the first time.
I'm going to rewrite the whole thing, and split it in 2 sections:
- what happens the first time
- what happens on the next restarts of the server
that should make everything clearer.
There was a problem hiding this comment.
For what it's worth, the other edits look fine.
|
Something's wrong with my commit signatures. I have to fix that. |
|
|
||
| If CredHub is started in internal encryption mode: | ||
|
|
||
| * If a new encryption key is provided, CredHub will generate a new |
There was a problem hiding this comment.
Should this say "If a new encryption password is provided"? Is it a password or a key? Same for the bullet below.
There was a problem hiding this comment.
I agree, I struggled to phrase this. It's a bit annoying, because the credhub-release spec talks about encryption keys. But what it expects, in the case of internal keys, is only the password.
So mathematically, an encryption key and the password used to generate it, are the same thing. There's a one to one mapping between keys and passwords.
There was a problem hiding this comment.
I see. So the spec gives most of the details about the key, just not the salt. But we can talk about individual properties of that section of the spec, like encryption_password.
I'm not sure what happens if the user edits the encryption_key_name property.
| This design allows CredHub to have access to the AES key at all times, | ||
| without ever having to store it on disk. | ||
|
|
||
| Passwords and salts cannot be rotated individually, and they are never |
There was a problem hiding this comment.
This could probably be simplified. What people most likely care about is the the salt that can't be rotated by itself. This already explains that the salt will be changed when the password is changed. Maybe -
The password and salt are never automatically rotated. Also, the salt cannot be rotated by itself without changing the password.
* "generates" isn't the correct word here. CredHub doesn't generate a key. It calculates (or computes) it. * "deterministically" IMO can never be used with "generate", as "generate" implies "non deterministically", otherwise it's not generation, it's computation. * "site-specific": I don't understand this term, or how it helps make the documentation clearer. * "user-defined password": I think that "user-provided" is better. * "a key": was changed multiple times to "the key". We are talking about _the_ key that will be used for encryption by CredHub, not any AES key. * "lifetime of the server": this precison feels superfluous, and confusing. The lifetime of the CredHub server is so far about 7 years. I understand that the author meant "until the server instance stops running". I thought about rephrasing it, but since IMO it is superfluous, as this is exactly how memory works, I decided to remove it. * I should have made multiple commits for this change. Apologies to micro-commit advocates.
* split first and subsequent key uses to explain how the salt is dealt with * specify that the AES key is always the same, as people were confused about that
* The text after the line break was intended to be part of the heading, but they weren't rendered as part of the heading.
bruce-ricard
force-pushed
the
improve_docs
branch
from
3d34186 to
ca13236
Compare
"generates" isn't the correct word here. CredHub doesn't generate a key. It calculates (or computes) it.
"deterministically" IMO can never be used with "generate", as "generate" implies "non deterministically", otherwise it's not generation, it's computation.
"site-specific": I don't understand this term, or how it helps make the documentation clearer.
"user-defined password": I think that "user-provided" is better.
"a key": was changed multiple times to "the key". We are talking about the key that will be used for encryption by CredHub, not any AES key.
"lifetime of the server": this precison feels superfluous, and confusing. The lifetime of the CredHub server is so far about 7 years. I understand that the author meant "until the server instance stops running". I thought about rephrasing it, but since IMO it is superfluous, as this is exactly how memory works, I decided to remove it.
I should have made multiple commits for this change. Apologies to micro-commit advocates.