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

Agent does not validate KONTENA_URI=wss:// SSL certs #2500

Closed
SpComb opened this issue Jun 20, 2017 · 3 comments · Fixed by #2560
Closed

Agent does not validate KONTENA_URI=wss:// SSL certs #2500

SpComb opened this issue Jun 20, 2017 · 3 comments · Fixed by #2560
Labels
Milestone

Comments

@SpComb
Copy link
Contributor

SpComb commented Jun 20, 2017

The agent Kontena::WebsocketClient connects to the configured KONTENA_URI=wss://... using Faye::WebSocket::Client.new with the default options. The :tls option defaults to verify_peer: false.

The agent will immediately establish a websocket connection, sending the Kontena-Grid-Token header with the plaintext grid token secret. In the case of misconfiguration or active MITM attack, this could leak the grid secret.

The agent should support optional /etc/kontena-agent.env parameters for websocket client SSL cert verification, and the CLI plugins etc used for host node provisioning should be updated to configure the agent for strict SSL cert validation. For a master with a self-generated SSL cert, this would also need to deploy the cert to the agent, or possibly use some fingerprint-based mechanism.

@SpComb SpComb added the agent label Jun 20, 2017
@SpComb
Copy link
Contributor Author

SpComb commented Jul 4, 2017

It looks like Faye::Websocket::Client doesn't actually implement SSL cert validation... Passing the tls: { verify_peer: true} option will have EM request the server cert, but it will just get passed to EM::Connection#ssl_verify_peer... which Faye::Websocket::Client::Connection does not seem to implement?

Anyways, even if faye-websocket did implement it, the EM SSL cert validation design looks seriously flawed... verify_peer: true will call SSL_set_verify(..., SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE, ssl_verify_wrapper), but the ssl_verify_wrapper function just ignores the preverify_ok variable, and thus any cert validation errors (like the signature not matching the cert pubkey?! eventmachine/eventmachine#275).

A PR from 2012 to fix the EM ssl cert verification (eventmachine/eventmachine#378) to allow configuring the CA certs to use for validation, and passing the required cert-verify error/level information to the application-level callback... is still open, nearly 5 years later 😞

I don't think it's feasible to implement any kind of robust SSL cert validation for the agent WebsocketClient using the current Faye::Websocket + EventMachine stack...

@SpComb
Copy link
Contributor Author

SpComb commented Jul 4, 2017

Yes, passing Faye::WebSocket::Client.new(api_uri, nil, { tls: { verify_peer: true } }) just seems to fail to connect with EPROTO:

I, [2017-07-04T11:17:08.021304 #1]  INFO -- Kontena::WebsocketClient: connecting to master at wss://192.168.66.1:9293
D, [2017-07-04T11:17:08.122312 #1] DEBUG -- Kontena::WebsocketClient: Errno::EPROTO
E, [2017-07-04T11:17:08.122386 #1] ERROR -- Kontena::WebsocketClient: protocol error, check ws/wss: wss://192.168.66.1:9293
W, [2017-07-04T11:17:08.122443 #1]  WARN -- Kontena::WebsocketClient: connection closed with code 1006: 

I didn't trace this, but I suspect it's from the missing ssl_verify_peer callback, and the default nil value is just aborting the SSL handshake.

It looks like there is an open PR for SSL cert verification in faye-websocket, but I suspect that it's just deeply flawed: faye/faye-websocket-ruby#101

@SpComb
Copy link
Contributor Author

SpComb commented Jul 7, 2017

BTW these same issues also apply to the CLI websocket client used for the exec commands... because of how the OpenSSL::SSL::SSLContext defaults work and the CLI not explicitly setting verify_mode: OpenSSL::SSL::VERIFY_PEER, it doesn't actually verify the peer certs... it's also missing the post_connection_check to verify that the hostname matches the peer cert 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant