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

Presence of bearer token should cancel exec action #91745

Merged
merged 2 commits into from Jul 9, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions hack/lib/util.sh
Expand Up @@ -452,6 +452,22 @@ function kube::util::test_openssl_installed {
OPENSSL_BIN=$(command -v openssl)
}

# Query the API server for client certificate authentication capabilities
function kube::util::test_client_certificate_authentication_enabled {
local output
kube::util::test_openssl_installed

output=$(echo \
| "${OPENSSL_BIN}" s_client -connect "127.0.0.1:${SECURE_API_PORT}" 2> /dev/null \
| grep -A3 'Acceptable client certificate CA names')

if [[ "${output}" != *"/CN=127.0.0.1"* ]] && [[ "${output}" != *"CN = 127.0.0.1"* ]]; then
echo "API server not configured for client certificate authentication"
echo "Output of from acceptable client certificate check: ${output}"
anderseknert marked this conversation as resolved.
Show resolved Hide resolved
exit 1
fi
}

# creates a client CA, args are sudo, dest-dir, ca-id, purpose
# purpose is dropped in after "key encipherment", you usually want
# '"client auth"'
Expand Down
1 change: 1 addition & 0 deletions hack/make-rules/test-cmd.sh
Expand Up @@ -69,6 +69,7 @@ function run_kube_apiserver() {
--storage-media-type="${KUBE_TEST_API_STORAGE_TYPE-}" \
--cert-dir="${TMPDIR:-/tmp/}" \
--service-cluster-ip-range="10.0.0.0/24" \
--client-ca-file=hack/testdata/ca.crt \
anderseknert marked this conversation as resolved.
Show resolved Hide resolved
--token-auth-file=hack/testdata/auth-tokens.csv 1>&2 &
export APISERVER_PID=$!

Expand Down
17 changes: 17 additions & 0 deletions hack/testdata/ca.crt
@@ -0,0 +1,17 @@
-----BEGIN CERTIFICATE-----
MIICpjCCAY4CCQCZBiNB23olFzANBgkqhkiG9w0BAQsFADAUMRIwEAYDVQQDDAkx
MjcuMC4wLjEwIBcNMjAwNjE0MTk0OTM4WhgPMjI5NDAzMzAxOTQ5MzhaMBQxEjAQ
BgNVBAMMCTEyNy4wLjAuMTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB
AMxIjMd58IhiiyK4VjmuCWBUZksSs1CcQuo5HSpOqogVZ+vR5mdJDZ56Pw/NSM5c
RqOB3cvjGrxYQe/lKvo9D3UmWLcRKtxdlWxCfPekioJ25/dhGOxtBQcjtp/TSqTM
txprwT4fvsVwiwaURFoCOivF4xjQFG0K1i3/m7CiMHODy67M1EfJDrM7Vv5XPIuJ
VF8HhWBH2HiM25ak34XhxVTX8K97k6wO9OZ5GMqbYuVobTZrSRdiv8s95rkmik6P
jn0ePKqSz6cXNXgXqTl11WtsuoGgjOdB8j/noqTF3m3z17sSBqqG/xBFuSFoNceA
yBDb9ohbs8oY3NIZzyMrt8MCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAFgcaqRgv
qylx4ogL5iUr0K2e/8YzsvH7zLHG6xnr7HxpR/p0lQt3dPlppECZMGDKElbCgU8f
xVDdZ3FOxHTJ51Vnq/U5xJo+UOMJ4sS8fEH8cfNliSsvmSKzjxpPKqbCJ7VTnkW8
lonedCPRksnhlD1U8CF21rEjKsXcLoX5PsxlS4DX3PtO0+e8aUh9F4XyZagpejq8
0ttXkWd3IyYrpFRGDlFDxIiKx7pf+mG6JZ/ms6jloBSwwcz/Nkn5FMxiq75bQuOH
EV+99S2du/X2bRmD1JxCiMDw8cMacIFBr6BYXsvKOlivwfHBWk8U0f+lVi60jWje
PpKFRd1mYuEZgw==
-----END CERTIFICATE-----
Expand Up @@ -178,6 +178,15 @@ type credentials struct {
// UpdateTransportConfig updates the transport.Config to use credentials
// returned by the plugin.
func (a *Authenticator) UpdateTransportConfig(c *transport.Config) error {
// If a bearer token is present in the request - avoid the GetCert callback when
// setting up the transport, as that triggers the exec action if the server is
// also configured to allow client certificates for authentication. For requests
// like "kubectl get --token (token) pods" we should assume the intention is to
// use the provided token for authentication.
if c.HasTokenAuth() {
Copy link
Member

Choose a reason for hiding this comment

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

if we already have a client cert/key, should that take precedence as well and prevent calling the exec plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that makes sense, yes. This PR addressed a real problem we experienced with our exec plugin using tokens, and the client certificate scenario wasn't something I considered - I don't even know what the current behavior there is to be honest, but I could certainly look into supporting that case as well if you want. With that said, I think the single argument of --token makes it a lot more likely to be used ad-hoc than client certificates, which requires something like three(?) file locations to be provided.

Copy link
Member

Choose a reason for hiding this comment

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

I think the current state of the PR is coherent w.r.t. token auth. A follow up issue to discuss/resolve treatment of client cert auth is fine with me

return nil
}
anderseknert marked this conversation as resolved.
Show resolved Hide resolved

anderseknert marked this conversation as resolved.
Show resolved Hide resolved
c.Wrap(func(rt http.RoundTripper) http.RoundTripper {
return &roundTripper{a, rt}
})
Expand Down
Expand Up @@ -620,6 +620,27 @@ func TestRoundTripper(t *testing.T) {
get(t, http.StatusOK)
}

func TestTokenPresentCancelsExecAction(t *testing.T) {
a, err := newAuthenticator(newCache(), &api.ExecConfig{
Command: "./testdata/test-plugin.sh",
APIVersion: "client.authentication.k8s.io/v1alpha1",
})
if err != nil {
t.Fatal(err)
}

// UpdateTransportConfig returns error on existing TLS certificate callback, unless a bearer token is present in the
// transport config, in which case it takes precedence
cert := func() (*tls.Certificate, error) {
return nil, nil
}
tc := &transport.Config{BearerToken: "token1", TLS: transport.TLSConfig{Insecure: true, GetCert: cert}}

if err := a.UpdateTransportConfig(tc); err != nil {
t.Error("Expected presence of bearer token in config to cancel exec action")
}
}

func TestTLSCredentials(t *testing.T) {
now := time.Now()

Expand Down
81 changes: 81 additions & 0 deletions test/cmd/authentication.sh
@@ -0,0 +1,81 @@
#!/usr/bin/env bash

# Copyright 2020 The Kubernetes Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

run_exec_credentials_tests() {
set -o nounset
set -o errexit

kube::log::status "Testing kubectl with configured exec credentials plugin"

cat > "${TMPDIR:-/tmp}"/invalid_exec_plugin.yaml << EOF
apiVersion: v1
clusters:
- cluster:
name: test
contexts:
- context:
cluster: test
user: invalid_token_user
name: test
current-context: test
kind: Config
preferences: {}
users:
- name: invalid_token_user
user:
exec:
apiVersion: client.authentication.k8s.io/v1beta1
# Any invalid exec credential plugin will do to demonstrate
command: ls
EOF

### Provided --token should take precedence, thus not triggering the (invalid) exec credential plugin
# Pre-condition: Client certificate authentication enabled on the API server
kube::util::test_client_certificate_authentication_enabled
# Command
output=$(kubectl "${kube_flags_with_token[@]:?}" --kubeconfig="${TMPDIR:-/tmp}"/invalid_exec_plugin.yaml get namespace kube-system -o name || true)
anderseknert marked this conversation as resolved.
Show resolved Hide resolved

if [[ "${output}" == "namespace/kube-system" ]]; then
kube::log::status "exec credential plugin not triggered since kubectl was called with provided --token"
else
kube::log::status "Unexpected output when providing --token for authentication - exec credential plugin likely triggered. Output: ${output}"
exit 1
fi
# Post-condition: None

### Without provided --token, the exec credential plugin should be triggered
# Pre-condition: Client certificate authentication enabled on the API server - already checked by positive test above

# Command
output2=$(kubectl "${kube_flags_without_token[@]:?}" --kubeconfig="${TMPDIR:-/tmp}"/invalid_exec_plugin.yaml get namespace kube-system -o name 2>&1 || true)

if [[ "${output2}" =~ "json parse error" ]]; then
kube::log::status "exec credential plugin triggered since kubectl was called without provided --token"
else
kube::log::status "Unexpected output when not providing --token for authentication - exec credential plugin not triggered. Output: ${output2}"
exit 1
fi
# Post-condition: None

rm "${TMPDIR:-/tmp}"/invalid_exec_plugin.yaml

set +o nounset
set +o errexit
}
14 changes: 11 additions & 3 deletions test/cmd/legacy-script.sh
Expand Up @@ -29,6 +29,7 @@ KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/../..
# source "${KUBE_ROOT}/hack/lib/test.sh"
source "${KUBE_ROOT}/test/cmd/apply.sh"
source "${KUBE_ROOT}/test/cmd/apps.sh"
source "${KUBE_ROOT}/test/cmd/authentication.sh"
source "${KUBE_ROOT}/test/cmd/authorization.sh"
source "${KUBE_ROOT}/test/cmd/batch.sh"
source "${KUBE_ROOT}/test/cmd/certificate.sh"
Expand Down Expand Up @@ -341,11 +342,13 @@ runTests() {
'-s' "http://127.0.0.1:${API_PORT}"
)

# token defined in hack/testdata/auth-tokens.csv
kube_flags_with_token=(
'-s' "https://127.0.0.1:${SECURE_API_PORT}" '--token=admin-token' '--insecure-skip-tls-verify=true'
kube_flags_without_token=(
'-s' "https://127.0.0.1:${SECURE_API_PORT}" '--insecure-skip-tls-verify=true'
)

# token defined in hack/testdata/auth-tokens.csv
kube_flags_with_token=( "${kube_flags_without_token[@]}" '--token=admin-token' )

if [[ -z "${ALLOW_SKEW:-}" ]]; then
kube_flags+=('--match-server-version')
kube_flags_with_token+=('--match-server-version')
Expand Down Expand Up @@ -760,6 +763,11 @@ runTests() {
record_command run_nodes_tests
fi

########################
# Authentication
########################

record_command run_exec_credentials_tests

########################
# authorization.k8s.io #
Expand Down