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

Setting verify_mode to NONE results in FORCE_PEER in SslSimpleBuilder.java #196

Open
nick-george opened this issue Feb 25, 2017 · 12 comments

Comments

@nick-george
Copy link

Hi,

If the user has set verify_mode to NONE in the logstash beats input configuration, it is overridden by the default value set by the code in SslSimpleBuilder.java (https://github.com/logstash-plugins/logstash-input-beats/blob/master/src/main/java/org/logstash/netty/SslSimpleBuilder.java), see below:

SslClientVerifyMode verifyMode = SslClientVerifyMode.FORCE_PEER;

This is because the Java code has no concept of NONE as the ClientVerifyMode.

Due to various other limitations in Logstash, I am having to pass in a non-empty array of certificate authorities. Because of this, requireClientAuth() will always return true; which in turn, actually sets the verify mode to FORCE_PEER - even though I set it to NONE in the filter configuration. As a result of this, all SSL connections fail.

Could you please fix this behaviour? I am happy to write a pull-request, but have been unable to run your tests successfully in the past.

Nick

@Sakorah
Copy link

Sakorah commented Oct 23, 2017

Hi,

I'm running into the same issue here.
LS: 5.6.3
Filebeat 5.6.3

I set the beats input to:

beats {  
  port => "8214"  
  host => "10.37.129.203"  
  tags => "filebeat"  
  ssl => true  
  ssl_certificate => "/etc/logstash/certs/minion-elk01.shared-01-cert.pem"  
  ssl_key => "/etc/logstash/certs/minion-elk01.shared-02-key.pkcs8"  
  ssl_certificate_authorities => "/etc/logstash/certs/minion-elk01.shared-03-ca.pem" 
  ssl_verify_mode => "none"  
  tls_min_version => 1.2  
 }

And the Beat config to:

output:
  logstash:
    hosts: ["elk-vip01.shared:8214", "elk-vip02.shared:8214"]
    loadbalance: true
    ssl.certificate_authorities: ["/etc/logstash/certs/minion-elk01.shared-03-ca.pem"]
    ssl.supported_protocols: [TLSv1.2]
    ssl.verification_mode: full

If I give filebeat a client certificate it works, without a certificate the handshake fails although LS should not even ask the client for a certificate.

@nick-george
Copy link
Author

Ha! What are the odds of that, I was looking at this ticket only a few hours ago for the first time in months, hoping that there had been some movement on it. I was curious if anyone else has run into the same issue.
I've posted this issue and a pull request (#182), albeit without any tests.

The current behaviour of the beats input is at odds with the documentation, which clearly states that no client certificate checking is done if verify_mode is set to none.

I think the rationale behind any lack of action on this is that we are expected to use configuration management (or through hand crafting configs) to just remove the ssl_certificate_authorities line. Unfortunately for me, I store all my logstash config in a stand-alone GIT repository, and it's currently not possible to support both cases of using a client cert and not using a client cert with the same code. Part of the reason for this is because logstash cannot have conditionals in INPUTS, which is a real shame.

Have you tried just removing the "ssl_certificate_authorities" line from your config? That should fix the problem no? (or are you in the same boat as me?)

Cheers,
Nick

@Sakorah
Copy link

Sakorah commented Oct 23, 2017

Hi!

I can remove the "ssl_certificate_authorities" from my input config as I don't need to support both scenarios. I just was not aware that removing it also disables client certificate authentication... this is misleading in the documentation.

Sako

@nick-george
Copy link
Author

Damn, I was hoping there was more than one of us with this issue! The docs are currently incorrect, also a fix would involve only changing 1-2 lines of code.

@jordansissel
Copy link
Contributor

I can remove the "ssl_certificate_authorities" from my input config as I don't need to support both scenarios. I just was not aware that removing it also disables client certificate authentication...

How would clients be authenticated without a way to authenticate them (with trusted certificate authorities)

@Sakorah
Copy link

Sakorah commented Oct 24, 2017

I thought more about that the CA might be advertised to the client. That's how I know it from webservers so the client knows what certificate to present if they have more than one :) but given the configuration and situation here its clear that the client will only offer one and that this has to match against the serverside CAs.

Or that the complete certificate chain will be sent in the SSL handshake - also that how werbservers and loadbalancers do it.

@jordansissel
Copy link
Contributor

I appreciate all the feedback and details on this issue. I agree the configuration is confusing right now, and we will take steps to improve the docs as well as the behavior.

@nick-george
Copy link
Author

nick-george commented Oct 25, 2017

Thanks Jordan,

The docs are currently incorrect because of the following (I have copied some of the docs below)

ssl_certificate_authorities:
Validate client certificates against these authorities. You can define multiple files or paths. All the certificates will be read and added to the trust store. You need to configure the ssl_verify_mode to peer or force_peer to enable the verification. <- This is incorrect because the moment you provide the ssl_certificate_authorities option, the plugin ignores you if you set ssl_verify_mode to none, and defaults to force_peer.

ssl_verify_mode:
default value is none
By default the server doesn’t do any client verification. <- Not if ssl_certificate_authorities is provided

This option needs to be used with ssl_certificate_authorities and a defined list of CAs.

It would be fantastic if any of the following were implemented:

  • Inputs in LS configuration could use conditional logic
  • If it were possible to pass in a nil value via environment variable expansion, or at least fall-back to nil if the variable is absent.
  • If the beats input LS plugin were modified so that if the user sets ssl_verify_mode to none, then it really does not enforce the use of client certificates.

Many thanks!
Nick

@jstoja
Copy link

jstoja commented Dec 5, 2017

Hello @nick-george I'm also getting errors but reading this https://github.com/logstash-plugins/logstash-input-beats/blob/master/src/main/java/org/logstash/netty/SslSimpleBuilder.java#L138 I assume that specifying the ssl_certificate_authorities force the handshake no matter the mode. So your PR is not solving the issues (I think).

@jstoja
Copy link

jstoja commented Dec 5, 2017

@jordansissel Any news on the update of the configuration? I still can see that none is the default no matter what in the 6.0.0 of logstash documentation.

@nick-george
Copy link
Author

Hi @jstoja,

I have just hand re-tested my change and can confirm it works.

I assume that specifying the ssl_certificate_authorities force the handshake no matter the mode

This is correct for the current behaviour. What my PR does is allow the user to override this behaviour with the ssl_verify_mode being set to "none". If it is set to "none", the list of cert authorities is never added, which results in the desired behaviour.

Cheers,
Nick

@jstoja
Copy link

jstoja commented Jan 9, 2018

@nick-george Thank you very much for the answer! I finally got what the PR goal was and fixed my issues.

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

No branches or pull requests

4 participants