SecretResolver support file and base64 variable expansion #1408

jetersen · 2020-05-30T19:57:19Z

This enables nested variable expansion with included file and base64 encoding support.

If someone could argue for why we should add more String lookups I'd like to understand the use case.

Syntax is as follows:

${base64:${file:path to file}}
${readFile:path to file}
${readFileBase64:path to file}

fixes #1219
fixes #909

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or in Jenkins JIRA
Link to relevant pull requests, esp. upstream and downstream changes
Did you provide a test-case? That demonstrates feature works or fixes the issue.

plugin/src/main/java/io/jenkins/plugins/casc/SecretSourceResolver.java

codecov · 2020-05-30T21:08:11Z

Codecov Report

Merging #1408 into master will increase coverage by 0.35%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1408      +/-   ##
============================================
+ Coverage     80.66%   81.02%   +0.35%     
- Complexity      811      812       +1     
============================================
  Files            66       66              
  Lines          2333     2361      +28     
  Branches        329      329              
============================================
+ Hits           1882     1913      +31     
+ Misses          351      349       -2     
+ Partials        100       99       -1

Impacted Files	Coverage Δ	Complexity Δ
.../io/jenkins/plugins/casc/ConfigurationContext.java	`85.71% <100.00%> (+0.52%)`	`19.00 <1.00> (+1.00)`
.../io/jenkins/plugins/casc/SecretSourceResolver.java	`100.00% <100.00%> (+11.53%)`	`8.00 <5.00> (ø)`
...casc/impl/configurators/PrimitiveConfigurator.java	`83.33% <100.00%> (ø)`	`9.00 <1.00> (ø)`

jetersen · 2020-05-30T21:24:35Z

plugin/src/main/java/io/jenkins/plugins/casc/SecretSourceResolver.java

-        public String lookup(final String key) {
-            if (key == null) {
-                return null;
-            }
+        public String lookup(@NonNull final String key) {


Can never be null

jetersen · 2020-05-30T21:30:02Z

This might interest the kubernetes Jenkins helm chart and Jenkins operator maintainers:
cc @lachie83 @viglesiasce @maorfr @torstenwalter @mogaal @wmcdona89 @tomaszsek

jetersen · 2020-05-30T23:04:05Z

plugin/src/main/java/io/jenkins/plugins/casc/ConfigurationContext.java

@@ -21,7 +21,7 @@
    private Deprecation deprecation = Deprecation.reject;
    private Restriction restriction = Restriction.reject;
    private Unknown unknown = Unknown.reject;
-    private final int yamlMaxAliasesForCollections;
+    private transient final int yamlMaxAliasesForCollections;


Should have been transient since it should not be serialized 😓

jetersen · 2020-05-30T23:05:13Z

plugin/src/main/java/io/jenkins/plugins/casc/SecretSourceResolver.java

+    private final StringSubstitutor nullSubstitutor;
+    private final StringSubstitutor substitutor;
+
+    public SecretSourceResolver(ConfigurationContext configurationContext) {


Made the class none static to reduce memory footprint of the StringSubstitutor when JCasC is not actively using them.

jetersen · 2020-05-30T23:18:56Z

plugin/src/main/java/io/jenkins/plugins/casc/SecretSourceResolver.java

+    /**
+     * Resolve string with potential secrets
+     *
+     * @param context Configuration context
+     * @param toInterpolate potential variables that need to revealed
+     * @return original string with any secrets that could be resolved if secrets could not be
+     * resolved they will be defaulted to default value defined by ':-', otherwise default to empty
+     * String secrets are defined as anything enclosed by '${}'
+     * @since 1.42
+     * @deprecated use ${link {@link #resolve(String)}} instead.
+     */
+    @Deprecated
    public static String resolve(ConfigurationContext context, String toInterpolate) {
-        return substitutor(context).replace(toInterpolate);
+        return context.getSecretSourceResolver().resolve(toInterpolate);
    }


Could potentially remove this as usually this is only called directly from test packages in other plugins ie. HashiCorp Vault plugin.

Depeneds if we are okay with yet another breakage 😆

You could restrict it so when they upgrade they are forced to migrate to the new method?

olivergondza · 2020-05-31T12:30:56Z

@jetersen, Cool, thanks for looking into this!

Since the "lookup token" is inside the curly brackets, can file and base64 be used as a regular variable names? (Thinking of backward compatibility with existing clients that happen to use those)

jetersen · 2020-05-31T12:38:26Z

@olivergondza not sure I entirely understand the question.

Most people I seen have been using variable names such as ${BASE64_GOOGLE_PLAY_TOKEN}

that case is very unlikely as it would have to be ${base64:word} before the nested interpolation takes place, also we did not support nested cases before.

Yes you can have a case of ${base64:${GITHUB_PASSWORD}} or even further nested ${base64:${file:/run/secrets/${TEAM_A_GOOGLE_PLAY_TOKEN}.json}}

jetersen · 2020-05-31T12:40:27Z

I added benchmark which revealed some nice performance increases.

FYI this speed increase is definitely noticeable when loading configuration. 👏
Basically all YAML values goes though this method.

I will not be adding the benchmark to the CI runs just yet and the current benchmark only provides value for SecretSourceResolver.

Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>

jetersen · 2020-06-05T01:56:04Z

Hmm I discovered that secretBytes and plain credentials actually works fine with a binary base64 for the file content.

So perhaps we could settle for two base64 and file. Though you could no longer combine the two as file would also be.

I am tempted to combine it to one to reduce confusion even more.

Than inside the base64 lookup we would just check if a path exist.

Yeah to avoid any confusion I think a singular base64 would solve the problem nicely and we can always add more lookups if we find a good use-case.

Right now we are just trying to make the credential experience better!

jetersen · 2020-06-05T02:03:28Z

Now that I think about it and started making the code change... I thought about the user experience 😆

Users would get unexpected secret results when files were not found since they would suddenly have secrets with file path in base64.

jetersen · 2020-06-05T02:09:27Z

As our DockerSecretSource already reads file content into a string you can combine that with ${base64:${DOCKER_SECRET}} where docker secret is read from /run/secrets/DOCKER_SECRET.
You can see the code here:

configuration-as-code-plugin/plugin/src/main/java/io/jenkins/plugins/casc/impl/secrets/DockerSecretSource.java

Lines 20 to 40 in dcd748d

    
           public class DockerSecretSource extends SecretSource { 
        
               public static final String DOCKER_SECRETS = "/run/secrets/"; 
        
               private final File secrets; 
        
               @SuppressFBWarnings("DMI_HARDCODED_ABSOLUTE_FILENAME") 
        
               public DockerSecretSource() { 
        
                   String s = System.getenv("SECRETS"); 
        
                   secrets = s != null ? new File(s) : new File(DOCKER_SECRETS); 
        
               } 
        
               @Override 
        
               public Optional<String> reveal(String secret) throws IOException { 
        
                   if (StringUtils.isBlank(secret)) return Optional.empty(); 
        
                   final File file = new File(secrets, secret); 
        
                   if (file.exists()) { 
        
                       return Optional.of(FileUtils.readFileToString(file, StandardCharsets.UTF_8).trim()); 
        
                   } 
        
                   return Optional.empty(); 
        
               } 
        
           }

timja · 2020-07-01T08:10:07Z

@jetersen is there anything left to do on this?

prom3theu5 · 2020-07-18T19:24:59Z

Any idea how long is left on this one?
In a position where it's integral to us

jetersen · 2020-07-18T22:44:08Z

@timja after rethinking the issue. I think I want to reduce the complexity by having more complex logic that detects whether ${base64:VALUE} is a file or not to avoid introducing multiple implementations.

jetersen · 2020-07-18T23:22:14Z

@prom3theu5 which part? the base64 encoding?

jetersen · 2020-07-19T00:09:09Z

@timja after rethinking the issue. I think I want to reduce the complexity by having more complex logic that detects whether ${base64:VALUE} is a file or not to avoid introducing multiple implementations.

after making the code changes... nothing but errors would occur on so many level... What is a valid path? is it a readable file? Do we log it?.... ARGH
I feel like I already tried 😓 https://github.com/jenkinsci/configuration-as-code-plugin/pull/1408/files#issuecomment-639213839

jetersen · 2020-07-19T00:38:10Z

@timja I think this is ready we can always improve it.

prom3theu5 · 2020-07-19T00:50:38Z

@prom3theu5 which part? the base64 encoding?

Yes - base64 encoding, file support not so much right now

sarabesh · 2020-07-20T14:11:14Z

@jetersen , when can i get this commit as a release, since i am using jenkins operator, where the base plugins can't be added manually,.and casc is a base plugin. We can only specify the plugin versions.

jetersen · 2020-07-20T14:12:12Z

You should be able to point plugins to a url as far as I know.

jetersen · 2020-07-21T14:57:14Z

@sarabesh in any case it is now released with v1.42 :)

olivergondza · 2020-07-21T15:01:49Z

@jetersen, thanks!

SecretResolver support file and base64 variable expansion

8ec34d2

jetersen requested a review from a team as a code owner May 30, 2020 19:57

jetersen commented May 30, 2020

View reviewed changes

plugin/src/main/java/io/jenkins/plugins/casc/SecretSourceResolver.java Outdated Show resolved Hide resolved

jetersen added 3 commits May 30, 2020 22:13

imports...

eda3f9d

add file test with relative and spaces

ee745cb

lift escapeEnclosedBy to final static string

f604878

timja approved these changes May 30, 2020

View reviewed changes

commons libraries are such a pain to dependency manage 😭

62dcf9d

This was referenced May 30, 2020

PKCS12 cert doesn't work anymore #909

Closed

File credentials' secretBytes doesn't get replaced #884

Closed

jetersen added 2 commits May 30, 2020 23:21

coverage

6ed0b53

remove useless test

099bc8a

jetersen commented May 30, 2020

View reviewed changes

jetersen added the feature A PR that adds a feature - used by Release Drafter label May 30, 2020

less static state

355c5b0

jetersen commented May 30, 2020

View reviewed changes

jetersen added 2 commits May 31, 2020 01:06

format javadoc

cb8597b

imports...

67c8481

jetersen commented May 30, 2020

View reviewed changes

jetersen added 5 commits May 31, 2020 10:38

Add DoNotUse restricted annotations

7c6d5a3

java doc

4f4989a

Increase performance when no secrets are detected.

5ab0efc

add Benchmark

60fa76c

naming

468058b

jetersen and others added 2 commits June 5, 2020 02:21

quote them secrets

cc8ef38

Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>

tests

7d4caca

jetersen added 2 commits June 5, 2020 04:34

add binary secret file test

bb8eb1a

docs

cc2693e

jetersen mentioned this pull request Jun 11, 2020

Configure Script Approvals #1418

Open

1 task

Merge branch 'master' into fix/1219

352fca9

jetersen added 2 commits July 19, 2020 02:17

Include InvalidPathException

bf1617b

add aliases

8f385c1

imports

64bebfc

jetersen added 5 commits July 19, 2020 03:02

bump dependency management

a2a18e6

prefer usage of readFile

167c974

use latest version of commons-text

e340d79

bump commons-text in integrations as well

a86d8f0

Merge branch 'master' into fix/1219

f1e7737

jetersen merged commit 410c3ae into jenkinsci:master Jul 19, 2020

jetersen deleted the fix/1219 branch July 19, 2020 02:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SecretResolver support file and base64 variable expansion #1408

SecretResolver support file and base64 variable expansion #1408

jetersen commented May 30, 2020 •

edited

Loading

codecov bot commented May 30, 2020 •

edited

Loading

jetersen May 30, 2020

jetersen commented May 30, 2020 •

edited

Loading

jetersen May 30, 2020

jetersen May 30, 2020 •

edited

Loading

jetersen May 30, 2020

timja May 31, 2020

jetersen May 31, 2020

olivergondza commented May 31, 2020

jetersen commented May 31, 2020

jetersen commented May 31, 2020 •

edited

Loading

jetersen commented Jun 5, 2020

jetersen commented Jun 5, 2020 •

edited

Loading

jetersen commented Jun 5, 2020

timja commented Jul 1, 2020

prom3theu5 commented Jul 18, 2020

jetersen commented Jul 18, 2020 •

edited

Loading

jetersen commented Jul 18, 2020

jetersen commented Jul 19, 2020 •

edited

Loading

jetersen commented Jul 19, 2020

prom3theu5 commented Jul 19, 2020

sarabesh commented Jul 20, 2020

jetersen commented Jul 20, 2020

jetersen commented Jul 21, 2020

olivergondza commented Jul 21, 2020

SecretResolver support file and base64 variable expansion #1408

SecretResolver support file and base64 variable expansion #1408

Conversation

jetersen commented May 30, 2020 • edited Loading

Your checklist for this pull request

codecov bot commented May 30, 2020 • edited Loading

Codecov Report

jetersen May 30, 2020

Choose a reason for hiding this comment

jetersen commented May 30, 2020 • edited Loading

jetersen May 30, 2020

Choose a reason for hiding this comment

jetersen May 30, 2020 • edited Loading

Choose a reason for hiding this comment

jetersen May 30, 2020

Choose a reason for hiding this comment

timja May 31, 2020

Choose a reason for hiding this comment

jetersen May 31, 2020

Choose a reason for hiding this comment

olivergondza commented May 31, 2020

jetersen commented May 31, 2020

jetersen commented May 31, 2020 • edited Loading

jetersen commented Jun 5, 2020

jetersen commented Jun 5, 2020 • edited Loading

jetersen commented Jun 5, 2020

timja commented Jul 1, 2020

prom3theu5 commented Jul 18, 2020

jetersen commented Jul 18, 2020 • edited Loading

jetersen commented Jul 18, 2020

jetersen commented Jul 19, 2020 • edited Loading

jetersen commented Jul 19, 2020

prom3theu5 commented Jul 19, 2020

sarabesh commented Jul 20, 2020

jetersen commented Jul 20, 2020

jetersen commented Jul 21, 2020

olivergondza commented Jul 21, 2020

jetersen commented May 30, 2020 •

edited

Loading

codecov bot commented May 30, 2020 •

edited

Loading

jetersen commented May 30, 2020 •

edited

Loading

jetersen May 30, 2020 •

edited

Loading

jetersen commented May 31, 2020 •

edited

Loading

jetersen commented Jun 5, 2020 •

edited

Loading

jetersen commented Jul 18, 2020 •

edited

Loading

jetersen commented Jul 19, 2020 •

edited

Loading