Allow dot and colon in variable path #5898

xtremerui · 2020-07-21T02:25:59Z

What does this PR accomplish?

Bug Fix | Feature | Documentation

closes #4249 .

Changes proposed by this PR:

Allow dot . and : in variable path if quoted with ". Parse variable name in ((var_name)) to VariableReference so underneath implementation of Variable interface doesn't need to.

((foo:"a_path_with.dot".bar.zoo)) ->

VariableReference{
  Name: "foo:\"a_path_with.dot\".bar.zoo",
  Source: "foo",
  Path: "a_path_with.dot",
  Fields: ["bar","zoo"],
}

Release Note

You can now interpolate variables with special characters . and : in the name by wrapping them in double quotes
e.g. (("some.secret".field1)) accesses field1 of the secret some.secret

Notes to reviewer:

Contributor Checklist

Followed Code of conduct, Contributing Guide & avoided Anti-patterns
Signed all commits
Added tests (Unit and/or Integration)
Updated Documentation
Updated Release notes

Reviewer Checklist

Code reviewed
Tests reviewed
Documentation reviewed
Release notes reviewed
PR acceptance performed
New config flags added? Ensure that they are added to the
BOSH and
Helm packaging; otherwise, ignored for
the integration
tests
(for example, if they are Garden configs that are not displayed in the
--help text).

atc/creds/conjur/conjur.go

chenbh · 2020-07-21T13:48:53Z

atc/creds/credhub/credhub.go

 	var cred credentials.Credential
 	var found bool
 	var err error

-	cred, found, err = c.findCred(secretPath)
+	cred, found, err = c.findCred(ref.Name)


Same issue as Conjur, looking below it seems like it only cares about Path and not Fields. But I haven't actually used Credhub so can't say for sure

atc/creds/kubernetes/kubernetes_test.go

atc/creds/kubernetes/secrets.go

atc/creds/secret_var_lookup_rules.go

chenbh · 2020-07-21T14:02:05Z

vars/template.go


-var ErrEmptyVar = errors.New("empty var")
+	varRef := VariableReference{Name: name}


I'm beginning to think that the Name shouldn't include Source, that way it'll just be Path + Fields and won't break all of our existing cred managers

The problem is if we do path + fields we will need to concatenate them back with . and wrap them properly with ". I think the parsing of (()) makes us to use ref.Path for creds manager.

And the logic in template will take care of looking up fields.

We can probably use parts[1]

concourse/vars/template.go

Lines 144 to 145 in 46efd51

parts := strings.SplitN(name, ":", 2)

varRef.Source = parts[0]

And the logic in template will take care of looking up fields.

True, I guess another question is how much processing should the cred manager do vs Concourse. They technically have all the information (e.g. Fields) required to fully resolve the secret, but it might be less code duplication if Concourse handles that part

vars/template.go

chenbh · 2020-07-21T14:10:15Z

vars/template.go

+// Get value of a var. Name can be the following formats: 1) 'foo', where foo
+// is var name; 2) 'foo:bar', where foo is var source name, and bar is var name;
+// 3) '.:foo', where . means a local var, foo is var name.


Comment needs updating, the main purpose now is to extract Fields from a map[string]interface{}

I think the end goal of this Get is still finding a string value? Extracting fields is the first part. It then use the fields to do recursive look up.

aoldershaw · 2020-07-22T23:21:48Z

After a bunch of fiddling, I eventually got a local Conjur connected to Concourse. I ended up mostly following the quick start, but made the few modifications to the quick start repo. These are the changes I made

Don't use SSL (was having trouble getting self-signed certs to pair with concourse)

diff --git a/conf/default.conf b/conf/default.conf
index 8aa7e18..0832396 100755
--- a/conf/default.conf
+++ b/conf/default.conf
@@ -1,11 +1,8 @@
 server {
-    listen              443 ssl;
+    listen              80;
     server_name         proxy;
     access_log          /var/log/nginx/access.log;

-    ssl_certificate     /etc/nginx/tls/nginx.crt;
-    ssl_certificate_key /etc/nginx/tls/nginx.key;
-
     location / {
       proxy_pass http://conjur;
     }

diff --git a/docker-compose.yml b/docker-compose.yml
index 1332b86..56ad583 100755
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -53,10 +53,9 @@ services:
     image: nginx:1.13.6-alpine
     container_name: nginx_proxy
     ports:
-      - "8443:443"
+      - "8081:80"
     volumes:
       - ./conf/:/etc/nginx/conf.d/:ro
-      - ./conf/tls/:/etc/nginx/tls/:ro
     depends_on:
     - conjur
     - openssl

Use the following policy, rather than the provided BotApp.yml:

- !policy
  id: concourse
  body:
  - !variable "secret.value"
  - !variable "secret"

Use the following docker-compose override for running concourse:

version: '3'

services:
  web:
    environment:
      CONCOURSE_CONJUR_APPLIANCE_URL: http://docker.for.mac.localhost:8081
      CONCOURSE_CONJUR_ACCOUNT: myConjurAccount
      CONCOURSE_CONJUR_AUTHN_LOGIN: admin
      CONCOURSE_CONJUR_AUTHN_API_KEY: 1xkf4xh2efa4ja7k3ng0rhdqg52dajps63ks08h511q08ks1d8a4pd
      CONCOURSE_CONJUR_SECRET_TEMPLATE: concourse/{{.Secret}}

where the API_KEY is the admin key (in the admin_data file from the quick start).

Anyway, I tested it a bit with the changes in this PR and without. Something about this change seems to not work with (at least) Conjur, even in the case where there are no dots or colons in the path. e.g. consider the following pipeline:

jobs:
- name: job
  plan:
  - task: echo
    config:
      platform: linux
      image_resource:
        type: registry-image
        source: {repository: busybox}
      run:
        path: echo
        args: [((secret))]

On master, this works correctly (i.e. echos the value of secret in Conjur), but with this change, I get the following error:

failed to interpolate task config: 404 Not Found. Variable 'concourse/main/echo/secret' not found in account 'myConjurAccount'.

(note: I had to modify to modify this line

concourse/atc/creds/conjur/conjur.go

Line 43 in a822574

return nil, nil, false, nil

to get the error message to show up, since currently we just ignore it and assume the var wasn't found)

xtremerui · 2020-07-23T15:25:25Z

Thx for the detail of integration test.

(note: I had to modify to modify this line

concourse/atc/creds/conjur/conjur.go

Line 43 in a822574

return nil, nil, false, nil

to get the error message to show up, since currently we just ignore it and assume the var wasn't found)

This error is strange since it outputs the ~~correct path~~ path for looking up secret under path concourse/team_name/pipeline_name,~~which matches the unit test~~. Concourse doesn't implement RetrieveSecret so there is no change in that part. I wonder what the argument of RetrieveSecret is in master when it success to fetch the secret. Will try this locally.

Updated: the working path in master is concourse/secret, which matches the policy defined in conjur. So the problem is concourse is using the wrong path to fetch the secret (or it doesn't try each kind of template for path)

* extract source, path and fields from secrect name instead of treating it as black box * move nested variables lookup out of template into static_vars * refactor trackers error handling Signed-off-by: Rui Yang <ryang@pivotal.io> Co-authored-by: Bohan Chen <bochen@pivotal.io>

put back fields look up logic from static_vars to tempalte Signed-off-by: Rui Yang <ruiya@vmware.com>

Signed-off-by: Rui Yang <ruiya@vmware.com>

xtremerui · 2020-07-23T18:43:50Z

@aoldershaw the way that creds manager looking up secret is

generate secret looks up path. If no templating ENV is set, it will use the default path template concourse/team_name/pipeline_name/secret, concourse/team_name/secert, concourse/secret in Conjur's case.
try fetching the secret by EVERY path here

concourse/atc/creds/secret_var_lookup.go

Line 22 in c5e2ad3

for _, rule := range sl.LookupPaths {

notice how it relies on the return of Get

concourse/atc/creds/secret_var_lookup.go

Line 28 in c5e2ad3

result, _, found, err := sl.Secrets.Get(secretRef)

. It always expect no error so it could go to next iteration. So in your case if you change the line in Conjur to return an error. It will failed in the first attempt and stop the loop, while the correct path concourse/secret is ignored. Not sure if that is your case or not.

In my local testing, I rebased master and run the test pretty much follow your instruction. I have these values set up in Conjur.

docker-compose exec client conjur variable value concourse/secret 
blah
 docker-compose exec client conjur variable value concourse/secret.value
foo

In my test both ((secret)) and (("secret.value")) revolved to blah and foo, respectively. And ((secret.value)) give back error like
failed to interpolate task config: cannot access field 'value' of non-map value ('string') from var: secret.value.

Additionally, I use the setup to test static variable by fly -t local sp -p echo -c echo.yml -v secret.value=buzz.

I also used this pipeline to test named variable

var_sources:
- name: dumb
  type: dummy
  config:
    vars:
      simple: hello!
      user.value:
        username: big
        password: sekrit

jobs:
- name: job
  plan:
  - task: echo
    config:
      platform: linux
      image_resource:
        type: registry-image
        source: {repository: busybox}
      run:
        path: echo
        args: [((dumb:"user.value".password))]

All seems working. Could you try this rebased version again? Thx!

aoldershaw · 2020-07-27T14:03:05Z

@xtremerui just tried it out again (with both Conjur and Vault), and it works as expected. Will take a closer look through the code now

aoldershaw

Have some questions

aoldershaw · 2020-07-27T14:48:35Z

vars/template.go

 }

+// var ErrEmptyVar = errors.New("empty var")
+
 type varsTracker struct {


Just realized there's both a varsTracker and a credVarsTracker (which happens to live in vars_tracker.go). Doesn't have to be on this PR to change this, but it's a bit confusing

vars/template.go

atc/creds/cached_secrets.go

passing variable reference is unnecessary. Signed-off-by: Rui Yang <ruiya@vmware.com>

vars/template.go

aoldershaw · 2020-07-28T18:45:31Z

Looks pretty good but FYI some of the unit tests are broken - missed reverting some test code in dfcc427 I think

use source+path as unique identifier for visited all error Signed-off-by: Rui Yang <ruiya@vmware.com>

aoldershaw

LGTM!

TheSecMaven · 2020-07-29T21:17:06Z

what version of concourse will this roll into?

aoldershaw · 2020-07-29T21:19:29Z

@mkkeffeler should be the next release (6.5.0), which will probably be in around 2-3 weeks

documents concourse/concourse#5898 Signed-off-by: Izabela Gomes <igomes@pivotal.io> Co-authored-by: Aidan Oldershaw <aoldershaw@pivotal.io>

TheSecMaven · 2020-10-05T20:37:55Z

was this test case tested? seems to be faiiling for us on 6.6

params:
  CP_CREDENTIALS: (("me/Nonprod/N_A_GOG_GCP_SVC_PFOP_NL/CloudService-N_A_GOG_GCP_NL-me.me.com-test@me.com/password"))

xtremerui · 2020-10-06T14:27:07Z

was it working before? Seems to me the last part com-test@me.com needs to be quoted?

TheSecMaven · 2020-10-06T14:30:32Z

actually no. its a conjur credential. all of those dots are dots in the actual secret name, not a sub attribute or anything like that

xtremerui · 2020-10-06T15:39:29Z

do you mean the var is not evaluated to a value by conjur when you say "failed"? or there is an error.

TheSecMaven · 2020-10-06T15:44:28Z

the var is not evaluated to a value in conjur, conjur logs show it isn't being looked up. when we print "env" we see the variable that we typed in the pipeline, rather than the value. other variables work though.

xtremerui · 2020-10-06T15:56:48Z

@mkkeffeler Thx for the info. I think the interpolation regex failed to match the var name in your particular case. This is a bug we will fix it.

TheSecMaven · 2020-10-06T16:01:09Z

hmm not sure i follow. Can you explain how it does that? and is there a workaround?

xtremerui · 2020-10-06T16:10:45Z

we have a regex to first check if the node in yml file is a var that needed to be interpolated or not. For example, one condition is it needs to be surrounded by ((..)). There are some other conditions as well.

The example you gave by current regex is not considered a var at all so it is treated as normal text. By allowing @ in the regex will fix the problem. If you are more interested you can check the PR I submit to fix this later.

TheSecMaven · 2020-10-06T19:14:53Z

got it

we also will have spaces in some vars. not sure if thats already working, but worth calling out :)

vito assigned scottietremendous and aoldershaw Jul 21, 2020

xtremerui added the enhancement label Jul 21, 2020

chenbh reviewed Jul 21, 2020

View reviewed changes

xtremerui force-pushed the issue-4249-dot-in-var-path branch from b827a06 to 4709956 Compare July 21, 2020 19:36

Rui Yang and others added 6 commits July 23, 2020 11:38

chagne var name to var reference in creds package

c02c411

put back fields look up logic from static_vars to tempalte Signed-off-by: Rui Yang <ruiya@vmware.com>

vars: update test to take dot and colon in var path

ba0d062

Signed-off-by: Rui Yang <ruiya@vmware.com>

fix tests outside of vars and creds package

8d3171d

Signed-off-by: Rui Yang <ruiya@vmware.com>

creds: ref.Path instead of red.Name to fetch secret

390db17

Signed-off-by: Rui Yang <ruiya@vmware.com>

creds: fix credhub secret path

c5e2ad3

Signed-off-by: Rui Yang <ruiya@vmware.com>

xtremerui force-pushed the issue-4249-dot-in-var-path branch from 4709956 to c5e2ad3 Compare July 23, 2020 18:12

aoldershaw requested changes Jul 27, 2020

View reviewed changes

creds: revert change in Secrets interface

dfcc427

passing variable reference is unnecessary. Signed-off-by: Rui Yang <ruiya@vmware.com>

xtremerui force-pushed the issue-4249-dot-in-var-path branch 2 times, most recently from 8e2f773 to d8818bd Compare July 28, 2020 13:51

aoldershaw reviewed Jul 28, 2020

View reviewed changes

vars/template.go Show resolved Hide resolved

vars: use unique var name for missing error

03306f7

use source+path as unique identifier for visited all error Signed-off-by: Rui Yang <ruiya@vmware.com>

xtremerui force-pushed the issue-4249-dot-in-var-path branch from d8818bd to 03306f7 Compare July 29, 2020 02:13

aoldershaw approved these changes Jul 29, 2020

View reviewed changes

xtremerui merged commit 721bfa4 into master Jul 29, 2020

xtremerui deleted the issue-4249-dot-in-var-path branch July 29, 2020 21:04

aoldershaw added a commit to concourse/docs that referenced this pull request Aug 18, 2020

special characters in variables

973943f

documents concourse/concourse#5898 Signed-off-by: Izabela Gomes <igomes@pivotal.io> Co-authored-by: Aidan Oldershaw <aoldershaw@pivotal.io>

aoldershaw mentioned this pull request Aug 18, 2020

add docs for special characters in variables concourse/docs#361

Merged

aoldershaw added the release/documented Documentation and release notes have been updated. label Aug 21, 2020

aoldershaw mentioned this pull request Sep 8, 2020

Instanced pipelines - Add instance vars as part of the pipeline identifier #5896

Merged

11 tasks

mouellet mentioned this pull request Sep 26, 2020

set-pipeline --var no longer supports nested keys #6041

Closed

xtremerui mentioned this pull request Oct 6, 2020

Allow @ in vars path #6129

Merged

7 tasks

aoldershaw mentioned this pull request Oct 17, 2020

Fix regression preventing the use of nested fields with fly set-pipeline --var #6170

Merged

11 tasks

muntac mentioned this pull request Nov 16, 2020

Add nested fields for var fix to 6.7.2 #6279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow dot and colon in variable path #5898

Allow dot and colon in variable path #5898

xtremerui commented Jul 21, 2020 •

edited by aoldershaw

Loading

chenbh Jul 21, 2020

chenbh Jul 21, 2020

xtremerui Jul 21, 2020

chenbh Jul 21, 2020

chenbh Jul 21, 2020

chenbh Jul 21, 2020

xtremerui Jul 21, 2020

aoldershaw commented Jul 22, 2020

xtremerui commented Jul 23, 2020 •

edited

Loading

xtremerui commented Jul 23, 2020

aoldershaw commented Jul 27, 2020 •

edited

Loading

aoldershaw left a comment

aoldershaw Jul 27, 2020

aoldershaw commented Jul 28, 2020

aoldershaw left a comment

TheSecMaven commented Jul 29, 2020

aoldershaw commented Jul 29, 2020

TheSecMaven commented Oct 5, 2020

xtremerui commented Oct 6, 2020

TheSecMaven commented Oct 6, 2020

xtremerui commented Oct 6, 2020

TheSecMaven commented Oct 6, 2020

xtremerui commented Oct 6, 2020

TheSecMaven commented Oct 6, 2020

xtremerui commented Oct 6, 2020

TheSecMaven commented Oct 6, 2020


		var ErrEmptyVar = errors.New("empty var")
		varRef := VariableReference{Name: name}

	parts := strings.SplitN(name, ":", 2)
	varRef.Source = parts[0]

Allow dot and colon in variable path #5898

Allow dot and colon in variable path #5898

Conversation

xtremerui commented Jul 21, 2020 • edited by aoldershaw Loading

What does this PR accomplish?

Changes proposed by this PR:

Release Note

Notes to reviewer:

Contributor Checklist

Reviewer Checklist

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

aoldershaw commented Jul 22, 2020

xtremerui commented Jul 23, 2020 • edited Loading

xtremerui commented Jul 23, 2020

aoldershaw commented Jul 27, 2020 • edited Loading

aoldershaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoldershaw commented Jul 28, 2020

aoldershaw left a comment

Choose a reason for hiding this comment

TheSecMaven commented Jul 29, 2020

aoldershaw commented Jul 29, 2020

TheSecMaven commented Oct 5, 2020

xtremerui commented Oct 6, 2020

TheSecMaven commented Oct 6, 2020

xtremerui commented Oct 6, 2020

TheSecMaven commented Oct 6, 2020

xtremerui commented Oct 6, 2020

TheSecMaven commented Oct 6, 2020

xtremerui commented Oct 6, 2020

TheSecMaven commented Oct 6, 2020

xtremerui commented Jul 21, 2020 •

edited by aoldershaw

Loading

xtremerui commented Jul 23, 2020 •

edited

Loading

aoldershaw commented Jul 27, 2020 •

edited

Loading