Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JENKINS-60373: Read secret from file and encode it with base64 #1219

Closed
olivergondza opened this issue Dec 6, 2019 · 15 comments · Fixed by #1408
Closed

JENKINS-60373: Read secret from file and encode it with base64 #1219

olivergondza opened this issue Dec 6, 2019 · 15 comments · Fixed by #1408
Assignees
Labels
feature A PR that adds a feature - used by Release Drafter

Comments

@olivergondza
Copy link
Member

Feature Request

I am looking for a way to practically and securely inject binary secrets from /run/secret into a file credential type (plain-credentials plugin). The challenge is, the input is expected to be base64 encoded so reading the file as-is from /run/secrets does not work. Extending plain-credentials to read the file content instead of expecting inlined string is something that was proven challenging to secure. Several approaches to address that was discussed in JENKINS-60373 and I would like to propose the most straightforward one[1].


Introduce a new syntax, that would permit reading file from filesystem and encoding the content with base64 to close the gap between traditional secrets injection and file credential. The reasoning is that when the feature is specific for JCasC, it is by definition restricted to administrators eliminating possible arbitrary file read vulnerabilities.

[1] https://issues.jenkins-ci.org/browse/JENKINS-60373?focusedCommentId=381101&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-381101

@olivergondza olivergondza added the feature A PR that adds a feature - used by Release Drafter label Dec 6, 2019
@olivergondza
Copy link
Member Author

The suggested declaration format is:

secretBytes: "$base64file(/run/secrets/whatever)" # Read the file and encode content

or perhaps

secretBytes: "$base64(${whatever})" # Inject secret and encode it

@timja
Copy link
Member

timja commented Dec 6, 2019

Sounds like a reasonable approach that has caused pain for people using certain plugins for a long time

@olivergondza
Copy link
Member Author

@timja, great to hear that. If you can point to some other use-cases to keep in mind while implementing that, that would be good.

@timja
Copy link
Member

timja commented Dec 6, 2019

There's some google plugins that have similar issues, can't remember off the top of my head though

@jglick
Copy link
Member

jglick commented Dec 6, 2019

The suggested declaration format is

Note that this was just a dashed-off example. Probably you can find a more intuitive / readable syntax studying some other configuration languages.

@jetersen
Copy link
Member

jetersen commented Dec 6, 2019

${base64('base64string')} ${base64('/base/64/file')} ${base64('${secret}')}?

Test secret string whether it is path or base 64 string encoded?
Secret source reveal will should help here 🤔

@sshepel
Copy link
Contributor

sshepel commented May 30, 2020

Any updates on this?

@jetersen
Copy link
Member

jetersen commented May 30, 2020

@jglick @olivergondza @timja
Switching to apache's StringSubstitutor would enable this
Though the default lookup might be too powerful for JCasC use case.

Though it will cleanup some of the code as StringSubstitutor already supports default value via :-
Also supports nested variable substitution.
So you could have something like

${base64:${file:${/run/secrets/whatever}}}
Default String Lookups
Key	Implementation	Factory Method	Since
"base64Decoder"	Base64DecoderStringLookup	base64DecoderStringLookup()	1.6
"base64Encoder"	Base64EncoderStringLookup	base64EncoderStringLookup()	1.6
"const"	ConstantStringLookup	constantStringLookup()	1.5
"date"	DateStringLookup	dateStringLookup()	1.5
"dns"	DnsStringLookup	dnsStringLookup()	1.8
"env"	EnvironmentVariableStringLookup	environmentVariableStringLookup()	1.3
"file"	FileStringLookup	fileStringLookup()	1.5
"java"	JavaPlatformStringLookup	javaPlatformStringLookup()	1.5
"localhost"	LocalHostStringLookup	localHostStringLookup()	1.3
"properties"	PropertiesStringLookup	propertiesStringLookup()	1.5
"resourceBundle"	ResourceBundleStringLookup	resourceBundleStringLookup()	1.6
"script"	ScriptStringLookup	scriptStringLookup()	1.5
"sys"	SystemPropertyStringLookup	systemPropertyStringLookup()	1.3
"url"	UrlStringLookup	urlStringLookup()	1.5
"urlDecoder"	UrlDecoderStringLookup	urlDecoderStringLookup()	1.5
"urlEncoder"	UrlEncoderStringLookup	urlEncoderStringLookup()	1.5
"xml"	XmlStringLookup	xmlStringLookup()	1.5

I would properly settle for:

"base64" Base64EncoderStringLookup
"file" FileStringLookup

@jetersen
Copy link
Member

For my own sanity I will extend FileStringLookup to only allow UTF-8.

@jetersen
Copy link
Member

jetersen commented May 30, 2020

ARGH... StringSubstitutor 😭 Does not have a nice way to handle our existing cases when it comes to defaulting and properly logging it.

I am left to use second StringSubstitutor for replacing none replaced values...

@jetersen jetersen self-assigned this May 30, 2020
@jetersen
Copy link
Member

jetersen commented May 30, 2020

Really happy that we had such good test coverage of our SecretResolver.
I have made some code improvements that should clearly improve performance 👏

I would actually like to benchmark it @oleg-nenashev @AbhyudayaSharma 😅

@jetersen
Copy link
Member

jetersen commented May 30, 2020

Syntax would look like this:

${base64:${file:path to file}}
${base64:${file:${YET_ANOTHER_VARIABLE}}}

Hopefully that is acceptable?

@AbhyudayaSharma
Copy link
Member

@jetersen https://www.jenkins.io/blog/2019/06/21/performance-testing-jenkins/ should get you started. It will be great to have these benchmarks in JCasC. Please let me know if you need any help.

@jetersen
Copy link
Member

jetersen commented May 31, 2020

@AbhyudayaSharma I did some quick initial simple tests.

image

Though SecretSourceResolver is actually on the critical path meaning all values in the JCasC yaml has to go through it.

So I have added a test that sort of simulates it by calling it 100 times, which is also really slow 😅 (Stilling waiting on results from first run).

I should try and add a test case for actually going through jcasc.Configure method though it might be terribly slow

@jetersen
Copy link
Member

Ah I would need to use different BenchmarkMode lucky there are annotations for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A PR that adds a feature - used by Release Drafter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants