Add safer data-handling in ConjurCA #1688

liavyona · 2020-07-15T07:36:45Z

What does this PR do?

In order to avoid NoMethodError that can be raised when the ConjurCA relevant resources is missing/have no value, added custom exceptions which inform the proper error.

What ticket does this PR close?

resolves #1315

Checklists

Reproduce the bug by deploy current version
Deploy a new env with the these changes
Test authn k8s works fine
Test the new exceptions raised when ConjurCa has malformed data.

Change log

This PR does not include user-facing changes and doesn't require a CHANGELOG update

orenbm · 2020-07-15T08:15:51Z

app/domain/repos/conjur_ca.rb

+ private
+
+ def self.fetch_secret
+ @fetch_secret ||= ::Conjur::FetchRequiredSecrets.new


let's give this variable the same name as the class it represents - @fetch_required_secrets

orenbm · 2020-07-15T08:18:00Z

app/domain/repos/conjur_ca.rb

+ begin
+ fetch_secret.(resource_ids: [ca_info.cert_id, ca_info.key_id])
+ rescue Errors::Conjur::RequiredResourceMissing
+ raise Errors::Authentication::AuthnK8s::CaResourceMissing, resource_id


what is resource_id in this context? what will be printed to the log?

Also - which log message will be the most helpful for the user?

which message will give them the best pin-pointed action item?

Refactor exceptions messages

orenbm · 2020-07-15T08:20:39Z

app/domain/repos/conjur_ca.rb

@@ -14,12 +14,28 @@ def self.create(resource_id)
 #
 def self.ca(resource_id)
 ca_info = ::Conjur::CaInfo.new(resource_id)
- stored_cert = Resource[ca_info.cert_id].last_secret.value
- stored_key = Resource[ca_info.key_id].last_secret.value
+ stored_cert, stored_key = fetch_ca_secrets(ca_info, resource_id)


i think this is quite risky, as it assumes the order of the retrieval. what happens if someone will change the behaviour of FetchRequiredSecrets so that it retrieves the secrets in parallel? In this case the first secret in the array may not be stored_cert. How can you overcome this?

uCatu

WIP

uCatu · 2020-07-15T08:04:39Z

app/domain/errors.rb

@@ -306,6 +306,16 @@ module AuthnK8s
 msg: "Invalid Kubernetes host id: {0}. Must end with <namespace>/<resource_type>/<resource_id>",
 code: "CONJ00048E"
 )
+
+ CaResourceMissing = ::Util::TrackableErrorClass.new(
+ msg: "Resource {0-resource-id} doesn't have saved resource for cert_id or key_id ",


Do we have validation for log messages from Docs or is it not needed here?

From where are we getting the resource-id?

From the creation of the exception

uCatu · 2020-07-15T08:05:47Z

app/domain/errors.rb

+
+ CaResourceMissing = ::Util::TrackableErrorClass.new(
+ msg: "Resource {0-resource-id} doesn't have saved resource for cert_id or key_id ",
+ code: "CONJ00063E"


Those error codes need to be documeneted in a KB/Troubleshooting guide?

not at the moment. there is an effort led by @netacoral but i am not sure that it has started.

uCatu · 2020-07-15T08:07:28Z

app/domain/repos/conjur_ca.rb

@@ -14,12 +14,28 @@ def self.create(resource_id)
 #
 def self.ca(resource_id)
 ca_info = ::Conjur::CaInfo.new(resource_id)
- stored_cert = Resource[ca_info.cert_id].last_secret.value
- stored_key = Resource[ca_info.key_id].last_secret.value
+ stored_cert, stored_key = fetch_ca_secrets(ca_info, resource_id)


What is the reasoning for using oneliner?

uCatu · 2020-07-15T08:27:25Z

app/domain/repos/conjur_ca.rb

 ca_cert = OpenSSL::X509::Certificate.new(stored_cert)
 ca_key = OpenSSL::PKey::RSA.new(stored_key)
 ::Util::OpenSsl::CA.new(ca_cert, ca_key)
 end

+ class << self


Are we sure we want to use this architacutre of class + private? Is it TDD - can we write tests?

codeclimate · 2020-07-15T12:52:43Z

app/domain/repos/conjur_ca.rb

+ raise Errors::Authentication::AuthnK8s::CaResourceMissing, ca_resource_id
+ rescue Errors::Conjur::RequiredSecretMissing
+ raise Errors::Authentication::AuthnK8s::CaResourceValueMissing, ca_resource_id
+


Extra empty line detected at begin body end.

codeclimate · 2020-07-15T12:52:43Z

app/domain/repos/conjur_ca.rb

+ end
+
+ def self.fetch_ca_resource(ca_resource_id)
+ begin


Redundant begin block detected.

def self.fetch_ca_resource(ca_resource_id) begin fetch_required_secrets.(resource_ids: [ca_resource_id])[ca_resource_id] rescue Errors::Conjur::RequiredResourceMissing raise Errors::Authentication::AuthnK8s::CaResourceMissing, ca_resource_id rescue Errors::Conjur::RequiredSecretMissing raise Errors::Authentication::AuthnK8s::CaResourceValueMissing, ca_resource_id end end

@orenbm When I delete the begin it has errors with the indentation of rescue (and to fix this they need to be in the same indent as def which basically won't work). What's the right way?

this should do it:

def self.fetch_ca_resource(ca_resource_id) fetch_required_secrets.(resource_ids: [ca_resource_id])[ca_resource_id] rescue Errors::Conjur::RequiredResourceMissing raise Errors::Authentication::AuthnK8s::CaResourceMissing, ca_resource_id rescue Errors::Conjur::RequiredSecretMissing raise Errors::Authentication::AuthnK8s::CaResourceValueMissing, ca_resource_id end

orenbm · 2020-07-15T13:37:16Z

app/domain/repos/conjur_ca.rb

+ stored_cert = fetch_ca_resource(ca_info.cert_id)
+ stored_key = fetch_ca_resource(ca_info.key_id)


this will make 2 calls to the DB instead of 1 so it's not the ideal solution. I would retrieve them together and then take each var from the retrieved list. you can see how we did it in FetchAuthenticatorSecrets.

orenbm · 2020-07-15T13:38:21Z

app/domain/repos/conjur_ca.rb

+ rescue Errors::Conjur::RequiredResourceMissing
+ raise Errors::Authentication::AuthnK8s::CaResourceMissing, ca_resource_id
+ rescue Errors::Conjur::RequiredSecretMissing
+ raise Errors::Authentication::AuthnK8s::CaResourceValueMissing, ca_resource_id


do you see value in raising the error CaResourceValueMissing instead of the error RequiredResourceMissing? what value does the user get from this message that they don't get from the original?

orenbm

Beautiful!

Care to fix your git history?

orenbm · 2020-07-15T14:28:16Z

@liavyona you will also need to rebase on master

codeclimate · 2020-07-15T14:37:02Z

app/domain/repos/conjur_ca.rb

@@ -14,12 +14,16 @@ def self.create(resource_id)
 #
 def self.ca(resource_id)
 ca_info = ::Conjur::CaInfo.new(resource_id)
- stored_cert = Resource[ca_info.cert_id].last_secret.value
- stored_key = Resource[ca_info.key_id].last_secret.value
+ ca_secrets = fetch_required_secrets.(resource_ids: [ca_info.cert_id, ca_info.key_id])


Repos::ConjurCA#self.ca calls 'ca_info.cert_id' 2 times

codeclimate · 2020-07-15T14:37:02Z

app/domain/repos/conjur_ca.rb

@@ -14,12 +14,16 @@ def self.create(resource_id)
 #
 def self.ca(resource_id)
 ca_info = ::Conjur::CaInfo.new(resource_id)
- stored_cert = Resource[ca_info.cert_id].last_secret.value
- stored_key = Resource[ca_info.key_id].last_secret.value
+ ca_secrets = fetch_required_secrets.(resource_ids: [ca_info.cert_id, ca_info.key_id])


Repos::ConjurCA#self.ca calls 'ca_info.key_id' 2 times

Add fetch_required_secrets to handle malformed ca There are 3 types of malformed ca data: a. No resource - The ca has no key/cert saved on the db. b. No value - The resource exists but has an empty value (not initialized yet). c. Bad value - The value is not the right one. The added function is meant to take care of the first two cases by raising relevant exception to each case. The third one is being taken care by the OpenSSL library.

liavyona · 2020-07-16T06:38:15Z

Beautiful!

Care to fix your git history?

Done

liavyona · 2020-07-16T06:38:37Z

@liavyona you will also need to rebase on master

Done

codeclimate · 2020-07-16T06:49:24Z

Code Climate has analyzed commit 65b26a6 and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 86.2% (0.0% change).

View more on Code Climate.

uCatu

My comments are totally personal taste, up to you.
LGTM well done

uCatu · 2020-07-16T13:11:23Z

app/domain/repos/conjur_ca.rb

- stored_key = Resource[ca_info.key_id].last_secret.value
+ ca_cert_id = ca_info.cert_id
+ ca_key_id = ca_info.key_id
+ ca_secrets = fetch_required_secrets.(resource_ids: [ca_cert_id, ca_key_id])


Suggested change

ca_secrets = fetch_required_secrets.(resource_ids: [ca_cert_id, ca_key_id])

ca_secrets = fetch_required_secrets.(resource_ids: [ca_info.cert_id, ca_info.key_id])

uCatu · 2020-07-16T13:11:50Z

app/domain/repos/conjur_ca.rb

+ ca_cert_id = ca_info.cert_id
+ ca_key_id = ca_info.key_id
+ ca_secrets = fetch_required_secrets.(resource_ids: [ca_cert_id, ca_key_id])
+ stored_cert = ca_secrets[ca_cert_id]


Suggested change

stored_cert = ca_secrets[ca_cert_id]

stored_cert = ca_secrets[ca_info.cert_id]

uCatu · 2020-07-16T13:12:15Z

app/domain/repos/conjur_ca.rb

+ ca_key_id = ca_info.key_id
+ ca_secrets = fetch_required_secrets.(resource_ids: [ca_cert_id, ca_key_id])
+ stored_cert = ca_secrets[ca_cert_id]
+ stored_key = ca_secrets[ca_key_id]


Suggested change

stored_key = ca_secrets[ca_key_id]

stored_key = ca_secrets[ca_info.key_id]

liavyona added component/authenticators component/k8s component/conjur labels Jul 15, 2020

liavyona requested review from orenbm, nahumcohen and uCatu July 15, 2020 07:36

liavyona requested a review from a team as a code owner July 15, 2020 07:36

orenbm reviewed Jul 15, 2020

View reviewed changes

uCatu suggested changes Jul 15, 2020

View reviewed changes

codeclimate bot reviewed Jul 15, 2020

View reviewed changes

orenbm reviewed Jul 15, 2020

View reviewed changes

orenbm previously approved these changes Jul 15, 2020

View reviewed changes

codeclimate bot reviewed Jul 15, 2020

View reviewed changes

liavyona dismissed orenbm’s stale review via d738424 July 16, 2020 06:26

liavyona force-pushed the malformed-conjurca-handling branch from cd5fd49 to d738424 Compare July 16, 2020 06:26

liavyona force-pushed the malformed-conjurca-handling branch from d738424 to 65b26a6 Compare July 16, 2020 06:32

orenbm approved these changes Jul 16, 2020

View reviewed changes

uCatu approved these changes Jul 16, 2020

View reviewed changes

liavyona merged commit cfb781e into master Jul 16, 2020

ismarc deleted the malformed-conjurca-handling branch April 13, 2022 19:09

		stored_cert = fetch_ca_resource(ca_info.cert_id)
		stored_key = fetch_ca_resource(ca_info.key_id)

	ca_secrets = fetch_required_secrets.(resource_ids: [ca_cert_id, ca_key_id])
	ca_secrets = fetch_required_secrets.(resource_ids: [ca_info.cert_id, ca_info.key_id])

	stored_cert = ca_secrets[ca_cert_id]
	stored_cert = ca_secrets[ca_info.cert_id]

	stored_key = ca_secrets[ca_key_id]
	stored_key = ca_secrets[ca_info.key_id]

Add safer data-handling in ConjurCA #1688

Add safer data-handling in ConjurCA #1688

Conversation

liavyona commented Jul 15, 2020

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uCatu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Jul 15, 2020

Choose a reason for hiding this comment

codeclimate bot Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm left a comment

Choose a reason for hiding this comment

orenbm commented Jul 15, 2020

codeclimate bot Jul 15, 2020

Choose a reason for hiding this comment

codeclimate bot Jul 15, 2020

Choose a reason for hiding this comment

liavyona commented Jul 16, 2020

liavyona commented Jul 16, 2020

codeclimate bot commented Jul 16, 2020

uCatu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm Jul 15, 2020 •

edited

Loading