Add file backed certificate authority #280
Conversation
|
I'm concerned about supporting unencrypted private keys on disk, beyond something that is meant for testing (which we already have the ephemeral CA). In the issue, it was brought up that however the key is put onto disk is left up to the user. Do we think it might be better to take a stance on this? Otherwise, those that run their own instances of Fulcio may end up making decisions that don't adequately protect the signing key. The goal of this PR is to support dynamically injecting an intermediate. Is there another way that we could accomplish this, without having the key be left unencrypted? One option could be creating an abstraction layer for where the key lives, i.e Vault, a keyring, etc, and watching that layer for changes. What do you think? |
|
Generally, I think software should be flexible and the documentation should be great / educational. This bit adds more flexibility for operators, but we're sorely lacking in documentation to help folks understand what good looks like and how to operate Fulcio themselves. That said, I'm happy to flow with what ever ya'll think! I can add caveats like testing or unsafe around the flags in this PR or simply abandon it if ya'll think it opens a can of worms. I can also add support for passwords on the keyfile too :) |
|
While I think flexibility is valuable, this is a core security product, so the options we design should be secure by default. I definitely agree Fulcio needs more documentation too! A password-protected keyfile seems like a good first step. Though I wonder if it's worth it to implement just a file-based CA. You had mentioned in another issue implementing Vault as a CA. These two approaches seem very similar, which is why I think it'd be useful to have a "CA backed by rotating key" or something along those lines. We could support various key storage options, like Vault or a password-protected keyfile, and that layer could implement crypto.Signer too. |
|
I guess my main ask is why would we want this? If it's to make it easier for users, that's not a very compelling case as its not a tool that is really intended to give a friendly UX, its a service that will run a critical piece of infrastructure. This is why I am personally keen to keep the attack surface of fulcio as lean / low as possible. Would it be better if an option such as softHSM were easier to use? |
nsmith5
force-pushed
the
nfs/fileca
branch
2 times, most recently
from
849b288
to
f6c5497
Compare
nsmith5
changed the title
|
I think this would probably be fine, my main concern is just with the unencrypted PEM file. I know managing the password somewhere else is annoying, but it still seems safer than leaving the private file on disk. There are cases where it's OK to do this, but the password is generally safer. It would also match how the CT log personality for trillian is configured (passphrase/encrypted key). |
There was a problem hiding this comment.
The fileca should enable us to replace the ephemeralca in the e2e test here with one we can actually cosign verify once the job completes, since we need to register the verification key with cosign and there isn't a way to do that with ephemeralca today.
|
I'd suggest filing issues for the follow-ups called out, but otherwise this LGTM. |
|
Thanks for the feedback ya'll! I'll add password protection to the key file and support for a chain of certificates. @mattmoor are you saying we should merge as-is and iterate on those two points with some issues? |
|
I meant the cert chain support, and switching up the e2e test to use this, but feel free to do it here too, if you prefer. |
|
Just want to circle back here to understand whether the plan is to land and follow up on anything remaining or fix it in-place here? |
|
Hey! Sorry I've disappeared into eggnog land so I've haven't picked at this. My plan was to land the password support in this PR as consensus seemed to be that that was definitely a blocker and then iterate on the certificate chain support (basically get it merged if someone else wanted to tackle that and I'm too slow) @haydentherapper hows that sound to you? |
|
Sounds good! If you can also update #276 with the next steps that won't be added in this PR, that'd be great. |
nsmith5
force-pushed
the
nfs/fileca
branch
3 times, most recently
from
f58c021
to
f414226
Compare
|
Alright, this is ready for review again! Updates:
I've got some open questions after completing this work though:
|
|
At a glance, RFC-1423 uses a lot of dated encryption algorithms that are vulnerable to various attacks, like a padding oracle attack. I would prefer we don't support it. PKCS#8 would not support authenticated encryption, this is just the format. The underlying encryption algorithm would be what supports AEAD. It looks like openssl doesn't support AEAD for encrypting a private key (https://www.openssl.org/docs/man1.1.1/man1/openssl-genrsa.html, the set of algorithms are just encryption). A concern for the smallstep utility is that it looks like the only supported encryption algorithms are the set of RFC-1423 algorithms (https://github.com/smallstep/crypto/blob/ee37b94d634ed7b31b477d7535cad491446eeea9/pemutil/pkcs8.go#L191-L192), which are all outdated. I can't find much about what openssl defaults to, I saw one mention it was DES though which is...surprising and not great. This suggests to me that we should not be instructing users to use openssl to encrypt their private keys. For parsing operations, should we build these into https://github.com/sigstore/sigstore? Alternatively, we can use a different way of encrypting (like below) and just use x509.ParsePKCS8PrivateKey. For encryption/decryption, I would prefer we use a trusted library like https://github.com/google/tink or https://github.com/jedisct1/libsodium, which support modern encryption algorithms. Summarizing, the steps would be:
|
|
There's also experimental support for NaCl's secretbox in golang (https://pkg.go.dev/golang.org/x/crypto@v0.0.0-20211215153901-e495a2d5b3d3/nacl/secretbox), which is similar to libsodium or tink - Just give the box a key and bytes, and it handles encryption/decryption for you. Here be dragons, I don't know how experimental this feature is, but it seems simple to use and it's greatly preferred over trying to do this with crypto primitives. Tink or Libsodium will be a similarly simple experience (but you'll need to find go wrappers for each) |
|
Thanks so much for the deep dive and research! I'd love to use NaCL or tink for encryption, but my fear there is simply that the CLI tooling isn't super widely available. Feels like we're stuck between
I've used Also, just wow: its surprising how bizarre and broken the state of private key encryption is 😨 |
|
Yea, surprisingly there seem to be almost no CLIs for encryption. The libraries seems to be built around adding support in clients or servers. Could we add a simple CLI utility to handle encryption? Another alternative is OpenSSL supports more encryption algos through |
|
My two cents: The openssl stuff is fine here. It's standard, even though it's dated and deprecated. The user would most likely have an encrypted pem from whatever tooling they got the cert from, so we can just use that. We shouldn't encourage this pattern, but supporting it where something else generates the file is fine. In cosign we use the nacl secretbox thing for the private keys we generate - but we're working on support to "import" keys from the standard encrypted pem format. |
|
This also matches how the CTFE codebase accepts keys: https://github.com/google/certificate-transparency-go/blob/master/trillian/docs/ManualDeployment.md#key-generation |
|
Would you mind adding a usage here or to keep changes smaller, create an issue tracking adding the docs: Though, I wonder if that should also be added here too (yea, so probably an issue? :) )? And lastly, larger question, do wonder if the two documents might be better served if we combined some of the bits? |
It should all be ported to here https://docs.sigstore.dev/fulcio/overview |
|
Is there any documentation / papers / articles on what is considered insecure in RFC-1423? I can only find a comment in some go code. I am not doubting that there may be and if there is we should react accordingly, but it would be good to see proof, especially if the proposed replacement is an experimental crypto library. |
There are two main issues with the encryptedpem stuff from RFC1423 that I'm aware of. I'm not a cryptographer, so sorry if I mess up any details. The first is that the encryption used here is not authenticated. You can read more about that here. This means that an attacker can modify the ciphertext, which can lead to a padding oracle, breaking encryption completely: https://research.nccgroup.com/2021/02/17/cryptopals-exploiting-cbc-padding-oracles/. This isn't terribly relevant for this use case though, because everything is stored client-side. The second (and much more relevant one for this use case) is that an incredibly weak KDF is used here by the openssl implementation. It's a custom construct based on md5. See https://www.exploit-db.com/papers/41019 or the code here: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/pem_decrypt.go;drc=refs%2Ftags%2Fgo1.17.5;l=82 For this case, where we're passphrase encrypting a file, we need to make sure a much stronger KDF is used to make brute force decrypting much harder. bcrypt, scrypt, or argon are all the current state of the art recommendations. We used scrypt in cosign. More information is here: https://latacora.micro.blog/2018/04/03/cryptographic-right-answers.html Again - I think the encryptpemblock stuff is fine here because the passphrase is made available to the process anyway for online use, and we're aiming for compatiblity. For nacl/secretbox is in golang/x/crypto, but it's not "experimental" by any means. NACL/libsodium have been around for years and are widely considered to be the best practice for most cryptographic primitives. |
|
@vaikas Absolutely! Thanks for calling out the docs. I've added it to the list of follow up work on the original issue here #276. Limme know if you think it makes more sense to break all those bullet points into separate issues. I can do that too. This PR is already sizable and somewhat contentious to merge so I'm keeping it as small as possible 😅 |
Loads private key and certificate from disk. Optionally watches for file changes and loads updated key pair. Signed-off-by: Nathan Smith <nathan@nfsmith.ca>
|
What is the concern with compatibility? Having additional steps to set up a secure CA doesn't seem unreasonable. If the more common option (openssl encryption) is insecure, imo, it shouldn't be an option for a part of the project that's meant to be a root of trust. I recognize the desire for optionality, but I strongly believe every option this CA supports should be secure by default (sans the test ephemeral CA whose use-case is documented). If the industry standard was RFC-1423, I could see an argument for including this. It doesn't seem like there is a good standard for passing encrypted files from disk to a process though. I'd prefer we be an example of good practices. |
| watcher, err := fsnotify.NewWatcher() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| defer watcher.Close() |
| } | ||
|
|
||
| if watch { | ||
| watcher, err := fsnotify.NewWatcher() |
There was a problem hiding this comment.
It might be nice to be able to watcher.Close, but that would require exposing it on ca.CertificateAuthority and doing some plumbing, so I'd open an issue to follow up, if we think it matters.
Thanks for the pointers and I won't cloud the issue with more discussion, if you're all good with the approach. FYI openssl changed the default from md5 to sha256 quite a while ago after the collision attack stuff came to light . |
I'm not sure that's the same thing - that's the default digest but the default KDF is still their proprietary EVP_BytesToKey: https://www.openssl.org/docs/man3.0/man3/EVP_BytesToKey.html Some new builds allow specifying an alternate KDF, but the version on my Mac doesn't appear to be recent enough. |
|
This looks good enough for a merge! Let's tackle the remaining TODOs in followups. |
Same function, take a look at this SO post: https://stackoverflow.com/a/9500692
|
|
Oof, the default mac one must be pretty old. Mine doesn't support specifying the KDF and the default is still MD5 :( |
Seems to be 2016 it released. Thanks for bringing this up though, made me really refresh my knowledge around what's the latest in kdf / salting. |
Summary
Adds a simple file-based certificate authority to Fulcio. Expects PEM encoded key-pair without password protection.
Usage
By default the file passed are watched for updates and reloaded on change. This behaviour can be disabled with
---fileca-watch false.Fixes #276
Remaining Work
Release Note