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.

Sign up for GitHub

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

Jump to bottom

openssl-s_client.pod.in: Fix grammar in NOTES section. #9421

Closed
wants to merge 1 commit into from
Closed

openssl-s_client.pod.in: Fix grammar in NOTES section. #9421

wants to merge 1 commit into from

Conversation

aborkowski
Copy link
Contributor

@aborkowski aborkowski commented Jul 20, 2019 •
edited by levitte

This is just a small fix to the wording, which was ungrammatical.

Checklist
  • documentation is added or updated

@levitte levitte self-assigned this Jul 21, 2019
@mspncp
Copy link
Contributor

mspncp commented Oct 12, 2019

This pr is ready for merge.

mspncp
mspncp approved these changes Oct 12, 2019
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

Approved for 1.1.1 as well, if applicable.

@mspncp mspncp added branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: master Merge to master branch labels Oct 12, 2019
@kaduk
Copy link
Contributor

kaduk commented Dec 17, 2019

@aborkowski it looks like you just added a merge commit, which is not the preferred workflow.
It would be better to rebase the original commit onto a more recent version of the master branch and resolve conflicts as part of the rebase process, eliminating the merge commit. If you are uncomfortable doing that (or need more detailed instructions), please let us know, as a committer should be able to do it as well.

@aborkowski aborkowski force-pushed the aborkowski-fix-s_client-grammar branch from 1cb89d8 to 9d24f34 Compare December 31, 2019 17:15
@aborkowski aborkowski changed the title s_client.pod: Fix grammar in NOTES section. openssl-s_client.pod.in: Fix grammar in NOTES section. Dec 31, 2019
@aborkowski
Copy link
Contributor Author

@kaduk Thank you for the hint, my branch is now rebased onto current master.

mspncp
mspncp previously requested changes Dec 31, 2019
View reviewed changes
doc/man1/openssl-s_client.pod.in
@@ -781,7 +781,7 @@ server.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

kaduk
kaduk approved these changes Dec 31, 2019
View reviewed changes
doc/man1/openssl-s_client.pod.in
@@ -781,7 +781,7 @@ server.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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 mspncp dismissed their stale review January 1, 2020 09:00

Well, it's better than it was before, so I won't object to it being merged.

@iamamoose iamamoose added the approval: done This pull request has the required number of approvals label Aug 31, 2020
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Sep 1, 2020
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Sep 1, 2020
openssl-machine pushed a commit that referenced this pull request Sep 18, 2020
@aborkowski @slontis
s_client.pod: Fix grammar in NOTES section.
ecf15b1 
CLA: trivial

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #9421)
@slontis
Copy link
Member

slontis commented Sep 18, 2020

Thanks for your contribution.. The rewind on this took a while :). Merged to master..

@slontis slontis mentioned this pull request Sep 18, 2020
Closed
2 tasks
@slontis
Copy link
Member

slontis commented Sep 18, 2020

Closing this now that PR #12907 has been added for the 1_1_1 branch.

@slontis slontis closed this Sep 18, 2020
@slontis slontis removed the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@kroeckx kroeckx kroeckx approved these changes

@kaduk kaduk kaduk approved these changes

@mspncp mspncp mspncp left review comments

Assignees

@levitte levitte

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@aborkowski @mspncp @kaduk @openssl-machine @slontis @levitte @kroeckx @iamamoose