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

feat: add support for namespace label selector to authn-k8s #2613

Merged
merged 16 commits into from
Aug 1, 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: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ 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.18.0] - 2022-07-12
## [1.18.0] - 2022-08-01

### Added
- Adds support for namespace label based identity scope for the Kubernetes Authenticator
[cyberark/conjur#2613](https://github.com/cyberark/conjur/pull/2613)

### Changed
- Adds support for authentication using OIDC's code authorization flow
[cyberark/conjur#2595](https://github.com/cyberark/conjur/pull/2595)
Expand Down
1 change: 0 additions & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ group :development, :test do
gem 'cucumber', '~> 7.1'
gem 'database_cleaner', '~> 1.8'
gem 'debase', '~> 0.2.5.beta2'
gem 'faye-websocket'
gem 'json_spec', '~> 1.1'
gem 'faye-websocket'
gem 'net-ssh'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ def valid_restriction?(restriction)
case restriction.name
when Restrictions::NAMESPACE
if restriction.value != @namespace
raise Errors::Authentication::AuthnK8s::NamespaceMismatch(@namespace, restriction.value)
raise Errors::Authentication::AuthnK8s::NamespaceMismatch.new(@namespace, restriction.value)
end
when Restrictions::NAMESPACE_LABEL_SELECTOR
@k8s_resource_validator.valid_namespace?(label_selector: restriction.value)
else
# Restrictions defined using '-', but the k8s client expects type with '_' instead.
# e.g. 'restriction=stateful-set' converted to 'k8s_type=stateful_set'
Expand Down
7 changes: 4 additions & 3 deletions app/domain/authentication/authn_k8s/consts.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module AuthnK8s
module Restrictions

NAMESPACE = "namespace"
NAMESPACE_LABEL_SELECTOR = "namespace-label-selector"
SERVICE_ACCOUNT = "service-account"
POD = "pod"
DEPLOYMENT = "deployment"
Expand All @@ -17,13 +18,13 @@ module Restrictions
# This is not exactly a restriction, because it only validates container existence and not requesting container name.
AUTHENTICATION_CONTAINER_NAME = "authentication-container-name"

REQUIRED = [NAMESPACE].freeze
REQUIRED_EXCLUSIVE = [NAMESPACE, NAMESPACE_LABEL_SELECTOR].freeze
RESOURCE_TYPE_EXCLUSIVE = [DEPLOYMENT, DEPLOYMENT_CONFIG, STATEFUL_SET].freeze
OPTIONAL = [SERVICE_ACCOUNT, POD, AUTHENTICATION_CONTAINER_NAME].freeze
PERMITTED = REQUIRED + RESOURCE_TYPE_EXCLUSIVE + OPTIONAL
PERMITTED = REQUIRED_EXCLUSIVE + RESOURCE_TYPE_EXCLUSIVE + OPTIONAL

CONSTRAINTS = Constraints::MultipleConstraint.new(
Constraints::RequiredConstraint.new(required: REQUIRED),
Constraints::RequiredExclusiveConstraint.new(required_exclusive: REQUIRED_EXCLUSIVE),
Constraints::PermittedConstraint.new(permitted: PERMITTED),
Constraints::ExclusiveConstraint.new(exclusive: RESOURCE_TYPE_EXCLUSIVE)
)
Expand Down
9 changes: 9 additions & 0 deletions app/domain/authentication/authn_k8s/k8s_object_lookup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,15 @@ def pod_by_name(podname, namespace)
k8s_client_for_method("get_pod").get_pod(podname, namespace)
end

# Returns the labels hash for a Namespace with a given name.
#
# @return nil if no such Namespace exists.
def namespace_labels_hash(namespace)
namespace_object = k8s_client_for_method("get_namespace").get_namespace(namespace)

return namespace_object.metadata.labels.to_h unless namespace_object.nil?
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnK8s::K8sObjectLookup#namespace_labels_hash refers to 'namespace_object' more than self (maybe move it to another class?)

Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnK8s::K8sObjectLookup#namespace_labels_hash performs a nil-check

end

# Locates pods matching label selector in a namespace.
#
def pods_by_label(label_selector, namespace)
Expand Down
38 changes: 38 additions & 0 deletions app/domain/authentication/authn_k8s/k8s_resource_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,44 @@ def valid_resource?(type:, name:)
@logger.debug(LogMessages::Authentication::AuthnK8s::ValidatedK8sResource.new(type, name))
end

# Validates label selector and creates a hash
# In the spirit of https://github.com/kubernetes/apimachinery/blob/master/pkg/labels/selector.go
def valid_namespace?(label_selector:)
Copy link

Choose a reason for hiding this comment

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

Method valid_namespace? has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

Copy link

Choose a reason for hiding this comment

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

Complex method Authentication::AuthnK8s::K8sResourceValidator#valid_namespace? (33.5)

Copy link

Choose a reason for hiding this comment

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

Complex method Authentication::AuthnK8s::K8sResourceValidator#valid_namespace? (40.4)

Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnK8s::K8sResourceValidator#valid_namespace? has approx 14 statements

@logger.debug(LogMessages::Authentication::AuthnK8s::ValidatingK8sResourceLabel.new('namespace', namespace, label_selector))

if label_selector.length == 0
Copy link

Choose a reason for hiding this comment

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

Use empty? instead of length == 0.

raise Errors::Authentication::AuthnK8s::InvalidLabelSelector.new(label_selector)
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnK8s::K8sResourceValidator#valid_namespace? calls 'raise Errors::Authentication::AuthnK8s::InvalidLabelSelector.new(label_selector)' 2 times

Copy link

Choose a reason for hiding this comment

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

Provide an exception class and message as arguments to raise.

end
Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

label_selector_hash = label_selector
.split(",")
.map{ |kv_pair|
kv_pair = kv_pair.split(/={1,2}/, 2)

invalid ||= kv_pair.length != 2
invalid ||= kv_pair[0].include?("!")
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnK8s::K8sResourceValidator#valid_namespace? calls 'kv_pair[0]' 2 times


if (invalid)
Copy link

Choose a reason for hiding this comment

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

Don't use parentheses around a variable.

Copy link

Choose a reason for hiding this comment

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

Don't use parentheses around the condition of an if.

raise Errors::Authentication::AuthnK8s::InvalidLabelSelector.new(label_selector)
Copy link

Choose a reason for hiding this comment

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

Provide an exception class and message as arguments to raise.

end

kv_pair[0] = kv_pair[0].to_sym
kv_pair
}
.to_h

# Fetch namespace labels
# TODO: refactor this to have a generic label fetching method in @k8s_object_lookup
labels_hash = @k8s_object_lookup.namespace_labels_hash(namespace)

# Validates label selector hash against labels hash
unless label_selector_hash.all? { |k, v| labels_hash[k] == v }
raise Errors::Authentication::AuthnK8s::LabelSelectorMismatch.new('namespace', namespace, label_selector)
end

@logger.debug(LogMessages::Authentication::AuthnK8s::ValidatedK8sResourceLabel.new('namespace', namespace, label_selector))
return true
Copy link

Choose a reason for hiding this comment

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

Redundant return detected.

end

private

def retrieve_k8s_resource(type, name)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module Authentication
module Constraints

# This constraint is initialized with an array of strings.
# They represent resource restrictions where exactly one is required.
class RequiredExclusiveConstraint

def initialize(required_exclusive:)
@required_exclusive = required_exclusive
end

def validate(resource_restrictions:)
restrictions_found = resource_restrictions & @required_exclusive
raise Errors::Authentication::Constraints::IllegalRequiredExclusiveCombination.new(@required_exclusive, restrictions_found) unless restrictions_found.length == 1
end

end
end
end
16 changes: 16 additions & 0 deletions app/domain/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,16 @@ module AuthnK8s
code: "CONJ00026E"
)

LabelSelectorMismatch = ::Util::TrackableErrorClass.new(
msg: "Kubernetes {0-resource-type} '{1-resource-id}' does not match label-selector: '{2-label-selector}'",
code: "CONJ00083E"
)

InvalidLabelSelector = ::Util::TrackableErrorClass.new(
msg: "Invalid label-selector '{0-label-selector}': must adhere to format '<key>=<value>,<key1>=<value2>,...', supports '=' and '=='",
code: "CONJ00094E"
)

ContainerNotFound = ::Util::TrackableErrorClass.new(
msg: "Container '{0}' was not found in the pod. Host id: {1}",
code: "CONJ00028E"
Expand Down Expand Up @@ -699,6 +709,12 @@ module Constraints
msg: "Role must have at least one relevant annotation",
code: "CONJ00099E"
)

IllegalRequiredExclusiveCombination = ::Util::TrackableErrorClass.new(
msg: "Role must have exactly one of the following required constraints: " \
"{0-constraints}. Role configured with {1-provided}",
code: "CONJ00131E"
)
end
end

Expand Down
14 changes: 12 additions & 2 deletions app/domain/logs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -234,14 +234,24 @@ module AuthnK8s
)

ValidatingK8sResource = ::Util::TrackableLogMessageClass.new(
msg: "Validating K8s resource. Type:'{0}', Name: {1}",
msg: "Validating K8s resource. Type:'{0}', Name:'{1}'",
code: "CONJ00050D"
)

ValidatedK8sResource = ::Util::TrackableLogMessageClass.new(
msg: "Validated K8s resource. Type:'{0}', Name: {1}",
msg: "Validated K8s resource. Type:'{0}', Name:'{1}'",
code: "CONJ00051D"
)

ValidatingK8sResourceLabel = ::Util::TrackableLogMessageClass.new(
msg: "Validating K8s resource using label selector. Type:'{0}', Name:'{1}', Label:'{2}'",
code: "CONJ00145D"
)

ValidatedK8sResourceLabel = ::Util::TrackableLogMessageClass.new(
msg: "Validated K8s resource using label selector. Type:'{0}', Name:'{1}', Label:'{2}'",
code: "CONJ00146D"
)
end

module AuthnIam
Expand Down
37 changes: 37 additions & 0 deletions build-and-publish-internal-appliance.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#!/usr/bin/env bash
Copy link
Contributor

@john-odonnell john-odonnell Aug 1, 2022

Choose a reason for hiding this comment

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

Do we want to merge this script?

Maybe this could be codified as a way of building appliance images for a target working/POC branches, but it seems like once this PR merges, we won't need it in its current form.

set -euo pipefail


IMAGE="registry.tld/conjur-appliance:eval-authn-k8s-label-selector"

echo "Building on top of stable appliance image and pushing to ${IMAGE}"

echo "

# ---
FROM registry.tld/conjur-appliance:5.0-stable

# Copy new source files
$(
echo "
app/domain/authentication/authn_k8s/authentication_request.rb
app/domain/authentication/authn_k8s/consts.rb
app/domain/authentication/authn_k8s/k8s_object_lookup.rb
app/domain/authentication/authn_k8s/k8s_resource_validator.rb
app/domain/authentication/constraints/required_exclusive_constraint.rb
app/domain/errors.rb
" | docker run --rm -i --entrypoint="" ruby:2-alpine ruby -e '
files = STDIN.read.split("\n").reject(&:empty?)
puts files.map {|file| "COPY #{file} /opt/conjur/possum/#{file}"}.join("\n")
'
)

RUN chown -R conjur:conjur /opt/conjur/possum/app

# ---

" | \
tee /dev/stderr | \
docker build -f - -t "${IMAGE}" .

docker push "${IMAGE}"
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
# frozen_string_literal: true

require 'spec_helper'

RSpec.describe(Authentication::AuthnK8s::K8sResourceValidator) do
let(:log_output) { StringIO.new }
let(:logger) {
Logger.new(
log_output,
formatter: proc do | severity, time, progname, msg |
"#{severity},#{msg}\n"
end)
}

subject {
described_class.new(k8s_object_lookup: k8s_object_lookup, pod: pod, logger: logger)
}

let(:k8s_object_lookup) {
double("k8s_object_lookup").tap { |d|
allow(d).to receive(:namespace_labels_hash)
.with("namespace_name")
.and_return({ :key1 => "value1", :key2 => "value2" })
}
}

let(:pod) {
double("pod").tap { |d|
d.stub_chain("metadata.namespace").and_return("namespace_name")
}
}

context "#valid_namespace?" do
it 'raises error on empty label selector' do
expect { subject.valid_namespace?(label_selector: "") }.to(
raise_error(
::Errors::Authentication::AuthnK8s::InvalidLabelSelector
)
)
end

it 'raises error on invalid label selector' do
# No key-value pair
expect { subject.valid_namespace?(label_selector: "key,") }.to(
raise_error(
::Errors::Authentication::AuthnK8s::InvalidLabelSelector
)
)

# Unsupported operator
expect { subject.valid_namespace?(label_selector: "key!=value") }.to(
raise_error(
::Errors::Authentication::AuthnK8s::InvalidLabelSelector
)
)
end

it 'returns true for labels matching label-selector' do
# Single key, single equals format
expect(
subject.valid_namespace?(label_selector: "key1=value1")
).to be true
# Single key, double equals format
expect(
subject.valid_namespace?(label_selector: "key2==value2")
).to be true
# Multiple keys
expect(
subject.valid_namespace?(label_selector: "key1=value1,key2=value2")
).to be true
end

it 'throws an error for labels not matching label-selector' do
# Value mismatch
expect { subject.valid_namespace?(label_selector: "key1=notvalue") }.to(
raise_error(
::Errors::Authentication::AuthnK8s::LabelSelectorMismatch
)
)
# Key not found
expect { subject.valid_namespace?(label_selector: "notfoundkey=value") }.to(
raise_error(
::Errors::Authentication::AuthnK8s::LabelSelectorMismatch
)
)
# One of multiple keys does not match
expect { subject.valid_namespace?(label_selector: "key1=value1,notfoundkey=value") }.to(
raise_error(
::Errors::Authentication::AuthnK8s::LabelSelectorMismatch
)
)
end

it 'logs before label-selector validation begins, and after success' do
subject.valid_namespace?(label_selector: "key1=value1")

expect(log_output.string.split("\n")).to include(
"DEBUG,CONJ00145D Validating K8s resource using label selector. Type:'namespace', Name:'namespace_name', Label:'key1=value1'",
"DEBUG,CONJ00146D Validated K8s resource using label selector. Type:'namespace', Name:'namespace_name', Label:'key1=value1'"
)
end

it 'logs before label-selector validation begins, but not after failure' do
expect { subject.valid_namespace?(label_selector: "key1=notvalue") }.to raise_error

expect(log_output.string.split("\n")).to include(
"DEBUG,CONJ00145D Validating K8s resource using label selector. Type:'namespace', Name:'namespace_name', Label:'key1=notvalue'",
)
expect(log_output.string.split("\n")).not_to include(
"DEBUG,CONJ00146D Validated K8s resource using label selector. Type:'namespace', Name:'namespace_name', Label:'key1=notvalue'"
)
end
end
end
Loading