-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
contrib: Dump keys/certs from acme.json to files #1484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I left a few comments.
contrib/scripts/dumpcerts.sh
Outdated
@@ -0,0 +1,141 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/usr/bin/env bash
is more portable and thus preferred.
contrib/scripts/dumpcerts.sh
Outdated
# 4 - The destination certificate directory does not exist | ||
# 8 - Missing private key | ||
|
||
USAGE="dumpcerts.sh <path to acme> <destination cert directory>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use $(basename "$0")
to display the script name in order to be portable should we ever rename the file?
# 1 - A component is missing or could not be read | ||
# 2 - There was a problem reading acme.json | ||
# 4 - The destination certificate directory does not exist | ||
# 8 - Missing private key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which exit codes one is allowed to use is a bit disputed; personally, I'm okay with using the "reserved" ones in the low-digit range.
Why powers of two though instead of just 1, 2, 3, and 4? We don't really need to shift numbers around, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would just be so that in the long run you could continue through a number of failure situations and then give the user a composite at the end which could be used to identify all of the things that have gone wrong before doing anything (potentially) destructive.
contrib/scripts/dumpcerts.sh
Outdated
echo "" | ||
echo "You must have the binary 'jq' to use this." | ||
echo "jq is available at: https://stedolan.github.io/jq/download/" | ||
echo "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please output the message to stderr (>&2
) using either a here document or a string stretching over multiple lines (SO answer).
Same for other error outputs below.
contrib/scripts/dumpcerts.sh
Outdated
USAGE="dumpcerts.sh <path to acme> <destination cert directory>" | ||
|
||
# Allow us to exit on a missing jq binary | ||
exitJQ() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a naming convention using only lower-case and underscores (exit_jq
), like the Google style guide recommends as well.
contrib/scripts/dumpcerts.sh
Outdated
# after writing the private key switch it back to the default | ||
oldumask=$(umask) | ||
umask 177 | ||
# For some reason traefik stores the private key in stripped base64 format, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to know the reason and phrase it here.
Maybe @emilevauge or @dtomcej know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timoreimann what is your point exactly ? base64 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emilevauge Yes. I think it would be nice to rephrase
For some reason traefik stores the private key in stripped base64 format
into something like
traefik stores the private key in stripped base64 format because [reason]
or just
traefik stores the private key in stripped base64 format (without explanation)
I don't think we should leave comments in scripts that express uncertainty about the reasoning of our own code if we can avoid it.
contrib/scripts/dumpcerts.sh
Outdated
} | ||
|
||
|
||
acme="${1}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd readonly
the variable.
contrib/scripts/dumpcerts.sh
Outdated
|
||
|
||
acme="${1}" | ||
certdir="${2}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd readonly
the variable.
contrib/scripts/dumpcerts.sh
Outdated
# and place each in a variable for later use, normalizing the path | ||
mkdir -p "${certdir}"/{certs,private} | ||
|
||
pdir=$(realpath "${certdir}/private/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason we need to use realpath
? AFAIK, it's not available on all systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was only really using this to normalize paths in the event of ${certdir}
being provided with a trailing slash. I was of two minds about this. realpath
is a part of coreutils
and thus should be on the lion's share of hosts (at least where traefik would be able to run). On the other hand it is a better answer to just guarantee this at the time of assignment via ${certdir%/}
... Admittedly this was me making this mostly for myself and thinking of it as "contrib" status work (YMMV). Regardless, will fix with the assignment.
# Save the existing umask, change the default mode to 600, then | ||
# after writing the private key switch it back to the default | ||
oldumask=$(umask) | ||
umask 177 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use a TRAP
to revert the umask in order to cover for the case where the script breaks anywhere between this line and the point where we return to the old umask?
@brianredbeard Wow great 👍 |
@emilevauge More than likely this would be a good idea, that being said this was a case of something that I was solving for myself anyways and figured could be useful for others. I'd say making something like this available until a better answer is available (and then burning this script in a fire) is more than acceptable. |
@brianredbeard It's clearly acceptable, can you fix the comments made by @timoreimann ? Thanks. @emilevauge I've opened the issue #1512 for make the command in Traefik. |
Ping @brianredbeard |
In the event that a user needs to explode their acme.json file into a set of directories and relevant files for troubleshooting or use with other programs this script will parse them into the components in the following path structure: ``` certdir ├── certs │ ├── domain-1.example.com │ ├── domain-2.example.com │ └── domain-n.example.com └── private └── letsencrypt.key ```
@brianredbeard I carried your PR "in-place" and addressed all of my own comments that seemed undisputed. Hope that's okay with you. Feel free to comment back on me, of course. @emilevauge PTAL. |
That's absolutely fine by me! Sorry about being a drag ass but you have my full support. |
Due to SemaphoreCI, I close and reopen the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @brianredbeard and @timoreimann ❤️
LGTM
Will this dump the certs automatically in real time? I'd like to mount a volume with my certs into gitlab and a mailserver, so am looking to find a folder with |
hi @brianredbeard. First of all, thanks for the contribution. I have another (quick) follow up question. From how I understand let's encrypt it has two certificates. This script is extracting the domain handling key into a .key file and the certificate into the .pem file. But it doesn't extract the private key for the individual certificate (i.e. DomainsCertificate.Certs[n].Certificate.PrivateKey). Gitlab Container Registry is asking me to provide both, so I did the base64 extraction manually to confirm that this was actually required. I suppose extracting the PrivateKey would only be a minor duplication in the script. tl;dr Is there a reason you chose not to extract the PrivateKey? (or just didn't require it?) |
someone know about it:
|
@frederik could you please share your current workaround? I might need to pass a private key to gitlab or another upstream service in near future :–) |
I took the contents from DomainsCertificate.Certs[n].Certificate.PrivateKey and base64 decoded it by hand.
(on linux -d, on Mac -D) .. to a file and referenced the key in gitlab.
|
have you tried with the new version ? #2161 |
I have now. This works for me. 👌 |
Getting an error in line 71 with this
"ist nicht gesetzt" means "is not set" |
Couldn’t this be a built in feature of traefik? Dumping the certs to a volume to be shared live with other software? Why require an external cron-driven export command to be issued, adding complexity for the use-case? |
In the event that a user needs to explode their acme.json file into
a set of directories and relevant files for troubleshooting or use
with other programs this script will parse them into the components
in the following path structure: