Adds validation logic for secrets aliases and IDs (Conjur var paths) #376

diverdane · 2021-10-19T22:11:30Z

What does this PR do?

This PR adds logic to validate the secrets aliases (based on output file format, e.g. YAML, JSON, Bash, or dotenv) and to validate the IDs (i.e. Conjur variable paths) contained in each SecretSpec.

The validation is done according to the
Input Validations for Annotations
section of the
M1 Push-to-File Design Spec.

The validation is done before retrieving any secret values from Conjur so that we fail fast.

What ticket does this PR close?

ONYX-12559

Checklists

Change log

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, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

rpothier

Looks great! just curious if you looked at using the json Valid function.

diverdane · 2021-10-20T19:08:05Z

just curious if you looked at using the json Valid function.

I suppose it would be possible to use the json.Valid() function, but there are a couple of factors:

We're testing the validity of JSON keys before they are used to generate an actual JSON object. So in order to test using json.Valid(), we would have to construct a temporary JSON object (using some dummy string for the corresponding value for that key, e.g. "{\"<key-being-tested>\": \"dummy-value\"}" and pass that JSON to json.Valid().
I played around a bit with the json.Valid() function, and it is accepting the DEL character as being valid within a JSON key. The JSON spec says that ALL control characters are invalid within a JSON key. ASCII control characters include the DEL character: https://i.stack.imgur.com/Ip6y7.png.

doodlesbykumbi

PR looking good. I left a couple comments.

Fly by comments...

I'm wondering about how far down the rabbit hole of validation we want to go. The YAML validation seems a bit involved. I looked at the spec for YAML, and compared that against some commonly used YAML processing libraries and I haven't seen anything yet that goes into the weeds for validation. I worry that we might potentially constrain too much in a way that creates friction in SP interacting with other tools that consume the formats.

'm not sure what the answer is but the following questions have been on my mind re: alias validation for formats

What value are we providing to the user by adding this nuanced validation
What is the cost to us of providing this nuanced validation in terms of maintenance, ensuring we enforce the constraints without getting in the way etc.

doodlesbykumbi · 2021-10-21T17:39:07Z

pkg/secrets/pushtofile/retrieve_secrets.go

 	for _, group := range *secretGroups {
 		for _, spec := range group.SecretSpecs {
-			ids = append(ids, spec.Id)
+			if err := validateSecretSpec(spec, group.FileFormat); err != nil {


I think this is too late to validate the secret spec. For early feedback it would be better to carry out these checks at the time when the annotations are used to create the secret group.

Good point! I'll move this validation so that it happens earlier.

The validation occurs earlier now, in the NewSecretGroupsFromAnnotations function.

pkg/secrets/pushtofile/secret_spec.go

diverdane · 2021-10-22T17:57:42Z

@doodlesbykumbi :

I'm wondering about how far down the rabbit hole of validation we want to go. The YAML validation seems a bit involved. I looked at the spec for YAML, and compared that against some commonly used YAML processing libraries and I haven't seen anything yet that goes into the weeds for validation. I worry that we might potentially constrain too much in a way that creates friction in SP interacting with other tools that consume the formats.

I'm not sure what the answer is but the following questions have been on my mind re: alias validation for formats

What value are we providing to the user by adding this nuanced validation

What is the cost to us of providing this nuanced validation in terms of maintenance, ensuring we enforce the constraints without getting in the way etc.

This may be more of a quality question for @andytinkham or @adamouamani. Questions came up during the design review for M1 re. input validation for a bunch of inputs. At that time the thought was that the allowable character sets for the keys of each of these formats was well-known, and that for each format, we could fairly easily implement validity checks that are the most liberal allowable for that format (i.e. allow the widest range of characters as allowed for that format). (Applications may want to restrict the allowable character set even further... but each application may have different restrictions, so we need to design our validity checks to be the most liberal/lenient so that we work across all applications.)

The value add is that if someone is using M1, but has a weird programmatic error that results in strange characters being used for secrets aliases, we'll fail fast and provide an explicit error log pointing to the error.

If we've interpreted the specifications for each format correctly, and we're allowing the widest range of characters allowed, then I don't think the ongoing maintenance cost will be that big.

diverdane · 2021-10-28T16:13:10Z

@doodlesbykumbi,

I've addressed your latest comments/suggestions.

One issue that came up is that the current alias validator functions (and the FileTemplateForFormat function) return a single error, rather than a slice. I started to change this behavior, but it became a bit involved. For now, the alias validation fails at the first invalid alias it finds, so only the first invalid alias is reported. I think we should address this as a followup, and as you suggested, consider using Golang custom errors to allow multiple, similar, context-specific errors such as these to get passed up the stack as a single custom error.

andytinkham · 2021-10-28T19:46:57Z

This may be more of a quality question for @andytinkham or @adamouamani.

I don't have anything more to add here. My main concern is that we don't inadvertently leak secret values or otherwise introduce a security risk in parsing the Yaml. @adamouamani might have quality thoughts.

rpothier

LGTM!

rpothier · 2021-10-29T14:31:39Z

pkg/secrets/pushtofile/secret_group.go

@@ -25,6 +27,8 @@ type SecretGroup struct {
 	SecretSpecs      []SecretSpec
 }

+// ResolvedSecretSpecs resolves all of the secret paths for a secret
+// group by prepending each path with that group's policy path prefix.


Thanks for adding the comments!

rpothier · 2021-10-29T14:32:49Z

pkg/secrets/pushtofile/standard_templates_test.go

+		// YAML file format, 8-bit characters
+		{"printable ASCII", '\u003F', assertNoError()},
+		{"heart emoji", '💙', assertNoError()},
+		{"dog emoji", '🐶', assertNoError()},


What about the🔫 char ?

I suppose I could have added that, but figured we have 2 representative emojis being tested.

codeclimate · 2021-10-29T16:11:47Z

Code Climate has analyzed commit 75f5f5a and detected 0 issues on this pull request.

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

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

View more on Code Climate.

diverdane self-assigned this Oct 19, 2021

diverdane requested review from a team as code owners October 19, 2021 22:11

diverdane force-pushed the ONYX-12559 branch from a3fc270 to 6537fe9 Compare October 19, 2021 22:18

rpothier reviewed Oct 20, 2021

View reviewed changes

doodlesbykumbi reviewed Oct 21, 2021

View reviewed changes

diverdane force-pushed the ONYX-12559 branch 3 times, most recently from 8d277f0 to 31cc972 Compare October 22, 2021 17:32

diverdane force-pushed the ONYX-12559 branch 7 times, most recently from 3fdbaa0 to 1f29238 Compare October 28, 2021 16:06

diverdane force-pushed the ONYX-12559 branch from 1f29238 to a8b4699 Compare October 29, 2021 14:19

rpothier approved these changes Oct 29, 2021

View reviewed changes

Adds validation logic for secrets aliases and IDs (Conjur var paths)

75f5f5a

diverdane force-pushed the ONYX-12559 branch from a8b4699 to 75f5f5a Compare October 29, 2021 14:33

doodlesbykumbi approved these changes Oct 29, 2021

View reviewed changes

diverdane merged commit 538e83e into main Oct 29, 2021

diverdane deleted the ONYX-12559 branch November 11, 2021 19:41

jtuttle mentioned this pull request Nov 12, 2021

Bump version for v1.14.1+suite.1 release cyberark/conjur-oss-suite-release#237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds validation logic for secrets aliases and IDs (Conjur var paths) #376

Adds validation logic for secrets aliases and IDs (Conjur var paths) #376

diverdane commented Oct 19, 2021 •

edited

Loading

rpothier left a comment

diverdane commented Oct 20, 2021

doodlesbykumbi left a comment

doodlesbykumbi Oct 21, 2021

diverdane Oct 22, 2021

diverdane Oct 25, 2021

diverdane commented Oct 22, 2021

diverdane commented Oct 28, 2021

andytinkham commented Oct 28, 2021

rpothier left a comment

rpothier Oct 29, 2021

rpothier Oct 29, 2021

diverdane Oct 29, 2021

codeclimate bot commented Oct 29, 2021

Adds validation logic for secrets aliases and IDs (Conjur var paths) #376

Adds validation logic for secrets aliases and IDs (Conjur var paths) #376

Conversation

diverdane commented Oct 19, 2021 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

rpothier left a comment

Choose a reason for hiding this comment

diverdane commented Oct 20, 2021

doodlesbykumbi left a comment

Choose a reason for hiding this comment

doodlesbykumbi Oct 21, 2021

Choose a reason for hiding this comment

diverdane Oct 22, 2021

Choose a reason for hiding this comment

diverdane Oct 25, 2021

Choose a reason for hiding this comment

diverdane commented Oct 22, 2021

diverdane commented Oct 28, 2021

andytinkham commented Oct 28, 2021

rpothier left a comment

Choose a reason for hiding this comment

rpothier Oct 29, 2021

Choose a reason for hiding this comment

rpothier Oct 29, 2021

Choose a reason for hiding this comment

diverdane Oct 29, 2021

Choose a reason for hiding this comment

codeclimate bot commented Oct 29, 2021

diverdane commented Oct 19, 2021 •

edited

Loading