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

S server cmd #646

Closed
wants to merge 3 commits into from
Closed

S server cmd #646

wants to merge 3 commits into from

Conversation

ajmohan
Copy link
Contributor

@ajmohan ajmohan commented Feb 9, 2016

  • added missing help option messages
  • ecdh_single option is removed as it is a no-op and not an option
    supported in earlier versions
  • ssl_ctx_security_debug() was invoked before ctx check for NULL
  • trusted_first option can be removed, as it is always enabled in 1.1.
    But not removed the option, require confirmation.

{ "check_ss_sig", OPT_V_CHECK_SS_SIG, '-', \
"Enable checking of the root CA self signed certificate signature"}, \
{ "trusted_first", OPT_V_TRUSTED_FIRST, '-', \
"Use locally-trusted CA's first in building chain (enabled by default)" }, \
Copy link
Member

Choose a reason for hiding this comment

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

Has this changed? Last time I checked trusted_first was not on by default (although I know Viktor has been working in this area so perhaps that is no longer the case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

man verify(1) says this:

-trusted_first
When constructing the certificate chain, use the trusted certificates specified via -CAfile, -CApath or -trusted before any certificates specified
via -untrusted. This can be useful in environments with Bridge or Cross-Certified CAs. As of OpenSSL 1.1.0 this option is on by default and cannot
be disabled.

Hence I thought it is already implemented it that way. Not removed by self because I have not tested myself :)
If it is always enabled, probably the option can be removed. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Probably Viktor changed it. I would keep the option though for compatibility reasons (e.g. if scripts etc expect it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed.

@mattcaswell
Copy link
Member

General comment: we do not accept merge commits. Please can you squash all of this into a single commit?

@ajmohan
Copy link
Contributor Author

ajmohan commented Feb 9, 2016

@mattcaswell, I am trying to figure out how to squash into a single commit as you suggested, never done this before. Will commit after this change.
Thanks,

* added missing help option messages
* ecdh_single option is removed as it is a no-op and not an option
supported in earlier versions
* ssl_ctx_security_debug() was invoked before ctx check for NULL
* trusted_first option can be removed, as it is always enabled in 1.1.
But not removed the option, require confirmation.
@ajmohan
Copy link
Contributor Author

ajmohan commented Feb 9, 2016

@mattcaswell, squashed merge commits. Please have a look. Thanks.

@mattcaswell
Copy link
Member

Looks good.

+1

@richsalz
Copy link
Contributor

richsalz commented Feb 9, 2016

+1. merged at 32eabe3 thanks!

@richsalz richsalz closed this Feb 9, 2016
@ajmohan ajmohan deleted the s_server_cmd branch February 9, 2016 16:17
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.

None yet

3 participants