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

mTLS/TLS documentation fixes #5382

Merged
merged 7 commits into from
May 8, 2020
Merged

Conversation

darkn3rd
Copy link
Contributor

@darkn3rd darkn3rd commented May 6, 2020

mTLS/TLS documentation fixes

The text for using setting up dgraph alpha and using clients of dgraph live or curl are not correct and could compromise security. This PR fixes those specific issues.

Affects:


This change is Reviewable

Docs Preview: Dgraph Preview

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @danielmai, @darkn3rd, @MichaelJCompton, and @MichelDiz)


wiki/content/deploy/index.md, line 1652 at r1 (raw file):

.tls/ca.crt

tls/ca.crt ("tls", not ".tls"). Or, ./tls/ca.crt.


wiki/content/deploy/index.md, line 1678 at r1 (raw file):

.tls/ca.crt

tls/ca.crt ("tls", not ".tls"). Or, ./tls/ca.crt.


wiki/content/deploy/index.md, line 1680 at r1 (raw file):

 \

Do we need these backslashes at the end? I think they can be removed.


wiki/content/deploy/index.md, line 1687 at r1 (raw file):

# Now, connect to server using TLS
$ dgraph live \
   --tls_cert ./tls/client.dgraphuser.crt \

Do we need to use --tls_cacert here? It was covered above.


wiki/content/deploy/index.md, line 1749 at r1 (raw file):

--silent 

Why are we adding --silent?

Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Got few comments.

Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 13 unresolved discussions (waiting on @danielmai, @darkn3rd, @MichaelJCompton, and @MichelDiz)


wiki/content/deploy/index.md, line 1661 at r2 (raw file):

mTLS (Mutual TLS) options

This section can be folded into the Client Authentication section because Client Authentication really just means mTLS.


wiki/content/deploy/index.md, line 1701 at r2 (raw file):

| Value | Description |
|-------|-------------|
| REQUEST | Server accepts any certificate, invalid and unverified (least secure) |

While you are at it, please rephrase this table to say the following:

REQUEST -> Client-Certificate is requested. The client may or may not provide a certificate. The certificate, if provided, is not verified. (least secure)

REQUIREANY -> Client-Certificate is requested. The client must provide a certificate. The certificate is not verified.

VERIFYIFGIVEN -> Client-Certtificate is requested. The client may or may not provide a certificate. The certificate, if provided, is verified. (default)

REQUIREANDVERIFY -> Client-Certtificate is requested. The client must provide a certificate. The certificate, if provided, is verified. (most secure)

Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 15 unresolved discussions (waiting on @danielmai, @darkn3rd, @MichaelJCompton, and @MichelDiz)


wiki/content/deploy/index.md, line 1529 at r2 (raw file):

{{% notice "tip" %}}If you're generating encrypted private keys with `openssl`, be sure to specify encryption algorithm explicitly (like `-aes256`). This will force `openssl` to include `DEK-Info` header in private key, which is required to decrypt the key by Dgraph. When default encryption is used, `openssl` doesn't write that header and key can't be decrypted.{{% /notice %}}

### Self-signed certificates

This is incorrect heading. The certificate are not Self-Signed.

Rename it to "Dgraph Certificate Management tool"


wiki/content/deploy/index.md, line 1531 at r2 (raw file):

### Self-signed certificates

The `dgraph cert` program creates and manages self-signed certificates using a generated Dgraph Root CA. The _cert_ command simplifies certificate management for you.

replace self-signed certificates -> CA-signed certificates and private-keys

@github-actions github-actions bot added the area/documentation Documentation related issues. label May 7, 2020
@darkn3rd darkn3rd requested a review from parasssh May 7, 2020 19:38
Copy link
Contributor Author

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 14 unresolved discussions (waiting on @danielmai, @MichaelJCompton, @MichelDiz, and @parasssh)


wiki/content/deploy/index.md, line 1540 at r1 (raw file):

Previously, parasssh wrote…

Create node certificate and private key.

Done.


wiki/content/deploy/index.md, line 1543 at r1 (raw file):

Previously, parasssh wrote…

Create client certificate and private key for mTLS (mutual TLS)

Done.


wiki/content/deploy/index.md, line 1644 at r1 (raw file):

Previously, parasssh wrote…

... and private keys.

Done.


wiki/content/deploy/index.md, line 1652 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…
.tls/ca.crt

tls/ca.crt ("tls", not ".tls"). Or, ./tls/ca.crt.

Done.


wiki/content/deploy/index.md, line 1678 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…
.tls/ca.crt

tls/ca.crt ("tls", not ".tls"). Or, ./tls/ca.crt.

Done.


wiki/content/deploy/index.md, line 1680 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…
 \

Do we need these backslashes at the end? I think they can be removed.

Done.


wiki/content/deploy/index.md, line 1687 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Do we need to use --tls_cacert here? It was covered above.

Done.


wiki/content/deploy/index.md, line 1749 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…
--silent 

Why are we adding --silent?

Done.


wiki/content/deploy/index.md, line 1529 at r2 (raw file):

Previously, parasssh wrote…

This is incorrect heading. The certificate are not Self-Signed.

Rename it to "Dgraph Certificate Management tool"

Done.


wiki/content/deploy/index.md, line 1531 at r2 (raw file):

Previously, parasssh wrote…

replace self-signed certificates -> CA-signed certificates and private-keys

Done.


wiki/content/deploy/index.md, line 1661 at r2 (raw file):

Previously, parasssh wrote…

This section can be folded into the Client Authentication section because Client Authentication really just means mTLS.

I think I fixed it.


wiki/content/deploy/index.md, line 1667 at r2 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…

Thank you for explanation.

Done.


wiki/content/deploy/index.md, line 1701 at r2 (raw file):

Previously, parasssh wrote…

While you are at it, please rephrase this table to say the following:

REQUEST -> Client-Certificate is requested. The client may or may not provide a certificate. The certificate, if provided, is not verified. (least secure)

REQUIREANY -> Client-Certificate is requested. The client must provide a certificate. The certificate is not verified.

VERIFYIFGIVEN -> Client-Certtificate is requested. The client may or may not provide a certificate. The certificate, if provided, is verified. (default)

REQUIREANDVERIFY -> Client-Certtificate is requested. The client must provide a certificate. The certificate, if provided, is verified. (most secure)

I chose a different approach, let me know if this works.
I can add another column for Client Verified?, but given word VERIFY in option, seems self-documenting.


wiki/content/deploy/index.md, line 1742 at r2 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…

I was getting progress bar for a small query. Do we want users to have progress bar for small queries. Progress bars are fine for downloading tarballs, but for API, not the preferred experience.

Done.

@darkn3rd darkn3rd requested a review from parasssh May 7, 2020 19:57
Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 15 unresolved discussions (waiting on @danielmai, @darkn3rd, @MichaelJCompton, @MichelDiz, and @parasssh)


wiki/content/deploy/index.md, line 1661 at r2 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…

I think I fixed it.

I meant move this entire section under Client Authentication


wiki/content/deploy/index.md, line 1701 at r2 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…

I chose a different approach, let me know if this works.
I can add another column for Client Verified?, but given word VERIFY in option, seems self-documenting.

Its better to spell it out. As discussed on Slack, we can have 4 columns and 5 rows (4 for the options and 5th for option absent).

…ated for curl section in relation to client authentication
Copy link
Contributor Author

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 15 unresolved discussions (waiting on @danielmai, @darkn3rd, @MichaelJCompton, @MichelDiz, and @parasssh)


wiki/content/deploy/index.md, line 1749 at r1 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…

Done.

Progress Bar for API is not intuitive.


wiki/content/deploy/index.md, line 1661 at r2 (raw file):

Previously, parasssh wrote…

I meant move this entire section under Client Authentication

Done.


wiki/content/deploy/index.md, line 1701 at r2 (raw file):

Previously, parasssh wrote…

Its better to spell it out. As discussed on Slack, we can have 4 columns and 5 rows (4 for the options and 5th for option absent).

Done.


wiki/content/deploy/index.md, line 1701 at r3 (raw file):

Previously, parasssh wrote…

Please change all "Description" to what I provided in previous comment.

Done.

Copy link
Contributor Author

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

These were resolved in a later commit, I am not sure why this UI shows up.

wiki/content/deploy/index.md Outdated Show resolved Hide resolved
wiki/content/deploy/index.md Outdated Show resolved Hide resolved
wiki/content/deploy/index.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

These were resolved in a later commit.

Copy link
Contributor Author

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

These were all marked resolved and addressed in commit after this point.

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

A few comments, and then :lgtm:.

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @darkn3rd and @parasssh)


wiki/content/deploy/index.md, line 1662 at r4 (raw file):

### Client authentication

#### mTLS (Mutual TLS) options

We can remove this header. Keep everything saying "Client authentication".


wiki/content/deploy/index.md, line 1664 at r4 (raw file):

The following configuration options are available for Alpha:

Dgraph Alpha can be configured with the following options for client authentication

(This phrasing follows the parallelism with the next paragraph about live loader).


wiki/content/deploy/index.md, line 1677 at r4 (raw file):

with following options:

with the following options:


wiki/content/deploy/index.md, line 1698 at r4 (raw file):

The server will always REQUEST Client Authentication **only** when the `--tls_client_auth` option is specified. 

We can remove this sentence. --tls_client_auth has a default value of VERIFYIFGIVEN, so this sentence doesn't really make sense.

Copy link
Contributor Author

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 14 unresolved discussions (waiting on @danielmai and @parasssh)


wiki/content/deploy/index.md, line 1661 at r3 (raw file):

Previously, parasssh wrote…

Still think it is a good idea to fold this into Client Authentication section below.

Everything is client authentication.


wiki/content/deploy/index.md, line 1662 at r4 (raw file):

Previously, danielmai (Daniel Mai) wrote…

We can remove this header. Keep everything saying "Client authentication".

Done.


wiki/content/deploy/index.md, line 1664 at r4 (raw file):

Previously, danielmai (Daniel Mai) wrote…
The following configuration options are available for Alpha:

Dgraph Alpha can be configured with the following options for client authentication

(This phrasing follows the parallelism with the next paragraph about live loader).

This has been revised. Before misunderstanding, so revised to have one common section, and detail commands for with and without client auth.


wiki/content/deploy/index.md, line 1677 at r4 (raw file):

Previously, danielmai (Daniel Mai) wrote…
with following options:

with the following options:

Done.


wiki/content/deploy/index.md, line 1698 at r4 (raw file):

Previously, danielmai (Daniel Mai) wrote…
The server will always REQUEST Client Authentication **only** when the `--tls_client_auth` option is specified. 

We can remove this sentence. --tls_client_auth has a default value of VERIFYIFGIVEN, so this sentence doesn't really make sense.

This sentence have been revised.

Copy link
Contributor Author

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 1 files reviewed, 14 unresolved discussions (waiting on @danielmai and @parasssh)

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 1 files reviewed, 14 unresolved discussions (waiting on @danielmai and @parasssh)

@darkn3rd darkn3rd merged commit cd454e5 into master May 8, 2020
@darkn3rd darkn3rd deleted the joaquin/deploy_tls_doc_bug_fixes branch May 8, 2020 04:06
darkn3rd added a commit that referenced this pull request Jul 7, 2020
* doc fixes for mTLS/TLS dgraph live, dgraph cert, curl

(cherry picked from commit cd454e5)
darkn3rd added a commit that referenced this pull request Jul 8, 2020
* doc: wiki deploy fixes for mTLS/TLS dgraph live, dgraph cert, curl (cherry picked from commit cd454e5)
danielmai pushed a commit that referenced this pull request Jul 9, 2020
* doc fixes for mTLS/TLS dgraph live, dgraph cert, curl
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
* doc fixes for mTLS/TLS dgraph live, dgraph cert, curl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Documentation related issues.
Development

Successfully merging this pull request may close these issues.

3 participants