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

doodlesbykumbi · 2022-07-27T13:57:39Z

Desired Outcome

Adds support for scoping Authn-K8s workload identity based on namespace labels.
See related solution design for more context.

Implemented Changes

Add new constraint class to enforce XOR on a set of resource restrictions
Add support for new Authn-K8s host annotation, authn-k8s/namespace-label-selector
Update Authn-K8s request validation to fetch a namespace's metadata and validate its labels against the configured selector
Unit test the new constraint and validation, and test the new validation end-to-end with the new in-process K8s server mock

Connected Issue/Story

CyberArk internal issue link:
ONYX-23180
ONYX-23186
ONYX-23189
ONYX-23191
ONYX-23193

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

Revert Jenkinsfile, clean up Gemfile, build-and-publish-internal-appliance.sh and authn-k8s test server

codeclimate · 2022-07-28T22:28:55Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

@@ -24,6 +24,41 @@ def valid_resource?(type:, name:)
        @logger.debug(LogMessages::Authentication::AuthnK8s::ValidatedK8sResource.new(type, name))
      end

+      def valid_namespace?(label_selector:)


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

codeclimate · 2022-07-28T22:28:55Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

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


Use empty? instead of length == 0.

codeclimate · 2022-07-28T22:28:55Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

@@ -24,6 +24,41 @@ def valid_resource?(type:, name:)
        @logger.debug(LogMessages::Authentication::AuthnK8s::ValidatedK8sResource.new(type, name))
      end

+      def valid_namespace?(label_selector:)


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

codeclimate · 2022-07-28T22:28:55Z

app/domain/authentication/authn_k8s/k8s_object_lookup.rb

+      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?


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

codeclimate · 2022-07-28T22:28:55Z

app/domain/authentication/authn_k8s/k8s_object_lookup.rb

+      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?


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

- revert to namespace-label-selector - break out resource validator logs test case - remove unneeded namespaces list response JSON

Refactor and test namespace label authentication

Add Changelog entry for Authn-K8s label-based authn scope

codeclimate · 2022-07-29T23:36:49Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

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


Redundant return detected.

codeclimate · 2022-07-29T23:36:49Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

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


Don't use parentheses around a variable.

codeclimate · 2022-07-29T23:36:49Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

@@ -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:)


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

codeclimate · 2022-07-29T23:36:49Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

+            kv_pair = kv_pair.split(/={1,2}/, 2)
+
+            invalid ||= kv_pair.length != 2
+            invalid ||= kv_pair[0].include?("!")


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

codeclimate · 2022-07-29T23:36:49Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

+        @logger.debug(LogMessages::Authentication::AuthnK8s::ValidatingK8sResourceLabel.new('namespace', namespace, label_selector))
+
+        if label_selector.length == 0
+          raise Errors::Authentication::AuthnK8s::InvalidLabelSelector.new(label_selector)


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

imheresamir

The change entries in the CHANGELOG should live under 1.18.0, please fix TY!

Addressed

codeclimate · 2022-08-01T08:24:32Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

+            invalid ||= kv_pair[0].include?("!")
+
+            if (invalid)
+              raise Errors::Authentication::AuthnK8s::InvalidLabelSelector.new(label_selector)


Provide an exception class and message as arguments to raise.

codeclimate · 2022-08-01T08:24:32Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

+        @logger.debug(LogMessages::Authentication::AuthnK8s::ValidatingK8sResourceLabel.new('namespace', namespace, label_selector))
+
+        if label_selector.length == 0
+          raise Errors::Authentication::AuthnK8s::InvalidLabelSelector.new(label_selector)


Provide an exception class and message as arguments to raise.

codeclimate · 2022-08-01T08:24:32Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

@@ -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:)


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

codeclimate · 2022-08-01T08:24:32Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

+
+        if label_selector.length == 0
+          raise Errors::Authentication::AuthnK8s::InvalidLabelSelector.new(label_selector)
+        end


Add empty line after guard clause.

codeclimate · 2022-08-01T08:24:32Z

app/domain/authentication/authn_k8s/k8s_resource_validator.rb

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


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

codeclimate · 2022-08-01T08:24:36Z

Code Climate has analyzed commit e7d1727 and detected 19 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	8
Bug Risk	1
Style	10

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

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

View more on Code Climate.

john-odonnell

LGTM, approved 👍
Not sure if we should remove the appliance build script.

john-odonnell · 2022-08-01T13:12:24Z

build-and-publish-internal-appliance.sh

@@ -0,0 +1,37 @@
+#!/usr/bin/env bash


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.

doodlesbykumbi force-pushed the appliance-authn-k8s-label-selector branch from 4d2ba6f to 74ec584 Compare July 27, 2022 14:07

doodlesbykumbi and others added 8 commits July 28, 2022 22:42

Rudimentary support for authn-k8s namespace label selector

1faba26

Build and push eval appliance image

9553766

Fix Dockerfile.appliance

070f233

Example of what local label-selector logic might look like

31b1f6f

Add unit tests for new XOR constraint

62ba804

Refactor namespace labels retrieval

dc0ad24

Refactor and test namespace label validation

fb441de

Clean up branch

1a7d45b

Revert Jenkinsfile, clean up Gemfile, build-and-publish-internal-appliance.sh and authn-k8s test server

doodlesbykumbi force-pushed the appliance-authn-k8s-label-selector branch from db53815 to 1a7d45b Compare July 28, 2022 22:06

codeclimate bot reviewed Jul 28, 2022

View reviewed changes

john-odonnell and others added 7 commits July 29, 2022 15:39

Refactor and test namespace label authentication

1083804

Refactor authn-k8s tests for reusability between test cases

5d2bed7

Fix refactor

80af822

- revert to namespace-label-selector - break out resource validator logs test case - remove unneeded namespaces list response JSON

Refactor errors, authn-k8s test server

35b9a18

Merge pull request #2615 from cyberark/refactor-label-tie-in

fd41865

Refactor and test namespace label authentication

Update Changelog

b6e4ff7

Merge pull request #2617 from cyberark/label-changelog

0012bb7

Add Changelog entry for Authn-K8s label-based authn scope

codeclimate bot reviewed Jul 29, 2022

View reviewed changes

imheresamir previously requested changes Jul 30, 2022

View reviewed changes

doodlesbykumbi marked this pull request as ready for review August 1, 2022 08:02

doodlesbykumbi requested a review from a team as a code owner August 1, 2022 08:02

Fix: fold release into 1.18.0 in CHANGELOG.md

e7d1727

doodlesbykumbi force-pushed the appliance-authn-k8s-label-selector branch from d1b1378 to e7d1727 Compare August 1, 2022 08:03

codeclimate bot reviewed Aug 1, 2022

View reviewed changes

john-odonnell approved these changes Aug 1, 2022

View reviewed changes

doodlesbykumbi merged commit ee58e09 into master Aug 1, 2022

doodlesbykumbi deleted the appliance-authn-k8s-label-selector branch August 1, 2022 15:53

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

doodlesbykumbi commented Jul 27, 2022 •

edited by john-odonnell

Loading

codeclimate bot Jul 28, 2022

codeclimate bot Jul 28, 2022

codeclimate bot Jul 28, 2022

codeclimate bot Jul 28, 2022

codeclimate bot Jul 28, 2022

codeclimate bot Jul 29, 2022

codeclimate bot Jul 29, 2022

codeclimate bot Jul 29, 2022

codeclimate bot Jul 29, 2022

codeclimate bot Jul 29, 2022

imheresamir left a comment •

edited

Loading

codeclimate bot Aug 1, 2022

codeclimate bot Aug 1, 2022

codeclimate bot Aug 1, 2022

codeclimate bot Aug 1, 2022

codeclimate bot Aug 1, 2022

codeclimate bot commented Aug 1, 2022

john-odonnell left a comment

john-odonnell Aug 1, 2022 •

edited

Loading

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

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

Conversation

doodlesbykumbi commented Jul 27, 2022 • edited by john-odonnell Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Jul 28, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 28, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 28, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 28, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 28, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 29, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 29, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 29, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 29, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 29, 2022

Choose a reason for hiding this comment

imheresamir left a comment • edited Loading

Choose a reason for hiding this comment

codeclimate bot Aug 1, 2022

Choose a reason for hiding this comment

codeclimate bot Aug 1, 2022

Choose a reason for hiding this comment

codeclimate bot Aug 1, 2022

Choose a reason for hiding this comment

codeclimate bot Aug 1, 2022

Choose a reason for hiding this comment

codeclimate bot Aug 1, 2022

Choose a reason for hiding this comment

codeclimate bot commented Aug 1, 2022

john-odonnell left a comment

Choose a reason for hiding this comment

john-odonnell Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

doodlesbykumbi commented Jul 27, 2022 •

edited by john-odonnell

Loading

imheresamir left a comment •

edited

Loading

john-odonnell Aug 1, 2022 •

edited

Loading