Skip to content

examples: make tls example easier to run#5715

Merged
jtattermusch merged 2 commits intogrpc:masterfrom
carl-mastrangelo:tls2
May 10, 2019
Merged

examples: make tls example easier to run#5715
jtattermusch merged 2 commits intogrpc:masterfrom
carl-mastrangelo:tls2

Conversation

@carl-mastrangelo
Copy link
Copy Markdown
Contributor

@carl-mastrangelo carl-mastrangelo commented May 9, 2019

  • Make the server cert able to be verified by the ca cert in openssl
  • Make the port number consistent in each example (easy to copy paste wrong one)
  • use correct netty-tcnative

cc: @jtattermusch

* Make the ca cert able to be verified by the server cert in openssl
* Make the port number consistent in each example (easy to copy paste wrong one)
* use correct netty-tcnative
@@ -43,14 +43,15 @@ You can use the following script to generate self-signed certificates for grpc-j
mkdir -p /tmp/sslcert
pushd /tmp/sslcert
# Changes these CN's to match your hosts in your environment if needed.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes => Change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

mkdir -p /tmp/sslcert
pushd /tmp/sslcert
# Changes these CN's to match your hosts in your environment if needed.
SERVER_CA_CN=localhost-ca # must be different than SERVER_CN
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need the comment "# must be different than SERVER_CN" ?

The comment is the justification for this change but why leave it in the README file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I suppose not.

openssl req -passin pass:1111 -new -x509 -days 365 -key ca.key -out ca.crt -subj "/CN=${SERVER_CA_CN}"
echo Generate server key:
openssl genrsa -passout pass:1111 -des3 -out server.key 4096
echo Generate server signing request:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lines 59 and 68 say "Self-signed..." which is incorrect, so we can fix that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what would you prefer? I think self-signed is pretty clear, it means someone didnt give it to us.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See https://en.wikipedia.org/wiki/Self-signed_certificate

In technical terms a self-signed certificate is one signed with its own private key.

Here server and client certs are signed by the ca cert's private key.

@sanjaypujare
Copy link
Copy Markdown
Contributor

  • Make the ca cert able to be verified by the server cert in openssl

Shouldn't this be "Make the server cert able to be verified by the ca cert in openssl" ?

Also this change is not making any material difference to the code, right? Because each cert is in its own file (server.crt vs ca.crt) and we are separately passing ca.crt as the trust-store it doesn't really matter that we used the same name "localhost" as the CN for both certs. In any case the example wouldn't have worked if that was the problem. I am okay with this change, but just trying to clarify.

@carl-mastrangelo
Copy link
Copy Markdown
Contributor Author

Shouldn't this be "Make the server cert able to be verified by the ca cert in openssl" ?

done.

Also this change is not making any material difference to the code, right? Because each cert is in its own file (server.crt vs ca.crt) and we are separately passing ca.crt as the trust-store it doesn't really matter that we used the same name "localhost" as the CN for both certs.

In java it worked. The origin of this PR is @jtattermusch trying out our example and finding it confusing and not working out of the box. The certs could not be used by the C version of gRPC.

Copy link
Copy Markdown
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

Couple of suggestions but LGTM

Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM.

@jtattermusch jtattermusch merged commit af51e96 into grpc:master May 10, 2019
ejona86 pushed a commit to ejona86/grpc-java that referenced this pull request May 16, 2019
* examples: make tls example easier to run

* Make the ca cert able to be verified by the server cert in openssl
* Make the port number consistent in each example (easy to copy paste wrong one)
* use correct netty-tcnative

* address comments
ejona86 pushed a commit that referenced this pull request May 16, 2019
* examples: make tls example easier to run

* Make the ca cert able to be verified by the server cert in openssl
* Make the port number consistent in each example (easy to copy paste wrong one)
* use correct netty-tcnative

* address comments
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants