Add ability to send hostname for TLS SNI extension #2432

foxefj-cyber · 2021-12-03T20:06:29Z

Desired Outcome

Ability to define the SNI certificate hostname so the proper certificate is returned from the Kubernetes API Server.

Implemented Changes

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

Added change to send the hostname during SSL connection as well as adding testing.

Connected Issue/Story

CyberArk internal issue link: ONYX-14386

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

codeclimate · 2021-12-21T19:41:03Z

ci/authn-k8s/dev/utils.sh

+  if [ -z "$OPENSHIFT_TOKEN" ]; then
+    oc login $url --username=$OPENSHIFT_USERNAME --password=$OPENSHIFT_PASSWORD --insecure-skip-tls-verify=true
+  else
+    oc login $url --token=$OPENSHIFT_TOKEN --insecure-skip-tls-verify=true


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-21T19:41:04Z

ci/authn-k8s/dev/utils.sh

+  if [ -z "$OPENSHIFT_TOKEN" ]; then
+    oc login $url --username=$OPENSHIFT_USERNAME --password=$OPENSHIFT_PASSWORD --insecure-skip-tls-verify=true
+  else
+    oc login $url --token=$OPENSHIFT_TOKEN --insecure-skip-tls-verify=true


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-21T19:41:04Z

ci/authn-k8s/dev/utils.sh

+  fi
+
+  if [ -z "$OPENSHIFT_TOKEN" ]; then
+    oc login $url --username=$OPENSHIFT_USERNAME --password=$OPENSHIFT_PASSWORD --insecure-skip-tls-verify=true


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-21T19:41:04Z

ci/authn-k8s/dev/utils.sh

+  fi
+
+  if [ -z "$OPENSHIFT_TOKEN" ]; then
+    oc login $url --username=$OPENSHIFT_USERNAME --password=$OPENSHIFT_PASSWORD --insecure-skip-tls-verify=true


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-21T19:41:04Z

ci/authn-k8s/dev/utils.sh

+  fi
+
+  if [ -z "$OPENSHIFT_TOKEN" ]; then
+    oc login $url --username=$OPENSHIFT_USERNAME --password=$OPENSHIFT_PASSWORD --insecure-skip-tls-verify=true


Double quote to prevent globbing and word splitting.

jvanderhoof · 2021-12-22T16:49:40Z

app/domain/authentication/authn_k8s/k8s_object_lookup.rb

@@ -25,6 +25,13 @@ def initialize(webservice = nil)
        @cert_store = OpenSSL::X509::Store.new
        @cert_store.set_default_paths
        ::Conjur::CertUtils.add_chained_cert(@cert_store, ca_cert)
+
+        # allows us to add additional CA certs for things like SNI certs
+        if ENV['SSL_CERT_DIRECTORY'] and File.exists?(ENV['SSL_CERT_DIRECTORY']) and File.exists?(ENV['SSL_CERT_DIRECTORY'] + '/ca')


To resolve the Code Climate errors (and to make it easier to test), what do you think about refactoring this to something like:

return unless ENV.key?('SSL_CERT_DIRECTORY') load_additional_certs( ssl_cert_directory: ENV['SSL_CERT_DIRECTORY'], cert_store: @cert_store, cert_utils: ::Conjur::CertUtils )

We can then break the work into a method like:

def load_additional_certs(ssl_cert_directory:, cert_store:, cert_utils:) certificate_directory = "#{ssl_cert_directory}/ca" if Dir.exist?(certificate_directory) Dir.entries(certificate_directory).each do |file_name| if File.exist?(file_name) cert_utils.add_chained_cert(cert_store, File.read(file_name)) else # Log error message a particular file is missing end end else # Log error message that the environment variable SSL_CERT_DIRECTORY is missing end end

codeclimate · 2021-12-22T18:06:09Z

ci/authn-k8s/dev/utils.sh

+  if [ -z "$OPENSHIFT_TOKEN" ]; then
+    oc login $url --username="$OPENSHIFT_USERNAME" --password="$OPENSHIFT_PASSWORD" --insecure-skip-tls-verify=true
+  else
+    oc login $url --token="$OPENSHIFT_TOKEN" --insecure-skip-tls-verify=true


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-22T18:06:10Z

ci/authn-k8s/dev/utils.sh

+  url=$OPENSHIFT_URL
+
+  if [ -z "$OPENSHIFT_TOKEN" ]; then
+    oc login $url --username="$OPENSHIFT_USERNAME" --password="$OPENSHIFT_PASSWORD" --insecure-skip-tls-verify=true


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-22T18:06:10Z

ci/authn-k8s/test_oc_entrypoint.sh

+  filename=$1
+  api_server_ip=$2
+  api_fqdn=$3
+  sed -e "s#{{ CONJUR_AUTHN_K8S_TAG }}#$CONJUR_AUTHN_K8S_TAG#g" dev/$filename.${TEMPLATE_TAG}yaml |


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-22T18:06:10Z

ci/authn-k8s/test_oc_entrypoint.sh

+  filename=$1
+  api_server_ip=$2
+  api_fqdn=$3
+  sed -e "s#{{ CONJUR_AUTHN_K8S_TAG }}#$CONJUR_AUTHN_K8S_TAG#g" dev/$filename.${TEMPLATE_TAG}yaml |


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-22T18:06:10Z

ci/authn-k8s/test_oc_entrypoint.sh

+#  printLogs
+
+  conjur_pod=$(retrieve_pod conjur-authn-k8s)
+  api_server_ip=$(kubectl exec "$conjur_pod" -- sh -c 'echo $KUBERNETES_SERVICE_HOST')


Expressions don't expand in single quotes, use double quotes for that.

codeclimate · 2021-12-22T19:21:19Z

ci/authn-k8s/build_locally.sh

+sni_cert=$1
+
+if [[ ! -z "$sni_cert" ]]; then
+  sni_cert="$(realpath $1)"


Double quote to prevent globbing and word splitting.

codeclimate · 2021-12-22T19:21:19Z

app/domain/authentication/authn_k8s/k8s_object_lookup.rb

@@ -183,6 +187,7 @@ def k8s_clients
          KubeClientFactory.client(
            api: 'apis/apps.openshift.io', version: 'v1', host_url: api_url,
            options: options
+


Empty line detected around arguments.

codeclimate · 2021-12-22T19:21:19Z

CONTRIBUTING.md

+    - Download and install the RedHat Code Ready Container
+      - This contains all the necessary pieces to have a local version of
+        Openshift
+    - After install, copy down the kubeadmin username/password and update the


Inconsistent indentation for list items at the same level

codeclimate · 2021-12-22T19:21:19Z

CONTRIBUTING.md

+    - `OPENSHIFT_USERNAME` - username of an account that can create 
+      namespaces, adjust cluster properties, etc
+    - `OPENSHIFT_PASSWORD` - password of the account
+    - `OPENSHIFT_URL` - the URL of the RedHat CRC cluster


Inconsistent indentation for list items at the same level

codeclimate · 2021-12-22T19:21:19Z

CONTRIBUTING.md

+      namespaces, adjust cluster properties, etc
+    - `OPENSHIFT_PASSWORD` - password of the account
+    - `OPENSHIFT_URL` - the URL of the RedHat CRC cluster
+      - If running this locally - use `https://host.docker.internal:6443` so


Inconsistent indentation for list items at the same level

andytinkham

This work should have had a security review meeting before any changes were made. I have concerns about pulling in any cert in a directory. We'll need to meet to talk through some threat modeling. @foxefj-cyber - please book some time for us to talk through this.

jvanderhoof · 2022-01-10T15:42:13Z

@andytinkham, I know you're just making sure we're not accidentally introducing a security concern with this:

This work should have had a security review meeting before any changes were made.

Although it may have been helpful to have you in a meeting before @foxefj-cyber did any work, in reality, you'd need to be involved in most enhancement/bug fix conversation, which doesn't feel like the best use of your time. I do think certificate related work would certainly benefit from your insight (and seems to be the majority of our security related issues).

Moving forward, what do you think about flagging all work that touches certificates as requiring early security architect collaboration?

@andytinkham, instead of pulling all certificates from a folder, what do you think about specifying a comma (or other separator) delineated list of certificates? It would allow support for multiple certificate chains while ensuring only the specified certs were loaded.

Thoughts?

andytinkham · 2022-01-10T19:11:19Z

Although it may have been helpful to have you in a meeting before @foxefj-cyber did any work, in reality, you'd need to be involved in most enhancement/bug fix conversation, which doesn't feel like the best use of your time. I do think certificate related work would certainly benefit from your insight (and seems to be the majority of our security related issues).

Moving forward, what do you think about flagging all work that touches certificates as requiring early security architect collaboration?

For new features, we should be doing security review & threat modeling discussions by default all the time with the occasional path to exception being determining we don't need one (as opposed to the occasional path being to do it). For bug fixes, I'd say if it touches certificates, TLS, encryption, authentication, authorization, or direct handling of secrets values, let's have at least a quick chat at the beginning - we can determine if more is needed after that chat. My guess is most of the time that will be sufficient.

@andytinkham, instead of pulling all certificates from a folder, what do you think about specifying a comma (or other separator) delineated list of certificates? It would allow support for multiple certificate chains while ensuring only the specified certs were loaded.

Maybe. @foxefj-cyber & I will talk tomorrow. I'm still at the level of "Should I have concerns here?" not "I definitely have concerns here". Once I get more background, I can weigh in on this idea. Definitely seems worth talking about risks & tradeoffs between the 2 methods.

Thanks!

andytinkham

@foxefj-cyber @micahlee @jvanderhoof & I talked through things and I agree there is no further security issue here. Good to go from a security standpoint.

codeclimate · 2022-02-07T22:34:47Z

ci/authn-k8s/test.sh

+  then
+    docker rm temp_container || true
+    docker create --name temp_container "$image"
+    docker cp sni.crt "temp_container:/opt/conjur/etc/ssl/ca/${sni_cert##*/}"


sni_cert is referenced but not assigned.

codeclimate · 2022-02-07T22:34:47Z

ci/authn-k8s/test.sh

+           -servername "$SNI_FQDN" > sni.out < /dev/null
+
+    docker run --rm -i \
+           -w /home -v $PWD:/home \


Double quote to prevent globbing and word splitting.

codeclimate · 2022-02-07T22:34:47Z

ci/authn-k8s/test.sh

+  if [[ ! -z $SNI_FQDN ]]
+  then
+    docker run --rm -i \
+           -w /home -v $PWD:/home \


Double quote to prevent globbing and word splitting.

codeclimate · 2022-02-07T22:34:47Z

ci/authn-k8s/test_oc_entrypoint.sh

+
+  CONJUR_AUTHN_K8S_TAG=$(echo $CONJUR_AUTHN_K8S_TAG | sed "s/$OPENSHIFT_REGISTRY_URL/$OPENSHIFT_INTERNAL_REGISTRY_URL/")
+  CONJUR_TEST_AUTHN_K8S_TAG=$(echo $CONJUR_TEST_AUTHN_K8S_TAG | sed "s/$OPENSHIFT_REGISTRY_URL/$OPENSHIFT_INTERNAL_REGISTRY_URL/")
+  NGINX_TAG=$(echo $NGINX_TAG | sed "s/$OPENSHIFT_REGISTRY_URL/$OPENSHIFT_INTERNAL_REGISTRY_URL/")


Double quote to prevent globbing and word splitting.

codeclimate · 2022-02-07T22:34:47Z

ci/authn-k8s/test_oc_entrypoint.sh

+
+  CONJUR_AUTHN_K8S_TAG=$(echo $CONJUR_AUTHN_K8S_TAG | sed "s/$OPENSHIFT_REGISTRY_URL/$OPENSHIFT_INTERNAL_REGISTRY_URL/")
+  CONJUR_TEST_AUTHN_K8S_TAG=$(echo $CONJUR_TEST_AUTHN_K8S_TAG | sed "s/$OPENSHIFT_REGISTRY_URL/$OPENSHIFT_INTERNAL_REGISTRY_URL/")
+  NGINX_TAG=$(echo $NGINX_TAG | sed "s/$OPENSHIFT_REGISTRY_URL/$OPENSHIFT_INTERNAL_REGISTRY_URL/")


See if you can use ${variable//search/replace} instead.

codeclimate · 2022-02-09T18:24:52Z

CHANGELOG.md

+## [1.17.1] - 2022-02-09
+
+### Added
+- Added support for SNI certificates when talking to the Kubernetes API 


Trailing spaces

codeclimate · 2022-02-09T18:24:52Z

CHANGELOG.md

+## [1.17.1] - 2022-02-09
+
+### Added
+- Added support for SNI certificates when talking to the Kubernetes API 


Lists should be surrounded by blank lines

codeclimate · 2022-02-09T18:24:52Z

CONTRIBUTING.md

+The secrets file used for summons needs to contain the following environment
+variables
+
+  - openshift


Inconsistent indentation for list items at the same level

codeclimate · 2022-02-09T18:24:52Z

CONTRIBUTING.md

+variables
+
+  - openshift
+    - `OPENSHIFT_USERNAME` - username of an account that can create 


Inconsistent indentation for list items at the same level

codeclimate · 2022-02-09T18:24:52Z

CONTRIBUTING.md

+  - openshift
+    - `OPENSHIFT_USERNAME` - username of an account that can create 
+      namespaces, adjust cluster properties, etc
+    - `OPENSHIFT_PASSWORD` - password of the account


Inconsistent indentation for list items at the same level

codeclimate · 2022-02-09T18:24:55Z

Code Climate has analyzed commit ef11b52 and detected 51 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	40
Bug Risk	11

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

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

View more on Code Climate.

doodlesbykumbi

Hey @foxefj-cyber. This feedback is a bit late but I wanted to make sure it was covered at some point.

doodlesbykumbi · 2022-02-16T12:32:12Z

app/domain/authentication/authn_k8s/k8s_object_lookup.rb

+        return unless ENV.key?('SSL_CERT_DIRECTORY')
+
+        load_additional_certs(ENV['SSL_CERT_DIRECTORY'])


I might be missing something but is the singular call to add_chained_cert above not enough ? Wouldn't all the certs be stored in the Conjur variable or the cert file on disk when running inside a pod ?

Separately, I'm wondering about the documentation around the envvar SSL_CERT_DIRECTORY, I'm not familiar with it yet I see it being used in a few repos and without looking at the code here I wouldn't immediately know how it is used... and I'm not sure if its meaning in the context of the code here is shared across the different places it is used.

This directory is added in the Dockerfile under the conjur directory and I added pulling in the additional certs so we can add in the CA cert or the self signed cert for the SNI stuff.

doodlesbykumbi · 2022-02-16T12:47:56Z

app/domain/authentication/authn_k8s/web_socket_client.rb

+          if ssl_version != 'SSLv23'
+            @socket.hostname = options[:hostname] || uri.host
+          end
          @socket.connect
+          @socket.post_connection_check(@socket.hostname) if @socket.hostname


I would like to understand this better, since it's a bit more involved than my simple implementation of always setting the hostname.

Why are we only excluding this version ? Is it the case that SNI is only expected to be relevant on ssl_version > SSLv23 ?

Can you explain the logic around the post_connection_check

SNI is an extension in TLS only so we don't want to bother adding the hostname if they are using SSLv23 for some reason.

the post_connection_check will throw an error if we expected a certain SNI certificate but we didn't get it - this makes sure that if we are expecting SNI we get the proper certificate

On board with (1). For (2) I was sure I was able to get errors thrown with just calls to #connect, I'll reconfirm.

doodlesbykumbi · 2022-02-16T12:53:20Z

cucumber/kubernetes/features/sni.feature

+  @sni_fails
+  Scenario: Authenticate as a Pod.
+    When I authenticate with authn-k8s as "pod/inventory-pod" without cert and key
+    Then the HTTP status is "401"
+
+  @sni_success
+  Scenario: Authenticate as a Pod.
+    Given I can login to pod matching "app=inventory-pod" to authn-k8s as "*/*"
+    Then I can authenticate pod matching "pod/inventory-pod" with authn-k8s as "*/*"


I think the unit tests provide amazing validation and coverage for supporting SNI. I'm curious why we are adding a cucumber test which to me seems to be testing the same thing as the unit tests but paying a significantly higher price i.e. deploying a whole Conjur master for each one of these tests ?

these are just to validate that the entire solution works with SNI - purely for validation.

The Kubernetes API endpoint can include a path, as is the case with the Rancher API, the websocket implementation should take the URL as is instead of taking only host and port. The commit originally included support for server name indication (SNI) on the the websocket client, which enables the client to be specific about the host it is trying to reach in the first step of the TLS handshake, preventing common name mismatch errors. However, that commit was removed in favour of the more comprehensive work to support SNI at #2432.

foxefj-cyber self-assigned this Dec 3, 2021

foxefj-cyber force-pushed the ONYX-14386 branch from 9dcd1e1 to 9e2c83e Compare December 13, 2021 18:59

foxefj-cyber force-pushed the ONYX-14386 branch 3 times, most recently from 2a1dd5a to 8ca930c Compare December 21, 2021 19:15

foxefj-cyber marked this pull request as ready for review December 21, 2021 19:25

foxefj-cyber requested a review from a team as a code owner December 21, 2021 19:25

foxefj-cyber requested review from adamouamani, alexkalish and andytinkham December 21, 2021 19:25

codeclimate bot reviewed Dec 21, 2021

View reviewed changes

jvanderhoof reviewed Dec 22, 2021

View reviewed changes

foxefj-cyber force-pushed the ONYX-14386 branch from 8ca930c to 824b73f Compare December 22, 2021 17:41

codeclimate bot reviewed Dec 22, 2021

View reviewed changes

foxefj-cyber force-pushed the ONYX-14386 branch from 824b73f to 70e1ec7 Compare December 22, 2021 18:56

codeclimate bot reviewed Dec 22, 2021

View reviewed changes

andytinkham suggested changes Jan 5, 2022

View reviewed changes

andytinkham previously approved these changes Jan 11, 2022

View reviewed changes

foxefj-cyber dismissed andytinkham’s stale review via 7f65251 February 7, 2022 14:46

foxefj-cyber force-pushed the ONYX-14386 branch 2 times, most recently from 1bf4838 to b7fe347 Compare February 7, 2022 19:26

foxefj-cyber requested a review from a team February 7, 2022 19:27

foxefj-cyber force-pushed the ONYX-14386 branch from b7fe347 to ea71bc9 Compare February 7, 2022 19:49

foxefj-cyber marked this pull request as draft February 7, 2022 20:59

foxefj-cyber force-pushed the ONYX-14386 branch 3 times, most recently from a91588d to 8c9bdc4 Compare February 7, 2022 22:03

codeclimate bot reviewed Feb 7, 2022

View reviewed changes

Add ability to send hostname for TLS SNI extension

ef11b52

foxefj-cyber force-pushed the ONYX-14386 branch from 590e96d to ef11b52 Compare February 9, 2022 17:55

codeclimate bot reviewed Feb 9, 2022

View reviewed changes

micahlee approved these changes Feb 9, 2022

View reviewed changes

foxefj-cyber merged commit 05ddcb9 into master Feb 9, 2022

foxefj-cyber deleted the ONYX-14386 branch February 9, 2022 18:48

doodlesbykumbi reviewed Feb 16, 2022

View reviewed changes

alexkalish removed their request for review February 16, 2022 18:46

This was referenced Mar 3, 2022

Authn-k8s:websockets: api endpoints with path #2508

Closed

authn-k8s:websockets: support api endpoints with path #2479

Closed

Authn-k8s:websockets: support api endpoints with path #2509

Merged

doodlesbykumbi mentioned this pull request Mar 14, 2022

Fix: authn-k8s websocket client SNI #2516

Merged

13 tasks

jtuttle mentioned this pull request Jul 12, 2022

Update suite.yml for v1.17.6+suite.1 release cyberark/conjur-oss-suite-release#247

Merged

gl-johnson mentioned this pull request Aug 23, 2022

Update suite.yml for v18.0.0+suite1 release cyberark/conjur-oss-suite-release#250

Merged

		return unless ENV.key?('SSL_CERT_DIRECTORY')

		load_additional_certs(ENV['SSL_CERT_DIRECTORY'])

Add ability to send hostname for TLS SNI extension #2432

Add ability to send hostname for TLS SNI extension #2432

Conversation

foxefj-cyber commented Dec 3, 2021 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Dec 21, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 21, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 21, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 21, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

codeclimate bot Dec 22, 2021

Choose a reason for hiding this comment

andytinkham left a comment

Choose a reason for hiding this comment

jvanderhoof commented Jan 10, 2022 • edited Loading

andytinkham commented Jan 10, 2022

andytinkham left a comment

Choose a reason for hiding this comment

codeclimate bot Feb 7, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 7, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 7, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 7, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 7, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 9, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 9, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 9, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 9, 2022

Choose a reason for hiding this comment

codeclimate bot Feb 9, 2022

Choose a reason for hiding this comment

codeclimate bot commented Feb 9, 2022

doodlesbykumbi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxefj-cyber commented Dec 3, 2021 •

edited

Loading

jvanderhoof commented Jan 10, 2022 •

edited

Loading