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

feat: implement file template function #394

Closed
wants to merge 1 commit into from

Conversation

hbagdi
Copy link
Member

@hbagdi hbagdi commented May 18, 2021

Fix #91

@hbagdi
Copy link
Member Author

hbagdi commented May 18, 2021

@mflendrich @rainest How does this implementation look to you? I'm looking for some early feedback. I intend to add tests for this once this approach looks good.

@hbagdi hbagdi requested review from rainest and mflendrich May 18, 2021 23:44
@codecov-commenter
Copy link

Codecov Report

Merging #394 (3c9baeb) into main (829116a) will decrease coverage by 6.53%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
- Coverage   53.59%   47.06%   -6.54%     
==========================================
  Files          62       87      +25     
  Lines        5034     6440    +1406     
==========================================
+ Hits         2698     3031     +333     
- Misses       2037     3043    +1006     
- Partials      299      366      +67     
Impacted Files Coverage Δ
file/readfile.go 80.45% <33.33%> (-3.69%) ⬇️
cmd/root.go 50.73% <0.00%> (-6.29%) ⬇️
cmd/konnect_diff.go 68.00% <0.00%> (-5.69%) ⬇️
cmd/konnect_dump.go 20.89% <0.00%> (-5.64%) ⬇️
cmd/konnect_sync.go 61.90% <0.00%> (-4.77%) ⬇️
state/builder.go 0.00% <0.00%> (ø)
solver/target.go 0.00% <0.00%> (ø)
solver/plugin.go 0.00% <0.00%> (ø)
state/document.go 71.42% <0.00%> (ø)
solver/ca_cert.go 0.00% <0.00%> (ø)
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 829116a...3c9baeb. Read the comment docs.

Copy link
Contributor

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that allowing arbitrary FS access is a big deal from the security perspective: I am not convinced that benefits outweigh the security opportunity cost, considering the following alternative solution:

You can achieve the equivalent result using #286 by reading the file into an environment variable

env "DECK_FILE_AAA=$(cat /my/file.pem)" "DECK_FILE_BBB=$(cat /my/other/file.pem)" deck sync ...

and then using the env substitution function. This should be enough for CI use.

The solution described above seems to solve the problem pointed out in #91, at the same time not permitting unbounded FS access for a user who controls the decK file.

That's a -1 from me for implementing of file template function, because of that security concern.

@rainest
Copy link
Contributor

rainest commented May 19, 2021

Does loading them into environment variables change anything? The user running deck needs read permissions to the file whether they're cat-ing them into the environment before invoking deck or having deck load them in via the template.

While env loading can achieve the same, it could be a bit unwieldy with large numbers of certs.

@hbagdi
Copy link
Member Author

hbagdi commented Jul 12, 2021

Holding off on this for now.
We will revisit this later.

@hbagdi hbagdi closed this Jul 12, 2021
@hbagdi hbagdi deleted the feat/file-template-function branch February 9, 2022 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: add support to read certificate private keys from file-system
4 participants