Skip to content

[v4.y] Load cluster ca certificates using OpenSSL::X509::Store#add_file #552

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

Merged
merged 5 commits into from
Mar 23, 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
1 change: 1 addition & 0 deletions .github/workflows/actions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ on:
- '**'
jobs:
build:
continue-on-error: true
runs-on: ${{ matrix.os_and_command.os }}
strategy:
matrix:
Expand Down
15 changes: 15 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,21 @@ Notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
Kubeclient release versioning follows [SemVer](https://semver.org/).

## Unreleased 4.9.z

### Fixed

- `Config`: fixed parsing of `certificate-authority` file containing concatenation of
several certificates. Previously, server's cert was checked against only first CA cert,
resulting in possible "certificate verify failed" errors.

An important use case is a chain of root & intermediate cert(s) - necessary when cluster's CA
itself is signed by another custom CA.
But also helps when you simply concatenate independent certs. (#461, #552)

- Still broken (#460): inline `certificate-authority-data` is still parsed using `add_cert`
method that handles only one cert.

## 4.9.2 — 2021-05-30

### Added
Expand Down
16 changes: 10 additions & 6 deletions lib/kubeclient/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,15 @@ def context(context_name = nil)
user['exec_result'] = ExecCredentials.run(exec_opts)
end

ca_cert_data = fetch_cluster_ca_data(cluster)
client_cert_data = fetch_user_cert_data(user)
client_key_data = fetch_user_key_data(user)
auth_options = fetch_user_auth_options(user)

ssl_options = {}

if !ca_cert_data.nil?
if cluster_ca_data?(cluster)
cert_store = OpenSSL::X509::Store.new
cert_store.add_cert(OpenSSL::X509::Certificate.new(ca_cert_data))
populate_cert_store_from_cluster_ca_data(cluster, cert_store)
ssl_options[:verify_ssl] = OpenSSL::SSL::VERIFY_PEER
ssl_options[:cert_store] = cert_store
else
Expand Down Expand Up @@ -131,11 +130,16 @@ def fetch_context(context_name)
[cluster, user, namespace]
end

def fetch_cluster_ca_data(cluster)
def cluster_ca_data?(cluster)
cluster.key?('certificate-authority') || cluster.key?('certificate-authority-data')
end

def populate_cert_store_from_cluster_ca_data(cluster, cert_store)
if cluster.key?('certificate-authority')
File.read(ext_file_path(cluster['certificate-authority']))
cert_store.add_file(ext_file_path(cluster['certificate-authority']))
elsif cluster.key?('certificate-authority-data')
Base64.decode64(cluster['certificate-authority-data'])
ca_cert_data = Base64.decode64(cluster['certificate-authority-data'])
cert_store.add_cert(OpenSSL::X509::Certificate.new(ca_cert_data))
Copy link
Collaborator Author

@cben cben Mar 22, 2022

Choose a reason for hiding this comment

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

Umm, looks like the fix only covered certificate-authority, but certificate-authority-data would still have the bug.

  • need to add test with concatenated inline data.
  • can we fix it?
  • if not, correct CHANGELOG to explain partial fix.

end
end

Expand Down
19 changes: 19 additions & 0 deletions test/config/another-ca1.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-----BEGIN CERTIFICATE-----
MIIDADCCAeigAwIBAgIUQZjM/5qoAF78qIDyc+rKi4qBdOIwDQYJKoZIhvcNAQEL
BQAwGDEWMBQGA1UEAxMNa3ViZXJuZXRlcy1jYTAeFw0yMjAzMjIxNDQzMDBaFw0z
MjAzMTkxNDQzMDBaMBgxFjAUBgNVBAMTDWt1YmVybmV0ZXMtY2EwggEiMA0GCSqG
SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDGkG7g+UjpDhZ7A4Pm7Hme+RWs5IHz4I2X
IclvtO3LuJ26yzz2S8VaXFFeUqzEPb2G1RxFGvoAVN7qrTw0n5MQJCFLAA4dI7oY
8XLRJ7KgTBBIw1jYpgKb2zyHPIJE6VmslliKUiX+QDovdRU/dsbdup2EucrnGw4+
QNNAc3XMbXgm6lubA6znYZlSpcQ8BKer3tq75q4KUZicIjS6gKQyZjk9a6fcOuCS
ybtlAKp9lYzcwxZkNrx+V1PJMQ1qaJWPnMAVi7Oj5Dm3Jmf1WHBcNEh52Q/0vYlt
4WSaeM5t/Py/m/7c4Ve97f5m2X6EhYyUbzov4qeZOnIJI3MnU1FxAgMBAAGjQjBA
MA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSl1qyt
jd96WstRE8h9x5qkCvZUvjANBgkqhkiG9w0BAQsFAAOCAQEAJt55qYvBaniAwvgO
tbO79g1FcQGrxpMX45TuoCE/K+MWDjrr6bp+FbLOqT8MwOsbGwwJIRTHGvkEkVso
5AWI5aSNs3hWnltOdz27ZSHeX77WB4daK1tLK6ggZrp3v9iIpbBwWBFdmAqsPvEs
H17K2BgAzdh6xRKPQd0BGTUpJBfk50R2gDMj7FKyIzBN69IOGytBfAXBhHzEGy4+
MvtTEIMUjR//KgCrpNeyDuaWHttR5FdnuRxFO7O3BAfyNSaNmd/IEHQf7DIGgzOy
+xWLyH/HRHj5C70qAqjbnrgBODI99BsA9U7oXTuyPLdIboAcFt2zD5DIYgZET52X
53w4jA==
-----END CERTIFICATE-----
19 changes: 19 additions & 0 deletions test/config/another-ca2.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-----BEGIN CERTIFICATE-----
MIIDADCCAeigAwIBAgIUHW3OPnmuTquJ0YgbGpmm/blsY2QwDQYJKoZIhvcNAQEL
BQAwGDEWMBQGA1UEAxMNa3ViZXJuZXRlcy1jYTAeFw0yMjAzMjIxNDQ0MDBaFw0z
MjAzMTkxNDQ0MDBaMBgxFjAUBgNVBAMTDWt1YmVybmV0ZXMtY2EwggEiMA0GCSqG
SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDLMEJs5agS0hNQBxPTtsI6dIhIi/pY8liI
sNukbi5KwKf80FYNyRXqE8ufDVyTFzOc+MG96jnHjDaBWjrVN9On0PgUBo4nPyd4
DtyvYx2jMzwToSEIo/Z1aroMx1oGywCgdS4/3FWAbhlSbyXKJmhfh6gX0TxWz+dV
zqNuqQq9EWuRhOMg9vgzjfp3mjiPE10lW8pT0j5JT3PI/eGO+C2Z7z33LJXb6GM2
nXvhGFMGY+7XG65pqJ3L8g1mk+LjPiwyIItw8wPtrnrZ2VXMklMd5Mn+jgCTNe1B
om0nPpPIiTblCr6gcNcVjy5WGN37OKlqrT0JTuSPHcxSUp05LFjDAgMBAAGjQjBA
MA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBQvV/sB
wbR3UwjkLAMN+6P3fZ/3OjANBgkqhkiG9w0BAQsFAAOCAQEACAk4EQwCkw2EBsSR
2SKoa1SjYFkZzIr/0/TB2YcMUvHF+RpvlD5vQ8/RJjeAl1kc6/niZ9TWCemjBLqI
hPoFe49zr49DyQjC2ZfsXVJvFCr6g7o4q4DtQ6ltyBuTJbkn1hI+aB8zgvpofG44
mKj18Y7tPvgXtRua4SaeBq777+22AOvKxPied9p4PTrMN4RKTP6+yIbLflej7dBD
zQDjfmmYsH0T2ZRtBpE1dYrUbU3tkizcMZRJBgreoxoff+r5coibMIm/7gh+YoSb
BCItCaeuGSKQ8CJb8DElcPUd6nKUjmeiQL68ztsG/+CXLiL/TZb914VaaCXvPInw
49jJ7w==
-----END CERTIFICATE-----
20 changes: 20 additions & 0 deletions test/config/concatenated-ca.kubeconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
apiVersion: v1
clusters:
- cluster:
certificate-authority: concatenated-ca.pem
server: https://localhost:6443
name: local
contexts:
- context:
cluster: local
namespace: default
user: user
name: Default
current-context: Default
kind: Config
preferences: {}
users:
- name: user
user:
client-certificate: external-cert.pem
client-key: external-key.rsa
57 changes: 57 additions & 0 deletions test/config/concatenated-ca.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
-----BEGIN CERTIFICATE-----
MIIDADCCAeigAwIBAgIUQZjM/5qoAF78qIDyc+rKi4qBdOIwDQYJKoZIhvcNAQEL
BQAwGDEWMBQGA1UEAxMNa3ViZXJuZXRlcy1jYTAeFw0yMjAzMjIxNDQzMDBaFw0z
MjAzMTkxNDQzMDBaMBgxFjAUBgNVBAMTDWt1YmVybmV0ZXMtY2EwggEiMA0GCSqG
SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDGkG7g+UjpDhZ7A4Pm7Hme+RWs5IHz4I2X
IclvtO3LuJ26yzz2S8VaXFFeUqzEPb2G1RxFGvoAVN7qrTw0n5MQJCFLAA4dI7oY
8XLRJ7KgTBBIw1jYpgKb2zyHPIJE6VmslliKUiX+QDovdRU/dsbdup2EucrnGw4+
QNNAc3XMbXgm6lubA6znYZlSpcQ8BKer3tq75q4KUZicIjS6gKQyZjk9a6fcOuCS
ybtlAKp9lYzcwxZkNrx+V1PJMQ1qaJWPnMAVi7Oj5Dm3Jmf1WHBcNEh52Q/0vYlt
4WSaeM5t/Py/m/7c4Ve97f5m2X6EhYyUbzov4qeZOnIJI3MnU1FxAgMBAAGjQjBA
MA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSl1qyt
jd96WstRE8h9x5qkCvZUvjANBgkqhkiG9w0BAQsFAAOCAQEAJt55qYvBaniAwvgO
tbO79g1FcQGrxpMX45TuoCE/K+MWDjrr6bp+FbLOqT8MwOsbGwwJIRTHGvkEkVso
5AWI5aSNs3hWnltOdz27ZSHeX77WB4daK1tLK6ggZrp3v9iIpbBwWBFdmAqsPvEs
H17K2BgAzdh6xRKPQd0BGTUpJBfk50R2gDMj7FKyIzBN69IOGytBfAXBhHzEGy4+
MvtTEIMUjR//KgCrpNeyDuaWHttR5FdnuRxFO7O3BAfyNSaNmd/IEHQf7DIGgzOy
+xWLyH/HRHj5C70qAqjbnrgBODI99BsA9U7oXTuyPLdIboAcFt2zD5DIYgZET52X
53w4jA==
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
MIIC/zCCAeegAwIBAgITPbfpy29aBG67ChRdB6lJegTkmDANBgkqhkiG9w0BAQsF
ADAYMRYwFAYDVQQDEw1rdWJlcm5ldGVzLWNhMB4XDTIyMDIyMTA5MDIwMFoXDTMy
MDIxOTA5MDIwMFowGDEWMBQGA1UEAxMNa3ViZXJuZXRlcy1jYTCCASIwDQYJKoZI
hvcNAQEBBQADggEPADCCAQoCggEBAMLjZ2cFBalzoimaEcpT9Jz2JmdgsHMOagVd
It7OQpTwDZ3npIICVpguEh9xtovR8m8/HYM+/a4vMQHT+3p8HPjiDaRYGg7OZ9L+
Fp/9zhBuiaIvg8Z+Bbys9Q9Uuj6VEwfFJBcNH6TmzdiDgQUs5/k+6/vtuJ4ys3sD
KkAOxqPXDaBoANnLpIxdIMQDcWSLFA0wmFhdZJq3KEAoJpEL0WYo1ZRBV3iH77yf
sDbN1OBu2vNnRZ+DrV0ZJ5ApmbFXPX8i4KJaW9lCB62FN0j5XsNDoyTeAVpesfNs
zYufVpBdqNZFkOKg9diMuTMika2aYfDuiVzdebDgcp9aMloKtbECAwEAAaNCMEAw
DgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFBdOiygC
LcuJrq8rNa1xADr5Sp7CMA0GCSqGSIb3DQEBCwUAA4IBAQDCy4IlhASh6Br5XEcI
TpP5ThD1OyRzQnsPe6P1qgWP3kBXK/AcsSl+VGtaZp2oEhJoUnsz7kE8yW3gK+PA
51zY4aHTiF9xkyd5zOCAGB+cfp9Ys+szWzyu0QQ9IBjJ4+eDjg7W0/S+BM2Qn1iL
jTFIe2Bdf+Q/J24/q3ksTXK17UNun14vDRsJgsNcrFt/rumfHPx1ytwsiqKyEKV7
kFxSwa3d8/AvhGgFpPmfRjU7gAJCFcHz501zhi2a6L5TYBTecVRbqZoeHiZ0YNWI
is5g4VmVB+BxMAM2WEd29v4l/3oI1Pey9rvt7NJqSe1im9uqZgVDeg/vP8zKs/dF
ZYw8
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
MIIDADCCAeigAwIBAgIUHW3OPnmuTquJ0YgbGpmm/blsY2QwDQYJKoZIhvcNAQEL
BQAwGDEWMBQGA1UEAxMNa3ViZXJuZXRlcy1jYTAeFw0yMjAzMjIxNDQ0MDBaFw0z
MjAzMTkxNDQ0MDBaMBgxFjAUBgNVBAMTDWt1YmVybmV0ZXMtY2EwggEiMA0GCSqG
SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDLMEJs5agS0hNQBxPTtsI6dIhIi/pY8liI
sNukbi5KwKf80FYNyRXqE8ufDVyTFzOc+MG96jnHjDaBWjrVN9On0PgUBo4nPyd4
DtyvYx2jMzwToSEIo/Z1aroMx1oGywCgdS4/3FWAbhlSbyXKJmhfh6gX0TxWz+dV
zqNuqQq9EWuRhOMg9vgzjfp3mjiPE10lW8pT0j5JT3PI/eGO+C2Z7z33LJXb6GM2
nXvhGFMGY+7XG65pqJ3L8g1mk+LjPiwyIItw8wPtrnrZ2VXMklMd5Mn+jgCTNe1B
om0nPpPIiTblCr6gcNcVjy5WGN37OKlqrT0JTuSPHcxSUp05LFjDAgMBAAGjQjBA
MA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBQvV/sB
wbR3UwjkLAMN+6P3fZ/3OjANBgkqhkiG9w0BAQsFAAOCAQEACAk4EQwCkw2EBsSR
2SKoa1SjYFkZzIr/0/TB2YcMUvHF+RpvlD5vQ8/RJjeAl1kc6/niZ9TWCemjBLqI
hPoFe49zr49DyQjC2ZfsXVJvFCr6g7o4q4DtQ6ltyBuTJbkn1hI+aB8zgvpofG44
mKj18Y7tPvgXtRua4SaeBq777+22AOvKxPied9p4PTrMN4RKTP6+yIbLflej7dBD
zQDjfmmYsH0T2ZRtBpE1dYrUbU3tkizcMZRJBgreoxoff+r5coibMIm/7gh+YoSb
BCItCaeuGSKQ8CJb8DElcPUd6nKUjmeiQL68ztsG/+CXLiL/TZb914VaaCXvPInw
49jJ7w==
-----END CERTIFICATE-----
2 changes: 2 additions & 0 deletions test/config/update_certs_k0s.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ def sh!(*cmd)
# The rest could easily be extracted from allinone.kubeconfig, but the test is more robust
# if we don't reuse YAML and/or Kubeclient::Config parsing to construct test data.
sh! "#{DOCKER} exec #{CONTAINER} cat /var/lib/k0s/pki/ca.crt > test/config/external-ca.pem"
sh! 'cat test/config/another-ca1.pem test/config/external-ca.pem '\
' test/config/another-ca2.pem > test/config/concatenated-ca.pem'
sh! "#{DOCKER} exec #{CONTAINER} cat /var/lib/k0s/pki/admin.crt > test/config/external-cert.pem"
sh! "#{DOCKER} exec #{CONTAINER} cat /var/lib/k0s/pki/admin.key > test/config/external-key.rsa"

Expand Down
6 changes: 6 additions & 0 deletions test/test_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ def test_external_nopath_absolute
end
end

def test_concatenated_ca
config = Kubeclient::Config.read(config_file('concatenated-ca.kubeconfig'))
assert_equal(['Default'], config.contexts)
check_context(config.context, ssl: true)
end

def test_nouser
config = Kubeclient::Config.read(config_file('nouser.kubeconfig'))
assert_equal(['default/localhost:6443/nouser'], config.contexts)
Expand Down
79 changes: 54 additions & 25 deletions test/test_real_cluster.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,59 +16,88 @@ def teardown
WebMock.disable_net_connect! # Don't allow any connections in other tests.
end

# Partially isolated tests that check Client behavior with given `verify_ssl` value:

# localhost and 127.0.0.1 are among names on the certificate
HOSTNAME_COVERED_BY_CERT = 'https://127.0.0.1:6443'.freeze
# 127.0.0.2 also means localhost but is not included in the certificate.
HOSTNAME_NOT_ON_CERT = 'https://127.0.0.2:6443'.freeze

def test_real_cluster_verify_peer
config = Kubeclient::Config.read(config_file('external.kubeconfig'))
context = config.context
# localhost and 127.0.0.1 are among names on the certificate
client1 = Kubeclient::Client.new(
'https://127.0.0.1:6443', 'v1',
HOSTNAME_COVERED_BY_CERT, 'v1',
ssl_options: context.ssl_options.merge(verify_ssl: OpenSSL::SSL::VERIFY_PEER),
auth_options: context.auth_options
)
client1.discover
client1.get_nodes
exercise_watcher_with_timeout(client1.watch_nodes)
# 127.0.0.2 also means localhost but is not included in the certificate.
check_cert_accepted(client1)
client2 = Kubeclient::Client.new(
'https://127.0.0.2:6443', 'v1',
HOSTNAME_NOT_ON_CERT, 'v1',
ssl_options: context.ssl_options.merge(verify_ssl: OpenSSL::SSL::VERIFY_PEER),
auth_options: context.auth_options
)
# TODO: all OpenSSL exceptions should be wrapped with Kubeclient error.
assert_raises(Kubeclient::HttpError, OpenSSL::SSL::SSLError) do
client2.discover
end
# Since discovery fails, methods like .get_nodes, .watch_nodes would all fail
# on method_missing -> discover. Call lower-level methods to test actual connection.
assert_raises(Kubeclient::HttpError, OpenSSL::SSL::SSLError) do
client2.get_entities('Node', 'nodes', {})
end
assert_raises(Kubeclient::HttpError, OpenSSL::SSL::SSLError) do
exercise_watcher_with_timeout(client2.watch_entities('nodes'))
end
check_cert_rejected(client2)
end

def test_real_cluster_verify_none
config = Kubeclient::Config.read(config_file('external.kubeconfig'))
context = config.context
# localhost and 127.0.0.1 are among names on the certificate
client1 = Kubeclient::Client.new(
'https://127.0.0.1:6443', 'v1',
HOSTNAME_COVERED_BY_CERT, 'v1',
ssl_options: context.ssl_options.merge(verify_ssl: OpenSSL::SSL::VERIFY_NONE),
auth_options: context.auth_options
)
client1.get_nodes
# 127.0.0.2 also means localhost but is not included in the certificate.
check_cert_accepted(client1)
client2 = Kubeclient::Client.new(
'https://127.0.0.2:6443', 'v1',
HOSTNAME_NOT_ON_CERT, 'v1',
ssl_options: context.ssl_options.merge(verify_ssl: OpenSSL::SSL::VERIFY_NONE),
auth_options: context.auth_options
)
client2.get_nodes
check_cert_accepted(client2)
end

def test_real_cluster_concatenated_ca
config = Kubeclient::Config.read(config_file('concatenated-ca.kubeconfig'))
context = config.context
client1 = Kubeclient::Client.new(
HOSTNAME_COVERED_BY_CERT, 'v1',
ssl_options: context.ssl_options.merge(verify_ssl: OpenSSL::SSL::VERIFY_PEER),
auth_options: context.auth_options
)
check_cert_accepted(client1)
client2 = Kubeclient::Client.new(
HOSTNAME_NOT_ON_CERT, 'v1',
ssl_options: context.ssl_options.merge(verify_ssl: OpenSSL::SSL::VERIFY_PEER),
auth_options: context.auth_options
)
check_cert_rejected(client2)
end

private

# Test cert checking on discovery, CRUD, and watch code paths.
def check_cert_accepted(client)
client.discover
client.get_nodes
exercise_watcher_with_timeout(client.watch_nodes)
end

def check_cert_rejected(client)
# TODO: all OpenSSL exceptions should be wrapped with Kubeclient error.
assert_raises(Kubeclient::HttpError, OpenSSL::SSL::SSLError) do
client.discover
end
# Since discovery fails, methods like .get_nodes, .watch_nodes would all fail
# on method_missing -> discover. Call lower-level methods to test actual connection.
assert_raises(Kubeclient::HttpError, OpenSSL::SSL::SSLError) do
client.get_entities('Node', 'nodes', {})
end
assert_raises(Kubeclient::HttpError, OpenSSL::SSL::SSLError) do
exercise_watcher_with_timeout(client.watch_entities('nodes'))
end
end

def exercise_watcher_with_timeout(watcher)
thread = Thread.new do
sleep(1)
Expand Down