Skip to content

log the internal error message from *SSL when the cert and private key doesn't match#14023

Merged
ggreenway merged 2 commits intoenvoyproxy:masterfrom
qudongfang:better_error_msg_on_tls_cert_key_mismatch
Nov 16, 2020
Merged

log the internal error message from *SSL when the cert and private key doesn't match#14023
ggreenway merged 2 commits intoenvoyproxy:masterfrom
qudongfang:better_error_msg_on_tls_cert_key_mismatch

Conversation

@qudongfang
Copy link
Copy Markdown
Contributor

@qudongfang qudongfang commented Nov 13, 2020

Issue #14022

@repokitteh-read-only
Copy link
Copy Markdown

Hi @qudongfang, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14023 was opened by qudongfang.

see: more, trace.

…ivate doesn't match.

Signed-off-by: Dongfang Qu <qudongfang@gmail.com>
absl::StrCat("Failed to load private key from ", tls_certificate.privateKeyPath()));
throw EnvoyException(fmt::format("Failed to load private key from {}, Cause: {}",
tls_certificate.privateKeyPath(),
Utility::getLastCryptoError().value_or("not found")));
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.

"not found" here is misleading, since it sounds like file was not found.

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.

Yeah, I think you're right.
Does Unknown sound sane to you?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, unknown is much better

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.

"unknown" sounds fine, but all other uses of Utility::getLastCryptoError() in this file use .value_or("").

I'm fine with either.

Copy link
Copy Markdown
Contributor Author

@qudongfang qudongfang Nov 13, 2020

Choose a reason for hiding this comment

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

Cool, I updated it.
At the same time, I noticed that there're a few other places that we haven't checked the internal error messages.
I'd like to help if you think it's nice to have them.

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.

Go for it. Thanks!

Signed-off-by: Dongfang Qu <qudongfang@gmail.com>
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! I always like improving error reporting.

@ggreenway ggreenway self-assigned this Nov 13, 2020
@qudongfang
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14023 (comment) was created by @qudongfang.

see: more, trace.

@ggreenway ggreenway merged commit 588d934 into envoyproxy:master Nov 16, 2020
mpuncel added a commit to mpuncel/envoy that referenced this pull request Nov 18, 2020
* master: (117 commits)
  vrp: allow supervisord to open its log file (envoyproxy#14066)
  [http1] fix H/1 response pipelining (envoyproxy#13983)
  wasm: make dependency clearer (envoyproxy#14062)
  docs: updating 100-continue docs (envoyproxy#14040)
  quiche: fix stream trailer decoding issue (envoyproxy#13871)
  tidy: use last_github_commit script instead of target branch (envoyproxy#14052)
  stats: use RE2 and a better pattern to accelerate a single stats tag-extraction RE (envoyproxy#8831)
  wasm: use static registration for runtimes (envoyproxy#14014)
  grpc-json-transcoder: Add support for configuring unescaping behavior (envoyproxy#14009)
  ci: fix CodeQL-build by removing deprecated set-env command (envoyproxy#14046)
  config: fix crash when type URL doesn't match proto. (envoyproxy#14031)
  Build: Propagate user-supplied tags to external headers library. (envoyproxy#14016)
  [test host utils] use make_shared to avoid memory leaks (envoyproxy#14042)
  jwt_authn: update to jwt_verify_lib with 1 minute clock skew (envoyproxy#13872)
  quiche: update QUICHE tar (envoyproxy#13949)
  sds: improve watched directory documentation. (envoyproxy#14029)
  log the internal error message from *SSL when the cert and private key doesn't match (envoyproxy#14023)
  wasm: fix CPE for Wasmtime. (envoyproxy#14024)
  docs: Bump sphinxext-rediraffe version (envoyproxy#13996)
  CDS: remove warming cluster if CDS response desired (envoyproxy#13997)
  ...
andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020
…y doesn't match (envoyproxy#14023)

Fixes envoyproxy#14022

Signed-off-by: Dongfang Qu <qudongfang@gmail.com>
qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020
…y doesn't match (envoyproxy#14023)

Fixes envoyproxy#14022

Signed-off-by: Dongfang Qu <qudongfang@gmail.com>
Signed-off-by: Qin Qin <qqin@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants