Skip to content

Support fail_if_no_peer_cert ssl option#231

Closed
KlausTrainer wants to merge 1 commit intoapache:masterfrom
KlausTrainer:fail_if_no_peer_cert
Closed

Support fail_if_no_peer_cert ssl option#231
KlausTrainer wants to merge 1 commit intoapache:masterfrom
KlausTrainer:fail_if_no_peer_cert

Conversation

@KlausTrainer
Copy link
Contributor

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

How about require_peer_cert instead? Sounds better, imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to stick with fail_if_no_peer_cert, which is also the name of the ssl application option. The "require"-part would be misleading in my opinion, as it is ignored if verify_ssl_certificates = false.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I saw that fail_if_no_peer_cet is also used by ssl app, but I was driven by the idea that config options should be more user friendly (no matter how they are related with internals) (whenever it's possible). Fail word sounds scary: no one wanted to let their server (note, that there is no mention about connection - so we assume the worst) to fail because of something, so in fact this option name generates negative emotions and will mostly remains untouched. The require_peer_cert sounds more..."secure", since we don't fail, but we raising requirements for our clients - that's more solid and user friendly.

Anyway, so my loud thoughts (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're discussing here about an advanced feature most people will likely never need. If that option will mostly remain untouched that's good. And if the "fail" part sounds a bit scary, it will more likely stop people who don't know what they're doing from making their server unreachable. If people want to use that feature, they should really know what they're doing, and in that case the "fail"-part won't stop them from using that feature :)

Copy link
Member

Choose a reason for hiding this comment

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

I understand the position "RTFM First", but that's the actual problem of all configurations: you're reading the config, you're don't understand what the options actually does, you're going to google (nobody reads the docs) to find the answer or to ML to ask the same question again. Name should reflect the behaviour clearly. "Fail" is good for Erlang since "Let it crash!", but CouchDB wouldn't actually "fail" because of empty client certificate: it will just reject such requests and keep going. So actually there are two names that are suitable for configuration: require_peer_cert and reject_if_no_peer_cert. Last one too verbose.

I understand the reasons why you picked such name, but I feel that it could be better. Or at least comment should clarify what the "fail" actually stands for. Probably, we could ask others devs opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 05/13/2014 06:00 PM, Alexander Shorin wrote:

I understand the position "RTFM First", but that's the actual problem
of all configurations: you're reading the config, you're don't
understand what the options actually does, you're going to google
(nobody reads the docs) to find the answer or to ML to ask the same
question again.

I agree, we should definitely try to give good explanations in the
documentation. Creating high-quality documentation is one way to get
people to actually read the documentation ;)

Name should reflect the behaviour clearly. "Fail" is good for Erlang
since "Let it crash!", […]

You're mixing up different things here. This is not about the Erlang
"let it crash" philosophy. Are we talking here about the ssl
configuration section, and the corresponding section in the
documentation which is captioned "Secure Socket Level Options"? Isn't
it clear from the context that this is not about Erlang/OTP principles?

I updated the documentation to make it more clear that this is about the
TLS/SSL handshake. The purpose of such a handshake is to establish a
TLS/SSL session. If the server decides that it doesn't want to
establish a session because the client does not send a certificate, it
will send a "handshake failure" alert message and close the connection,
which means that the handshake "failed". Using the term "fail" in that
context is not something made up by me or by the people who created the
ssl application with that very "fail_if_no_peer_cert" option, it's the
technical terminology that has been introduced in the respective RFCs,
and which is actually used by the people who are talking about that
stuff.

[…] but CouchDB wouldn't actually "fail" because of empty client
certificate: it will just reject such requests and keep going.

I agree that this makes a difference, at least for us. However, end
users usually don't care if the server is actually running or not when
their browser can't establish a connection it, as their experience is
exactly the same: the server is "down"; something "failed", apparently.

I understand the reasons why you picked such name, but I feel that it
could be better. Or at least comment should clarify what the "fail"
actually stands for.

I checked again and updated the section in the documentation to make it
more clear what the configuration does.

Copy link
Member

Choose a reason for hiding this comment

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

Using the term "fail" in that context is not something made up by me or by the people who created the ssl application with that very "fail_if_no_peer_cert" option, it's the technical terminology that has been introduced in the respective RFCs, and which is actually used by the people who are talking about that stuff.

That's the key phrase! +1 for fail_if_no_peer_cert. I have no more questions about and you made this name explained for others who will be turned on by the same questions like I did. Thank you! (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review!

@dch
Copy link
Contributor

dch commented May 14, 2014

LGTM, +1 for mergeship.

@KlausTrainer KlausTrainer deleted the fail_if_no_peer_cert branch May 14, 2014 14:38
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