Fix: authn-k8s websocket client SNI

[doodlesbykumbi]

Context

Link to initial SNI PR

I'm working with a Rancher deployment based off https://github.com/rancher/quickstart. My understanding is that the deployment uses Traefik (with SNI) to expose a public endpoint. In this case to connect to the Rancher API (for the purposes of consuming the Kubernetes API of an underlying cluster) requires a client that supports SNI. The Kubernetes authenticator in Conjur fails to connect to this Rancher API.

I did some digging and found out what was causing the issue. It appears that SNI is only applied when the ssl_version parameter (of Authentication::AuthnK8s::WebSocketClient) is set to something other than the default :SSLv23. The unit tests for Authentication::AuthnK8s::WebSocketClient validate this behavior by setting the value of the ssl_version parameter to :TLSv1 . However, in actual usage of Authentication::AuthnK8s::WebSocketClient within the Kubernetes authenticator controller the ssl_version parameter is never set, is therefore always set to the default and therefore Conjur at present has no SNI support.

This begs the question "what about the E2E tests?". The E2E tests are not actually validating anything at all, since they are passing even though the implementation is borked.

I couldn't make sense of why we have the ssl_version parameter at all if it never explicitly set in actual usage, or why it defaults to :SSLv23. I did some digging and landed on the understanding that we sort of just copied that from https://github.com/shokai/websocket-client-simple/blob/master/lib/websocket-client-simple/client.rb#L22-L31.

To better make sense of things, I looked to Ruby's standard library's Net::HTTP for a canonical implementation of establishing a TLS connection that supports SNI. See the code at https://github.com/ruby/net-http/blob/master/lib/net/http.rb#L982-L1084. This guided the changes I have made.

My intended changes are, though I might not get to all of them:

1. I removed the default ssl_version of :SSLv23. My understanding is that leaving the ssl_version parameter without a default results in the socket assuming the OpenSSL defaults specified at https://github.com/cyberark/conjur/blob/master/config/initializers/openssl.rb. We retain the ssl_version parameter to facilitate testing.
2. I do not apply SNI to IP addresses since you can not use IP addresses with SNI as per the spec
3. I updated unit tests to better reflected the usage, AND removed parameters where they are not needed. Unused parameters means creating unit tests for what might never be
4. Remove any loading of certs from ENV['SSL_CERT_DIRECTORY']?
   • I'm still not sure why this was added at all. I can understand if the idea here is that this directory is the shared location for the roots any Conjur-wide (e.g. in authenticators or anywhere elsewhere certs are needed) custom certs. This would be a static setup that requires restarting the Conjur server to be restarted to acknowledge updates. This would be separate from the ca_cert Conjur secret field which is configuration that is local to the Kubernetes authenticator. Though in the case of the Kubernetes authenticator the ca_cert field is a required field.
   • This envvar is undocumented as far as I can tell and I don't see usage of it anywhere else in the org
5. Fix the integration tests? For me the integration tests are a bit of a mystery right now.
   • I think the cucumber test for SNI failure (https://github.com/cyberark/conjur/blob/master/cucumber/authenticators_k8s/features/sni.feature#L5-L7) is testing a case that will always return a 401. It should test a case that would otherwise return a 200.
   • For development there are env vars $SNI_FQDN=sni-test.dev.conjur.net and $SNI_PORT=6443 , which are sort of magical and need explanation!. These are not defined in CI

Desired Outcome

Please describe the desired outcome for this PR. Said another way, what was
the original request that resulted in these code changes? Feel free to copy
this information from the connected issue.

Implemented Changes

Describe how the desired outcome above has been achieved with this PR. In
particular, consider:

• What's changed? Why were these changes made?
• How should the reviewer approach this PR, especially if manual tests are required?
• Are there relevant screenshots you can add to the PR description?

Connected Issue/Story

Resolves #[relevant GitHub issue(s), e.g. 76]

CyberArk internal issue link: insert issue ID

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

• The CHANGELOG has been updated, or
• This PR does not include user-facing changes and doesn't require a
  CHANGELOG update

Test coverage

• This PR includes new unit and integration tests to go with the code
  changes, or
• The changes in this PR do not require tests

Documentation

• Docs (e.g. READMEs) were updated in this PR
• A follow-up issue to update official docs has been filed here: insert issue ID
• This PR does not require updating any documentation

Behavior

• This PR changes product behavior and has been reviewed by a PO, or
• These changes are part of a larger initiative that will be reviewed later, or
• No behavior was changed with this PR

Security

• Security architect has reviewed the changes in this PR,
• These changes are part of a larger initiative with a separate security review, or
• There are no security aspects to these changes

[jtuttle]

These commits will be merged into master from a different branch shortly.

[codeclimate]

Code Climate has analyzed commit fd4b452 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 88.0% (-1.5% change).

View more on Code Climate.

[mFelgate]

lgtm