openssl-s_client.pod.in: Fix grammar in NOTES section.#9421
openssl-s_client.pod.in: Fix grammar in NOTES section.#9421aborkowski wants to merge 1 commit intoopenssl:masterfrom
Conversation
|
This pr is ready for merge. |
mspncp
left a comment
mspncp
left a comment
There was a problem hiding this comment.
Approved for 1.1.1 as well, if applicable.
mspncp
added
branch: 1.1.1
|
@aborkowski it looks like you just added a merge commit, which is not the preferred workflow. |
CLA: trivial
aborkowski
force-pushed
the
aborkowski-fix-s_client-grammar
branch
from
1cb89d8 to
9d24f34
Compare
aborkowski
changed the title
|
@kaduk Thank you for the hint, my branch is now rebased onto current master. |
| This command is a test tool and is designed to continue the | ||
| handshake after any certificate verification errors. As a result it will | ||
| accept any certificate chain (trusted or not) sent by the peer. None test | ||
| accept any certificate chain (trusted or not) sent by the peer. Non-test |
There was a problem hiding this comment.
Rereading the sentence I get the impression that this little change does not really improve it. How about reformulating the sentence, for example as follows:
Applications should B<not> do this in productive use as it makes them vulnerable...
There was a problem hiding this comment.
Or perhaps
This is only appropriate in a testing tool; other applications need to perform certificate validation and engage error handling on validation failure. Failure to perform proper validation leaves the application vulnerable to a MITM attack.
Though I'm willing to accept the current version of this patch as a clear improvement over the existing buggy state.
There was a problem hiding this comment.
The latter was my intention, but I'm also happy to close this pull request if someone else comes up with a more substantial improvement.
| This command is a test tool and is designed to continue the | ||
| handshake after any certificate verification errors. As a result it will | ||
| accept any certificate chain (trusted or not) sent by the peer. None test | ||
| accept any certificate chain (trusted or not) sent by the peer. Non-test |
There was a problem hiding this comment.
Or perhaps
This is only appropriate in a testing tool; other applications need to perform certificate validation and engage error handling on validation failure. Failure to perform proper validation leaves the application vulnerable to a MITM attack.
Though I'm willing to accept the current version of this patch as a clear improvement over the existing buggy state.
mspncp
dismissed
their stale review
Well, it's better than it was before, so I won't object to it being merged.
iamamoose
added
the
approval: done
openssl-machine
removed
the
approval: done
|
This pull request is ready to merge |
openssl-machine
added
the
approval: ready to merge
CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #9421)
|
Thanks for your contribution.. The rewind on this took a while :). Merged to master.. |
|
Closing this now that PR #12907 has been added for the 1_1_1 branch. |
8 participants
This is just a small fix to the wording, which was ungrammatical.
Checklist