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

Add ability to send hostname for TLS SNI extension #2432

Merged
merged 1 commit into from
Feb 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Nothing should go in this section, please add to the latest unreleased version
(and update the corresponding date), or add a new version.

## [1.17.1] - 2022-02-09

### Added
- Added support for SNI certificates when talking to the Kubernetes API
Copy link

Choose a reason for hiding this comment

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

Trailing spaces

Copy link

Choose a reason for hiding this comment

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

Lists should be surrounded by blank lines

server through the web socket client.
[ONYX-14386](https://ca-il-jira.il.cyber-ark.com:8443/browse/ONYX-14386)

## [1.17.0] - 2022-02-09

### Changed
Expand Down
62 changes: 62 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,68 @@ Rake tasks are easy to run from within the `conjur` server container:
The next available error number is 63 ( CONJ00063E )
```

### Kubernetes specific Cucumber tests

Several cucumber tests are written to verify conjur works properly when
authenticating to Kubernetes. These tests have hooks to run against both
Openshift and Google GKE.

The cucumber tests are located under `cucumber/kubernetes/features` and can be
run by going into the `ci/authn-k8s` directory and running:

```shell
$ summon -f [secrets.ocp.yml|secrets.yml] ./init_k8s.sh [openshift|gke]
$ summon -f [secrets.ocp.yml|secrets.yml] ./test.sh [openshift|gke]
```

- `init_k8s.sh` - executes a simple login to Openshift or GKE to verify
credentials as well as logging into the Docker Registry defined
- `test.sh` - executes the tests against the defined platform

#### Secrets file

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

- openshift
Copy link

Choose a reason for hiding this comment

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

Inconsistent indentation for list items at the same level

- `OPENSHIFT_USERNAME` - username of an account that can create
Copy link

Choose a reason for hiding this comment

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

Inconsistent indentation for list items at the same level

namespaces, adjust cluster properties, etc
- `OPENSHIFT_PASSWORD` - password of the account
Copy link

Choose a reason for hiding this comment

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

Inconsistent indentation for list items at the same level

- `OPENSHIFT_URL` - the URL of the RedHat CRC cluster
Copy link

Choose a reason for hiding this comment

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

Inconsistent indentation for list items at the same level

- If running this locally - use `https://host.docker.internal:6443` so
Copy link

Choose a reason for hiding this comment

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

Inconsistent indentation for list items at the same level

the docker container can talk to the CRC containers
- `OPENSHIFT_TOKEN` - the login token of the above username/password
- only needed for local execution because the docker container
executing the commands can't redirect for login
- obtained by running the following command locally after login -
`oc whoami -t`
- gke
- `GCLOUD_CLUSTER_NAME` - cluster name of the GKE environment in the cloud
- `GCLOUD_ZONE` - zone of the GKE environment in the cloud
- `GCLOUD_PROJECT_NAME` - project name of the GKE environment
- `GCLOUD_SERVICE_KEY` - service key of the GKE environment

#### Local Execution Prerequisites

To execute the tests locally, a few things will have to be done:

- Openshift
- 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
Copy link

Choose a reason for hiding this comment

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

Inconsistent indentation for list items at the same level

secrets.ocp.yml file with the password
- Execute `oc whoami -t` and update the token property
- GKE
- Work with infrastructure to obtain a GKE environment

If the local revision of your files don't have a docker image built yet - build
the docker images using the following command:

```shell
$ ./build_locally.sh <sni cert file>
```

## Pull Request Workflow

1. [Fork the project](https://help.github.com/en/github/getting-started-with-github/fork-a-repo)
Expand Down
15 changes: 15 additions & 0 deletions app/domain/authentication/authn_k8s/k8s_object_lookup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ def initialize(webservice = nil)
@cert_store = OpenSSL::X509::Store.new
@cert_store.set_default_paths
::Conjur::CertUtils.add_chained_cert(@cert_store, ca_cert)

return unless ENV.key?('SSL_CERT_DIRECTORY')

load_additional_certs(ENV['SSL_CERT_DIRECTORY'])
Comment on lines +29 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

end

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

Copy link

Choose a reason for hiding this comment

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

Empty line detected around arguments.

)
]
end
Expand All @@ -196,6 +201,16 @@ def handle_object_not_found &block
raise unless $!.error_code == 404
end
end

def load_additional_certs(ssl_cert_directory)
# allows us to add additional CA certs for things like SNI certs
if Dir.exist?("#{ssl_cert_directory}/ca")
Dir["#{ssl_cert_directory}/ca/*"].each do |file_name|
::Conjur::CertUtils.add_chained_cert(@cert_store, File.read(file_name)) if
File.exist?(file_name)
end
end
end
end
end
end
8 changes: 7 additions & 1 deletion app/domain/authentication/authn_k8s/web_socket_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,22 @@ def connect(url, options = {})
uri.port || (uri.scheme == 'wss' ? 443 : 80))
if %w[https wss].include?(uri.scheme)
ctx = OpenSSL::SSL::SSLContext.new
ctx.ssl_version = options[:ssl_version] || 'SSLv23'
ssl_version = options[:ssl_version] || 'SSLv23'
ctx.ssl_version = ssl_version
ctx.verify_mode = options[:verify_mode] || OpenSSL::SSL::VERIFY_NONE # use VERIFY_PEER for verification
cert_store = options[:cert_store]
unless cert_store
cert_store = OpenSSL::X509::Store.new
cert_store.set_default_paths
end
ctx.cert_store = cert_store

@socket = ::OpenSSL::SSL::SSLSocket.new(@socket, ctx)
if ssl_version != 'SSLv23'
@socket.hostname = options[:hostname] || uri.host
end
@socket.connect
@socket.post_connection_check(@socket.hostname) if @socket.hostname
Comment on lines +40 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. Why are we only excluding this version ? Is it the case that SNI is only expected to be relevant on ssl_version > SSLv23 ?
  2. Can you explain the logic around the post_connection_check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

end
@handshake = ::WebSocket::Handshake::Client.new(url: url, headers: options[:headers])
@handshaked = false
Expand Down
4 changes: 3 additions & 1 deletion ci/authn-k8s/.gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
dev/interventions/*.gke.yml
dev/tls/nginx.*
dev/tls/*.key
dev/tls/*.crt
nginx.crt
/output/
dev/**/*.openshift.yml
30 changes: 30 additions & 0 deletions ci/authn-k8s/build_locally.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#!/bin/bash

sni_cert=$1

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

Choose a reason for hiding this comment

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

Double quote to prevent globbing and word splitting.

fi

function copy_cert() {
image=$1
sni_cert=$2

if [[ ! -z $sni_cert ]]; then
docker rm temp_container || true
docker create --name temp_container "$image"
docker cp "$sni_cert" "temp_container:/opt/conjur/etc/ssl/ca/${sni_cert##*/}"
docker commit temp_container "$image"
docker rm temp_container
fi
}

cd "$(git rev-parse --show-toplevel)" || exit

TAG="$(git rev-parse --short=8 HEAD)"
export TAG="$TAG"
docker build --no-cache -t "conjur:$TAG" .
copy_cert "conjur:$TAG" "$sni_cert"
docker build --no-cache -t "registry.tld/conjur:$TAG" .
copy_cert "registry.tld/conjur:$TAG" "$sni_cert"
docker build --no-cache --build-arg "VERSION=$TAG" -t "registry.tld/conjur-test:$TAG" -f Dockerfile.test .
8 changes: 4 additions & 4 deletions ci/authn-k8s/dev/dev_conjur.template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ spec:
spec:
containers:
- image: {{ CONJUR_AUTHN_K8S_TAG }}
imagePullPolicy: Always
imagePullPolicy: IfNotPresent
name: conjur
command: ["conjurctl", "server"]
env:
Expand Down Expand Up @@ -135,7 +135,7 @@ spec:
spec:
containers:
- image: {{ CONJUR_TEST_AUTHN_K8S_TAG }}
imagePullPolicy: Always
imagePullPolicy: IfNotPresent
name: conjur
command: ["sleep", "infinity"]
env:
Expand Down Expand Up @@ -200,5 +200,5 @@ spec:
spec:
containers:
- name: nginx
image: {{NGINX_TAG}}
imagePullPolicy: Always
image: {{ NGINX_TAG }}
imagePullPolicy: IfNotPresent
60 changes: 60 additions & 0 deletions ci/authn-k8s/dev/dev_conjur_sni.template.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: conjur-authn-k8s
labels:
app: conjur-authn-k8s
spec:
replicas: 1
selector:
matchLabels:
app: conjur-authn-k8s
template:
metadata:
labels:
app: conjur-authn-k8s
spec:
hostAliases:
- ip: {{ KUBERNETES_SERVICE_HOST }}
hostnames:
- "{{ KUBERNETES_API_FQDN }}"
containers:
- image: {{ CONJUR_AUTHN_K8S_TAG }}
imagePullPolicy: Always
name: conjur
command: ["conjurctl", "server"]
env:
- name: DATABASE_URL
value: postgres://postgres@postgres:5432/postgres
- name: CONJUR_ADMIN_PASSWORD
value: admin
- name: CONJUR_ACCOUNT
value: cucumber
- name: CONJUR_DATA_KEY
value: "{{ DATA_KEY }}"
- name: RAILS_ENV
value: test
# Enable coverage tracking.
- name: REQUIRE_SIMPLECOV
value: "true"
# Sleep after generating the coverage report to keep the pod alive
# so the report can be retrieved.
- name: SIMPLECOV_SLEEP
value: "true"
- name: WEB_CONCURRENCY
value: "0"
- name: RAILS_MAX_THREADS
value: "10"
- name: CONJUR_AUTHENTICATORS
value: authn-k8s/minikube
- name: KUBERNETES_SERVICE_HOST
value: {{ KUBERNETES_API_FQDN }}
- name: SSL_CERT_DIRECTORY
value: /opt/conjur/etc/ssl/
volumeMounts:
- mountPath: /run/authn-local
name: authn-local
volumes:
- name: authn-local
emptyDir:
medium: Memory
4 changes: 2 additions & 2 deletions ci/authn-k8s/dev/dev_inventory.template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ spec:
app: inventory-deployment
spec:
containers:
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
name: inventory
command: ["sleep", "infinity"]
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
name: authenticator
command: ["sleep", "infinity"]
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,17 @@ metadata:
name: inventory-deployment-cfg
spec:
replicas: 1
selector:
matchLabels:
app: inventory-deployment-cfg
selector:
app: inventory-deployment-cfg
template:
metadata:
labels:
app: inventory-deployment-cfg
spec:
containers:
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
name: inventory
command: ["sleep", "infinity"]
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
name: authenticator
command: ["sleep", "infinity"]
6 changes: 3 additions & 3 deletions ci/authn-k8s/dev/dev_inventory_example.template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
name: inventory-pod-in-namespace
spec:
containers:
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
imagePullPolicy: IfNotPresent
name: inventory
env:
Expand All @@ -22,7 +22,7 @@ spec:
- mountPath: /run/conjur
name: conjur-access-token
readOnly: true
- image: {{AUTHENTICATOR_TAG}}
- image: {{ AUTHENTICATOR_TAG }}
imagePullPolicy: IfNotPresent
name: authenticator
env:
Expand All @@ -45,7 +45,7 @@ spec:
- name: CONJUR_ACCOUNT
value: cucumber
- name: CONJUR_AUTHN_LOGIN
value: {{CONJUR_AUTHN_K8S_TEST_NAMESPACE}}/*/*
value: {{ CONJUR_AUTHN_K8S_TEST_NAMESPACE }}/*/*
- name: CONJUR_SSL_CERTIFICATE
valueFrom:
configMapKeyRef:
Expand Down
8 changes: 4 additions & 4 deletions ci/authn-k8s/dev/dev_inventory_pod.template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ metadata:
spec:
serviceAccountName: inventory-pod-only
containers:
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
imagePullPolicy: IfNotPresent
name: inventory
command: ["sleep", "infinity"]
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
imagePullPolicy: IfNotPresent
name: authenticator
command: ["sleep", "infinity"]
Expand All @@ -40,11 +40,11 @@ metadata:
spec:
serviceAccountName: inventory-pod-only
containers:
- image: {{INVENTORY_BASE_TAG}}
- image: {{ INVENTORY_BASE_TAG }}
imagePullPolicy: IfNotPresent
name: inventory
command: ["sleep", "infinity"]
- image: {{INVENTORY_BASE_TAG}}
- image: {{ INVENTORY_BASE_TAG }}
imagePullPolicy: IfNotPresent
name: authenticator
command: ["sleep", "infinity"]
4 changes: 2 additions & 2 deletions ci/authn-k8s/dev/dev_inventory_stateful.template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ spec:
app: inventory-stateful
spec:
containers:
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
name: inventory
command: ["sleep", "infinity"]
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
name: authenticator
command: ["sleep", "infinity"]
4 changes: 2 additions & 2 deletions ci/authn-k8s/dev/dev_inventory_unauthorized.template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ metadata:
name: inventory-unauthorized
spec:
containers:
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
name: inventory
command: ["sleep", "infinity"]
- image: {{INVENTORY_TAG}}
- image: {{ INVENTORY_TAG }}
name: authenticator
command: ["sleep", "infinity"]
Loading