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

Support passing puppetdb certificates via environment variables #671

Merged
merged 2 commits into from
Mar 13, 2022

Conversation

SeanHood
Copy link
Contributor

@SeanHood SeanHood commented Mar 3, 2022

Closes #669

Adds support for passing certificates and keys to the following environment variables:

  • PUPPETDB_SSL_VERIFY
  • PUPPETDB_KEY
  • PUPPETDB_CERT

I have tested this in my own environment as per the issue. With the container running in AWS Fargate, certificates being passed as env vars from AWS SSM Parameter store.

I had ran into a couple edge cases where the shell was breaking the new lines. These feel like general environment variable edge cases and not so much an issue with this code.

@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #671 (c32bd58) into master (2d5e44a) will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
+ Coverage   88.12%   88.32%   +0.19%     
==========================================
  Files          18       18              
  Lines         943      959      +16     
==========================================
+ Hits          831      847      +16     
  Misses        112      112              
Impacted Files Coverage Δ
puppetboard/docker_settings.py 100.00% <100.00%> (ø)

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 2d5e44a...c32bd58. Read the comment docs.

@gdubicki
Copy link
Member

gdubicki commented Mar 3, 2022

Thanks @SeanHood!

If this works for you then we can merge it. But if you write that:

I had ran into a couple edge cases where the shell was breaking the new lines. These feel like general environment variable edge cases and not so much an issue with this code.

...then perhaps you should consider implementing support for base64-encoded strings, without line wrapping, as input? This is a common solution for passing file contents in a env variables when the file may contain new lines and you want to avoid having them in the variable because it causes problems. (Yes, I know that the certificates in PEM are internally base64-encoded too but they DO have wrapped lines and thus the new lines.)

@SeanHood
Copy link
Contributor Author

SeanHood commented Mar 4, 2022

That's a good shout, I'll add that to this PR.

As for where I've put this code does it make sense?

@gdubicki
Copy link
Member

gdubicki commented Mar 4, 2022

As for where I've put this code does it make sense?

Yes, for this purpose it makes perfect sense.

(Of course we could start a long design discussion that because it could be useful for non-dockerized Puppetboard, it should be moved out of docker_settings.py. Or that it should rather be implemented in the upstream pypuppetdb. Or that we should consider using something more secure than /tmp to keep the certificates (especially in non-dockerized env, with other processes possibly running on the host), so maybe we should use memory-tempfile module and so on and on, but this would be overdesign at this point. We can get back to these ideas in the future, when more people will be interested in the feature. 😊)

@gdubicki
Copy link
Member

gdubicki commented Mar 7, 2022

Please also add docs update, @SeanHood.

@SeanHood
Copy link
Contributor Author

SeanHood commented Mar 9, 2022

As for where I've put this code does it make sense?

Yes, for this purpose it makes perfect sense.

(Of course we could start a long design discussion that because it could be useful for non-dockerized Puppetboard, it should be moved out of docker_settings.py. Or that it should rather be implemented in the upstream pypuppetdb. Or that we should consider using something more secure than /tmp to keep the certificates (especially in non-dockerized env, with other processes possibly running on the host), so maybe we should use memory-tempfile module and so on and on, but this would be overdesign at this point. We can get back to these ideas in the future, when more people will be interested in the feature. 😊)

100% onboard with you there.

puppetboard/docker_settings.py Show resolved Hide resolved
puppetboard/docker_settings.py Show resolved Hide resolved
@gdubicki
Copy link
Member

Thanks for the code, @SeanHood, and for the code review, @russellcain!

@gdubicki gdubicki merged commit 5c543b1 into voxpupuli:master Mar 13, 2022
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: Accept certificate and key as strings
3 participants