-
Notifications
You must be signed in to change notification settings - Fork 455
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
add --cert-principal-map #7104
add --cert-principal-map #7104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade)
v20.1/create-security-certificates-openssl.md, line 77 at r1 (raw file):
{% include copy-clipboard.html %} ~~~ shell $ mkdir node-certs client-certs my-safe-directory
Is there a reason for using separate directories for the node and client certs? @dbist needed to do this in testing because of limitations in the cockroach cert
commands, but it shouldn't be necessary for the instructions here.
v20.1/create-security-certificates-openssl.md, line 189 at r1 (raw file):
[ distinguished_name ] organizationName = Cockroach commonName = DNS:<node-hostname>,DNS:<node-domain>,IP:<IP Address>
My understanding is that the commonName
field is but a single field. If you specify multiple names like this, the first is stored in the commonName
field and the others are stored in the subjectAltName
fields. I'm not sure how this actually plays out with the OpenSSL configuration file, though. So what you have may actually be correct, though it is a bit strange to have to specify the same fields in both commonName
and subjectAltName
. That is incompatible with my mental model.
Cc @aaron-crl for confirmation.
v20.1/create-security-certificates-openssl.md, line 345 at r1 (raw file):
{% include copy-clipboard.html %} ~~~ shell $ cockroach start-single-node --certs-dir=node-certs --cert-principal-map=node.crdb.io:node --background
The value of node.crdb.io
is dependent on how the cert was created. Perhaps this should be <node-domain>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR, @petermattis ! Does the overall approach make sense? Is it okay to document only the principal mapping approach and not to document the commonName=node
approach?
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl, @dbist, and @petermattis)
v20.1/create-security-certificates-openssl.md, line 77 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Is there a reason for using separate directories for the node and client certs? @dbist needed to do this in testing because of limitations in the
cockroach cert
commands, but it shouldn't be necessary for the instructions here.
IIRC, we ran into the same issues with openssl commands as well. Will retry the steps without separate directories and report back.
v20.1/create-security-certificates-openssl.md, line 189 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
My understanding is that the
commonName
field is but a single field. If you specify multiple names like this, the first is stored in thecommonName
field and the others are stored in thesubjectAltName
fields. I'm not sure how this actually plays out with the OpenSSL configuration file, though. So what you have may actually be correct, though it is a bit strange to have to specify the same fields in bothcommonName
andsubjectAltName
. That is incompatible with my mental model.Cc @aaron-crl for confirmation.
Yeah, I am not sure of this either. I was under the impression that commonName
is a single-value field, but couldn't figure out which value should it have and what should go in the subjectAltName
. Should <node-domain>
be in the commonName
field and everything else in subjectAltName
?
v20.1/create-security-certificates-openssl.md, line 345 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
The value of
node.crdb.io
is dependent on how the cert was created. Perhaps this should be<node-domain>
.
Yep. Sorry. Fixing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl, @dbist, and @petermattis)
v20.1/create-security-certificates-openssl.md, line 77 at r1 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
IIRC, we ran into the same issues with openssl commands as well. Will retry the steps without separate directories and report back.
Update: Single directory didn't work :(
Retried with separate directories and it worked!
v20.1/create-security-certificates-openssl.md, line 189 at r1 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
Yeah, I am not sure of this either. I was under the impression that
commonName
is a single-value field, but couldn't figure out which value should it have and what should go in thesubjectAltName
. Should<node-domain>
be in thecommonName
field and everything else insubjectAltName
?
Update: Tried <node-domain>
in the commonName field and everything else in subjectAltName
and it didn't work.
Tried <node-domain>
be in the commonName field and everything (including <node-domain>
) in subjectAltName
and that worked!
commonName = DNS:node.crdb.io
[ extensions ]
subjectAltName = DNS:node.crdb.io,DNS:localhost,IP:127.0.0.1
@dbist via Slack: Update: Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dbist and @petermattis)
v20.1/create-security-certificates-openssl.md, line 189 at r1 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
Update: Tried
<node-domain>
in the commonName field and everything else insubjectAltName
and it didn't work.
Tried<node-domain>
be in the commonName field and everything (including<node-domain>
) insubjectAltName
and that worked!commonName = DNS:node.crdb.io [ extensions ] subjectAltName = DNS:node.crdb.io,DNS:localhost,IP:127.0.0.1
After poking at this, I think this is borne out of how certificate use changed per RFC 5280.
commonName
used to be is expected to represent the dns hostname or IP of a server but ultimately it's just a printable string.
The subjectAltName
extension is meant to eventually replace commonName
because it adds typing to the subject value.
Today, it is incumbent on the consumer to check both fields as the RFC states "The subject name MAY be
carried in the subject field and/or the subjectAltName extension."
We know that CRDB now can do this courtesy of cockroachdb/cockroach#45819 meaning that the value of the commonName field doesn't actually matter as long as the correct value(s) appear in the subjectAltName(s).
The configuration immediately above would result in the commonName being set to "DNS:node.crdb.io" which isn't a valid URL, but because it is correctly represented as a DNS entry in subjectAltName as node.crdb.io
it doesn't matter.
It doesn't hurt anything to have it in both places today but it doesn't help either.
On an aside according to RFC 5280 "If subject naming information is present only in the subjectAltName extension (e.g., a key bound only to an email address or URI), then the subject name MUST be an empty sequence and the subjectAltName extension MUST be critical."
I was able to get OpenSSL to generate a certificate that matches this pattern with the below modifications but haven't yet had time to test it against the database.
In ca.cnf
replace "commonName = supplied" with "commonName = optional"
For 'node.cnf' I removed the commonName from the distinguished_name section and set subjectAltName as critical.
# OpenSSL node configuration file
[ req ]
prompt=no
distinguished_name = distinguished_name
req_extensions = extensions
[ distinguished_name ]
organizationName = Cockroach
[ extensions ]
subjectAltName = critical,DNS:localhost,DNS:node,IP:0.0.0.0
This is confusing :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dbist and @petermattis)
v20.1/create-security-certificates-openssl.md, line 189 at r1 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
This is confusing :) For now, I feel it's safer to recommend setting all values in both commonName and subjectAltName, especially since things seem to be in flux. Is that okay?
I think "in flux is relative" unfortunately. RFC 5280 is 12 years old now, but commonName is still widely used.
As for setting all values in both, we can't, at least not functionally. The best we can do is pick on to set in commonName and then either set it again with the others in subjectAltName or omit it there. Neither of which feel very satisfying to me.
I think moving all values to the subjectAltName extension is the "correct" security decision - and simpler in the long run. It's just not the way we've done things in the past. open to other thoughts though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dbist and @petermattis)
v20.1/create-security-certificates-openssl.md, line 189 at r1 (raw file):
Previously, aaron-crl wrote…
I think "in flux is relative" unfortunately. RFC 5280 is 12 years old now, but commonName is still widely used.
As for setting all values in both, we can't, at least not functionally. The best we can do is pick on to set in commonName and then either set it again with the others in subjectAltName or omit it there. Neither of which feel very satisfying to me.
I think moving all values to the subjectAltName extension is the "correct" security decision - and simpler in the long run. It's just not the way we've done things in the past. open to other thoughts though.
Tried the steps we discussed and they worked! Updated the doc :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the steps without a hitch. I think the following nits will improve the overall document.
-
In the root client creation section, rephrase repeat steps 1-5 for each additional client to refer to step 5. creating cert for clients other than
root
. -
rename Step 5 to say: create cert for client other than
root
. -
In step 5.1, creating additional clients, rephrase create
client.cnf
to create or reuse the one used forroot
user. It is the same content, just note to changecommonName
for each additional user. -
in the step to start cluster, I'd include
<fqdn,nodename>
. Check with others on the thoughts? -
In Peter's document, he provided a step to introspect certificates, I think it's valuable to add here for both client and node certs at the end of each respective certificate creation sections.
openssl x509 -in node-certs/node.crt -text | grep "X509v3 Subject Alternative Name" -A 1
X509v3 Subject Alternative Name: critical
DNS:localhost, DNS:node.example.com, IP Address:127.0.0.1
openssl x509 -in client-certs/client.root.crt -text | grep CN=
Issuer: O=Cockroach, CN=Cockroach CA
Subject: O=Cockroach, CN=root
openssl x509 -in client-certs/client.roach.crt -text | grep CN=
Issuer: O=Cockroach, CN=Cockroach CA
Subject: O=Cockroach, CN=roach
- finally, add a step to use the client cert in the very last step
cockroach sql --certs-dir=client-certs --user=roach
Oh, I had a question about this as I was writing it and forgot to ask it in the channel: Should I just delete steps 5 and 6 from the "create certs for root" section? We expect the users to connect to the cluster using their local machine using the root cert, right? Not upload the cert anywhere?
Done. Renamed to "Create the certificate and key pair for a non-
Good catch. I meant to say "edit" instead of "create". Is that the right way though? Should they reuse the file or create a new one for each client?
cc @aaron-crl for thoughts
Oooh...That's a great idea! Added!
Done. |
Yes..sorry. Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't scrutinize all of the step-by-step instructions. So glad that we provide these, though. They are somewhat onerous to create.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade, @dbist, and @petermattis)
v20.1/cockroach-start.md, line 89 at r5 (raw file):
`--certs-dir` | The path to the [certificate directory](cockroach-cert.html). The directory must contain valid certificates if running in secure mode.<br><br>**Default:** `${HOME}/.cockroach-certs/` `--insecure` | Run in insecure mode. If this flag is not set, the `--certs-dir` flag must point to valid certificates.<br><br><strong>Note the following risks:</strong> An insecure cluster is open to any client that can access any node's IP addresses; any user, even `root`, can log in without providing a password; any user, connecting as `root`, can read or write any data in your cluster; and there is no network encryption or authentication, and thus no confidentiality.<br><br>**Default:** `false` `--cert-principal-map` | A comma-separated list of `cert-principal:db-principal` mappings used to map the certificate principals to IP addresses, DNS names, and SQL users. This allows the use of certificates generated by Certificate Authorities that place restrictions on the contents of the `commonName` field. For usage information, see [Create Security Certificates using Openssl](create-security-certificates-openssl.html#examples)
The first sentence here is confusing to me. Specifically this part: used to map the certificate principals to IP addresses, DNS names, and SQL users
. The flag is used to map certificate principals to DB principals such as SQL users or the special "root" and "node" users. What are we trying to convey with the mention of IP addresses and DNS names?
v20.1/create-security-certificates-openssl.md, line 77 at r1 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
Update: Single directory didn't work :(
Retried with separate directories and it worked!
I wonder why a single directory didn't work. With openssl it should work. I don't mind the extra directory, just curious.
@petermattis in my tests with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries! Aaron and Artem tried out the steps, so I feel pretty confident about the instructions :)
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dbist and @petermattis)
v20.1/cockroach-start.md, line 89 at r5 (raw file):
Previously, petermattis (Peter Mattis) wrote…
The first sentence here is confusing to me. Specifically this part:
used to map the certificate principals to IP addresses, DNS names, and SQL users
. The flag is used to map certificate principals to DB principals such as SQL users or the special "root" and "node" users. What are we trying to convey with the mention of IP addresses and DNS names?
This is confusing 😕I thought "node" and "root" were certificate principals.
Is this right: "used to map certificate principals (IP addresses and DNS names) to database principals (SQL users, node
, root
)"
v20.1/create-security-certificates-openssl.md, line 77 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
I wonder why a single directory didn't work. With openssl it should work. I don't mind the extra directory, just curious.
I don't know :( I got a client certificate error when I tried with a single directory.
We may want to change the default certificate lifespans to bring usage in line with what OSX and Chrome expect per: #6946 (comment) Namely:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade, @dbist, and @petermattis)
v20.1/create-security-certificates-openssl.md, line 77 at r1 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
I don't know :( I got a client certificate error when I tried with a single directory.
I'd like to see that error if you can generate it? It doesn't make sense to me that it should fail.
v20.1/create-security-certificates-openssl.md, line 250 at r1 (raw file):
$ scp node-certs/ca.crt \ node-certs/node.crt \ Node-certs/node.key \
Does this break something? Node
vs node
?
Same error as Artem's:
Fixed :) |
@Amruta-Ranade here's my write-up with single directory and
|
Updated the instructions to use the connection URL instead of cc @aaron-crl @dbist @petermattis for final checks/approvals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aside from mentioning to remove .pem
file once because all of them will reside in the single certs
dir, this looks good to me. The client.cnf
file for additional users is missing SAN but it's not a deal breaker. Mine looks like so
[ extensions ]
subjectAltName = email:user2@example.com,DNS:user2.example.com,DNS:user2
which demonstrates this user is not root and also referenced by short name user2
. Again not major. This is long and painful, great job, thank you for sticking with it! LGTM after removing .pem
.
Updated as discussed on Slack.
The reason I didn't include SAN in the second user's conf is that we aren't mapping the second user to a DB principal, whereas in the first user's conf, we are mapping the user to root. It makes things clearer to me to see what's mapped and what isn't. But I might be mistaken. cc @aaron-crl for input.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few suggestions and got an error when running through the steps.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl, @Amruta-Ranade, @dbist, @jseldess, and @petermattis)
v20.1/cockroach-start.md, line 89 at r10 (raw file):
`--certs-dir` | The path to the [certificate directory](cockroach-cert.html). The directory must contain valid certificates if running in secure mode.<br><br>**Default:** `${HOME}/.cockroach-certs/` `--insecure` | Run in insecure mode. If this flag is not set, the `--certs-dir` flag must point to valid certificates.<br><br><strong>Note the following risks:</strong> An insecure cluster is open to any client that can access any node's IP addresses; any user, even `root`, can log in without providing a password; any user, connecting as `root`, can read or write any data in your cluster; and there is no network encryption or authentication, and thus no confidentiality.<br><br>**Default:** `false` `--cert-principal-map` | A comma-separated list of `cert-principal:db-principal` mappings used to map the certificate principals to IP addresses, DNS names, and SQL users. This allows the use of certificates generated by Certificate Authorities that place restrictions on the contents of the `commonName` field. For usage information, see [Create Security Certificates using Openssl](create-security-certificates-openssl.html#examples)
Add a "new in v20.1" callout:
`--cert-principal-map` | <span class="version-tag">New in v20.1:</span> A comma-separated list of...
Add a period at end of paragarph.
Also, I think we need to add this flag to the cockroach start-single-node
docs as well? Not sure about cockroach demo
?
v20.1/create-security-certificates-openssl.md, line 157 at r10 (raw file):
~~~ 6. Reset database and index files.
Replace period with colon.
v20.1/create-security-certificates-openssl.md, line 193 at r10 (raw file):
~~~ {{site.data.alerts.callout_danger}}The <code>subjectAltName</code> parameter is vital for CockroachDB functions. You can modify or omit other parameters as per your preferred OpenSSL configuration, but do not omit the <code>subjectAltName</code> parameter. {{site.data.alerts.end}}
nit: While you're here, you could convert the formatting in the callout to markdown.
v20.1/create-security-certificates-openssl.md, line 206 at r10 (raw file):
~~~ 3. Create the CSR for the first node using the [`openssl req`](https://www.openssl.org/docs/manmaster/man1/req.html) command:
This step fails for me now:
~$ openssl req -new -config node.cnf -key certs/node.key -out node.csr -batch
Error Loading request extension section extensions
4705336940:error:22FFF076:X509 V3 routines:func(4095):bad ip address:/AppleInternal/BuildRoot/Library/Caches/com.apple.xbs/Sources/libressl/libressl-47.100.4/libressl-2.8/crypto/x509v3/v3_alt.c:529:value=<IP Address>
4705336940:error:22FFF080:X509 V3 routines:func(4095):error in extension:/AppleInternal/BuildRoot/Library/Caches/com.apple.xbs/Sources/libressl/libressl-47.100.4/libressl-2.8/crypto/x509v3/v3_conf.c:100:name=subjectAltName, value=DNS:<node-hostname>,DNS:<node-domain>,IP:<IP Address>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl, @Amruta-Ranade, @dbist, @jseldess, and @petermattis)
v20.1/create-security-certificates-openssl.md, line 206 at r10 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
This step fails for me now:
~$ openssl req -new -config node.cnf -key certs/node.key -out node.csr -batch Error Loading request extension section extensions 4705336940:error:22FFF076:X509 V3 routines:func(4095):bad ip address:/AppleInternal/BuildRoot/Library/Caches/com.apple.xbs/Sources/libressl/libressl-47.100.4/libressl-2.8/crypto/x509v3/v3_alt.c:529:value=<IP Address> 4705336940:error:22FFF080:X509 V3 routines:func(4095):error in extension:/AppleInternal/BuildRoot/Library/Caches/com.apple.xbs/Sources/libressl/libressl-47.100.4/libressl-2.8/crypto/x509v3/v3_conf.c:100:name=subjectAltName, value=DNS:<node-hostname>,DNS:<node-domain>,IP:<IP Address>
Oh, the user has to replace the placeholders in the .cnf file with their values. I use "DNS:localhost,DNS:node.crdb.io,IP:127.0.0.1" for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got through it well. LGTM with a few suggestions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl, @Amruta-Ranade, @dbist, @jseldess, and @petermattis)
v20.1/create-security-certificates-openssl.md, line 250 at r10 (raw file):
6. Remove the `.pem` files in the `certs` directory. These files are unnecessary duplicates of the `.crt` files that CockroachDB requires. ### Step 3. Create the certificate and key pair for the first user
Probably want an intro about placeholders, like you have for step 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl, @Amruta-Ranade, @dbist, @jseldess, and @petermattis)
v20.1/create-security-certificates-openssl.md, line 157 at r10 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Replace period with colon.
Done.
v20.1/create-security-certificates-openssl.md, line 250 at r10 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Probably want an intro about placeholders, like you have for step 2.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a difference in the first and second url, for the root user, you didn't include place holder <username_1> in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding using sql --url
instead of sql --certs-dir
. Glad that approach could get rid of the separate client-certs
and node-certs
directories. I didn't actually walk through the examples step by step, but I trust you and others did.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @aaron-crl, @Amruta-Ranade, @dbist, @jseldess, and @petermattis)
I didn't add <username_1> for the first URL because it doesn't work 😕It gives the following error: It works without the username because it assumes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @aaron-crl, @dbist, @jseldess, and @petermattis)
v20.1/cockroach-start.md, line 89 at r10 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Add a "new in v20.1" callout:
`--cert-principal-map` | <span class="version-tag">New in v20.1:</span> A comma-separated list of...
Add a period at end of paragarph.
Also, I think we need to add this flag to the
cockroach start-single-node
docs as well? Not sure aboutcockroach demo
?
Added the callout and period and updated the cockroach start-single-node
doc. Checking if the flag works for demo.
it works with the following configuration file
my user2 client.cnf file is like so:
my command to start the cluster Again, it's a matter of preference but at least we know the reasoning was intentional. LGTM! |
Closes #6967
cc @piyush-singh for visibility: The docs and the review comments might help guide your acceptance testing efforts :)